Don't clear controller_ in ExtensionInstalledBubbleView::CloseBubble.

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

Index: chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
diff --git a/chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc b/chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
index 85331a2314e33042bda8588e73a22e6ae9e7ca33..fd76bd438755f9b5417f5169d2a78b0c0af2cfe2 100644
--- a/chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
+++ b/chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
@@ -166,7 +166,6 @@ void ExtensionInstalledBubbleView::CloseBubble() {
             ->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();
 }