[autofill] Avoid duplicate instances of the SaveCardBubble.

chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/autofill/save_card_bubble_controller_impl.h"
 
 #include <stddef.h>
 
 #include "chrome/browser/ui/autofill/save_card_bubble_view.h"
 #include "chrome/browser/ui/browser.h"
@@ skipping 26 matching lines @@
 
 SaveCardBubbleControllerImpl::SaveCardBubbleControllerImpl(
     content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       save_card_bubble_view_(nullptr),
       pref_service_(
           user_prefs::UserPrefs::Get(web_contents->GetBrowserContext())) {}
 
 SaveCardBubbleControllerImpl::~SaveCardBubbleControllerImpl() {
   if (save_card_bubble_view_)
     save_card_bubble_view_->Hide();

[Jared Saul, 2017/05/04 20:51:12]

Why did this line let the bug exist?  If the controller was cleaned up,
shouldn't the popup have gone away?

[Roger McFarlane (Chromium), 2017/05/05 16:16:11]

the problem is not in the cleanup, it's in the Show.

The Show paths were not checking that if a bubble was already shown. Instead,
they just overwrote save_card_bubble_view_ to point to a new bubble instance,
effectively leaking the previous bubble.

 }
 
 void SaveCardBubbleControllerImpl::ShowBubbleForLocalSave(
     const CreditCard& card,
     const base::Closure& save_card_callback) {
+  // Don't show the bubble if it's already visible.
+  if (save_card_bubble_view_)
+    return;
+
   is_uploading_ = false;
   is_reshow_ = false;
   should_cvc_be_requested_ = false;
   legal_message_lines_.clear();
 
   AutofillMetrics::LogSaveCardPromptMetric(
       AutofillMetrics::SAVE_CARD_PROMPT_SHOW_REQUESTED, is_uploading_,
       is_reshow_,
       pref_service_->GetInteger(
           prefs::kAutofillAcceptSaveCreditCardPromptState));
 
   card_ = card;
   save_card_callback_ = save_card_callback;
   ShowBubble();
 }
 
 void SaveCardBubbleControllerImpl::ShowBubbleForUpload(
     const CreditCard& card,
     std::unique_ptr<base::DictionaryValue> legal_message,
     bool should_cvc_be_requested,
     const base::Closure& save_card_callback) {
+  // Don't show the bubble if it's already visible.
+  if (save_card_bubble_view_)
+    return;
+
   is_uploading_ = true;
   is_reshow_ = false;
   should_cvc_be_requested_ = should_cvc_be_requested;
   AutofillMetrics::LogSaveCardPromptMetric(
       AutofillMetrics::SAVE_CARD_PROMPT_SHOW_REQUESTED, is_uploading_,
       is_reshow_,
       pref_service_->GetInteger(
           prefs::kAutofillAcceptSaveCreditCardPromptState));
 
   if (!LegalMessageLine::Parse(*legal_message, &legal_message_lines_)) {
@@ skipping 11 matching lines @@
 }
 
 void SaveCardBubbleControllerImpl::HideBubble() {
   if (save_card_bubble_view_) {
     save_card_bubble_view_->Hide();
     save_card_bubble_view_ = nullptr;
   }
 }
 
 void SaveCardBubbleControllerImpl::ReshowBubble() {
+  // Don't show the bubble if it's already visible.
+  if (save_card_bubble_view_)

[Jared Saul, 2017/05/04 20:51:12]

If the icon in the Omnibox is clicked which hides the bubble, then reclicked to
reshow the bubble, what's the value of save_card_bubble_view_?  Is it truly
null?

Because I'm assuming if save_card_bubble_view_->Hide() is called, that doesn't
mean save_card_bubble_view_ no longer exists, because I'd expect
save_card_bubble_view->Show() to bring it back.

[Roger McFarlane (Chromium), 2017/05/05 16:16:11]

The bubble actually manages it's own lifetime. We're just managing the
controllers references to it and its references to the controller.

SaveCardBubbleControllerImpl::HideBubble() asks the view to Hide() itself then
discards it's pointer to the view (see line 108). SaveBubbleCardViews::Hide()
both hides the view and discards its pointer back to the controller. Showing the
view again doesn't reuse the view, it creates a new one.

+    return;
+
   is_reshow_ = true;
   AutofillMetrics::LogSaveCardPromptMetric(
       AutofillMetrics::SAVE_CARD_PROMPT_SHOW_REQUESTED, is_uploading_,
       is_reshow_,
       pref_service_->GetInteger(
           prefs::kAutofillAcceptSaveCreditCardPromptState));
 
   ShowBubble();
 }
 
@@ skipping 176 matching lines @@
   location_bar->UpdateSaveCreditCardIcon();
 }
 
 void SaveCardBubbleControllerImpl::OpenUrl(const GURL& url) {
   web_contents()->OpenURL(content::OpenURLParams(
       url, content::Referrer(), WindowOpenDisposition::NEW_FOREGROUND_TAB,
       ui::PAGE_TRANSITION_LINK, false));
 }
 
 }  // namespace autofill