Don't clear controller_ in ExtensionInstalledBubbleView::CloseBubble.

Don't clear controller_ in ExtensionInstalledBubbleView::CloseBubble.

BUG=648280
Committed: https://crrev.com/2b385cbfe8340e4652b4a7b5210a3e0de01d89c2
Cr-Commit-Position: refs/heads/master@{#428037}

[Evan Stade, 2016-10-10 18:33:38]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-10 18:34:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2016-10-10 18:35:22]

estade@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Evan Stade, 2016-10-10 18:35:22]

I added this in https://codereview.chromium.org/1807793003 but now I can't
remember why it was important to set the controller to null. I tested this by
installing an extension on a fresh profile and clicking the sign in promo link,
and CloseBubble was only called once and I didn't observer any crashes or other
issues.

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

https://codereview.chromium.org/2409713002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(left):

https://codereview.chromium.org/2409713002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:160: if
(controller_ && controller_->anchor_position() ==
if it is safe to remove the null-out, then we can remove this null-check too.

https://codereview.chromium.org/2409713002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:169:
controller_ = nullptr;
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?

[commit-bot: I haz the power, 2016-10-10 19:18:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-10 19:18:04]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2409713002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(left):

https://codereview.chromium.org/2409713002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:169:
controller_ = nullptr;
On 2016/10/10 18:53:17, Devlin wrote:
> 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?  

no

> 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.

Seems like a reasonable concern, but this did not happen when I tested manually.

> 
> 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?

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]

https://codereview.chromium.org/2409713002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(left):

https://codereview.chromium.org/2409713002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:169:
controller_ = nullptr;
On 2016/10/10 21:30:18, Evan Stade wrote:
> On 2016/10/10 18:53:17, Devlin wrote:
> > 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?  
> 
> no
> 
> > 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.
> 
> Seems like a reasonable concern, but this did not happen when I tested
manually.

My guess is that users can trigger some pretty funny flows, so I'm not sure this
is enough to convince me.

> 
> > 
> > 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?
> 
> 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.

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. :)

[Evan Stade, 2016-10-24 13:08:18]

On 2016/10/10 21:41:21, Devlin wrote:
>
https://codereview.chromium.org/2409713002/diff/1/chrome/browser/ui/views/ext...
> File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
> (left):
> 
>
https://codereview.chromium.org/2409713002/diff/1/chrome/browser/ui/views/ext...
> chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:169:
> controller_ = nullptr;
> On 2016/10/10 21:30:18, Evan Stade wrote:
> > On 2016/10/10 18:53:17, Devlin wrote:
> > > 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?  
> > 
> > no
> > 
> > > 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.
> > 
> > Seems like a reasonable concern, but this did not happen when I tested
> manually.
> 
> My guess is that users can trigger some pretty funny flows, so I'm not sure
this
> is enough to convince me.
> 
> > 
> > > 
> > > 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?
> > 
> > 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.
> 
> 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. :)

sorry I've not come back to this recently, been ooo a bit lately. I don't think
you're being paranoid, but I don't like writing code that's a result of not
really knowing what might happen. I feel better asserting that something should
not happen (even if I'm wrong and later have to fix it) because otherwise the
code becomes a morass for the next person who comes along and tries to
understand it.

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

> sorry I've not come back to this recently, been ooo a bit lately. I don't
think
> you're being paranoid, but I don't like writing code that's a result of not
> really knowing what might happen. I feel better asserting that something
should
> not happen (even if I'm wrong and later have to fix it) because otherwise the
> code becomes a morass for the next person who comes along and tries to
> understand it.

Completely understand - just trying to make sure we don't add any new issues. :)

Added a concrete suggestion for an easy mitigation/assertion.

https://codereview.chromium.org/2409713002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(left):

https://codereview.chromium.org/2409713002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:169:
controller_ = nullptr;
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:34]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 14:16:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2409713002/diff/1/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(left):

https://codereview.chromium.org/2409713002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:160: if
(controller_ && controller_->anchor_position() ==
On 2016/10/10 18:53:18, Devlin wrote:
> if it is safe to remove the null-out, then we can remove this null-check too.

Done.

https://codereview.chromium.org/2409713002/diff/1/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:169:
controller_ = nullptr;
On 2016/10/25 22:09:26, Devlin wrote:
> 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.

added a dcheck

[Devlin, 2016-10-26 14:34:58]

lgtm w/ nit.  Thanks for bearing with me.

https://codereview.chromium.org/2409713002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(right):

https://codereview.chromium.org/2409713002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:161:
DCHECK(!GetWidget()->IsClosed());
nit: I'd be more comfortable with this being a release CHECK().  The previous
nulling out of controller and check served to ensure that the below code was
only called once.  Given the sparseness of testing in this area, I don't think a
DCHECK is sufficient to assure us that this method is, in fact, only called
once.

[commit-bot: I haz the power, 2016-10-26 15:01:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 15:01:12]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-10-26 17:24:22]

estade@chromium.org changed reviewers:
+ msw@chromium.org

[Evan Stade, 2016-10-26 17:24:22]

+msw for owners

https://codereview.chromium.org/2409713002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc
(right):

https://codereview.chromium.org/2409713002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_installed_bubble_view.cc:161:
DCHECK(!GetWidget()->IsClosed());
On 2016/10/26 14:34:58, Devlin wrote:
> nit: I'd be more comfortable with this being a release CHECK().  The previous
> nulling out of controller and check served to ensure that the below code was
> only called once.  Given the sparseness of testing in this area, I don't think
a
> DCHECK is sufficient to assure us that this method is, in fact, only called
> once.

Done.

[Evan Stade, 2016-10-26 17:24:27]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 17:24:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 19:25:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 19:25:47]

Dry run: This issue passed the CQ dry run.

[msw, 2016-10-26 19:34:41]

lgtm, but I wouldn't be surprised if this does bite you later.

[Evan Stade, 2016-10-27 15:57:15]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-10-27 15:57:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2409713002/#ps40001
(title: "check")

[commit-bot: I haz the power, 2016-10-27 15:57:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 16:03:00]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-27 16:29:32]

Description was changed from

==========
Don't clear controller_ in ExtensionInstalledBubbleView::CloseBubble.

BUG=648280
==========

to

==========
Don't clear controller_ in ExtensionInstalledBubbleView::CloseBubble.

BUG=648280

Committed: https://crrev.com/2b385cbfe8340e4652b4a7b5210a3e0de01d89c2
Cr-Commit-Position: refs/heads/master@{#428037}
==========

[commit-bot: I haz the power, 2016-10-27 16:29:33]

Patchset 3 (id:??) landed as
https://crrev.com/2b385cbfe8340e4652b4a7b5210a3e0de01d89c2
Cr-Commit-Position: refs/heads/master@{#428037}