Don't clear controller_ in ExtensionInstalledBubbleView::CloseBubble.

chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc

 // Copyright 2013 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 <algorithm>
 #include <string>
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/user_metrics_action.h"
@@ skipping 139 matching lines @@
       break;
   }
 
   // Default case.
   if (!reference_view || !reference_view->visible())
     reference_view = browser_view->toolbar()->app_menu_button();
   SetAnchorView(reference_view);
 }
 
 void ExtensionInstalledBubbleView::CloseBubble() {
   if (controller_ && controller_->anchor_position() ==

[Devlin, 2016/10/10 18:53:18]

if it is safe to remove the null-out, then we can remove this null-check too.

[Evan Stade, 2016/10/26 14:16:54]

Done.

       ExtensionInstalledBubble::ANCHOR_PAGE_ACTION) {
     BrowserView* browser_view =
         BrowserView::GetBrowserViewForBrowser(browser());
     browser_view->GetLocationBarView()->SetPreviewEnabledPageAction(
         extensions::ExtensionActionManager::Get(browser()->profile())
             ->GetPageAction(*controller_->extension()),
         false);  // preview_enabled
   }
-  controller_ = nullptr;

[Devlin, 2016/10/10 18:53:17]

My guess is that this was because the lifetime of these bubble objects is pretty
crazy (between the n delegates/controllers (and delegates that are controllers)
and the bubble manager et al), as well as to defend against multiple
CloseBubble() calls.  One important question: is GetWidget()->Close() guaranteed
to close the widget synchronously?  If not, then I could see a scenario in which
we call CloseBubble() from clicking the learn more link and then call Close()
from the bubble manager because the tab focus changes - and that could cause
breakages.

Looking at this, I also don't see where, if at all, the bubble would be removed
from the bubble manager... but maybe that's okay because the bubble ui goes
away?

[Evan Stade, 2016/10/10 21:30:18]

no
Seems like a reasonable concern, but this did not happen when I tested manually.
it seems it hangs around until TabStripModel::NotifyIfTabDeactivated() calls
BubbleManager::CloseAllMatchingBubbles().

The problem with refactoring this is that apparently no one has time to do it.
:( We should at least not crash.

[Devlin, 2016/10/10 21:41:21]

My guess is that users can trigger some pretty funny flows, so I'm not sure this
is enough to convince me.
Agreed - but it's not entirely clear to me that |controller_| is guaranteed to
be valid for the duration of this object (e.g. in the case of the bubble manager
closing the bubble, and the UI persisting due to an asynchronous close).  Maybe
the right thing to do is to check the controller for nullness, rather than not
null it.

You're probably more familiar with this code than I am, though, so if my
concerns (paranoia?) around lifetime aren't warranted, lemme know. :)

[Devlin, 2016/10/25 22:09:26]

If we're going to remove this, can we add a did_close_ flag here and either
CHECK(!did_close_) at the beginning of this method or
replace the if (controller_ && .... with if (!did_close_ && ...
?

If we're fairly sure CloseBubble() is only called once, then the CHECK would be
best.

[Evan Stade, 2016/10/26 14:16:53]

added a dcheck

   GetWidget()->Close();
 }
 
 base::string16 ExtensionInstalledBubbleView::GetWindowTitle() const {
   // Add the heading (for all options).
   base::string16 extension_name =
       base::UTF8ToUTF16(controller_->extension()->name());
   base::i18n::AdjustStringForLocaleDirection(&extension_name);
   return l10n_util::GetStringFUTF16(IDS_EXTENSION_INSTALLED_HEADING,
                                     extension_name);
@@ skipping 173 matching lines @@
             ->browser_actions();
     return !container->animating();
   }
   return true;
 }
 
 // Implemented here to create the platform specific instance of the BubbleUi.
 std::unique_ptr<BubbleUi> ExtensionInstalledBubble::BuildBubbleUi() {
   return base::WrapUnique(new ExtensionInstalledBubbleUi(this));
 }