Don't re-use a zoom bubble unless it has the same web-contents.

Reset singleton ZoomBubbleView::zoom_bubble_ in ::Close()

The current implementation of ZoomBubbleView is capable of attempting to re-use a zoom bubble with a stale WebContents*.

This CL resets ZoomBubbleView::zoom_bubble_ in the ZoomBubbleView::Close() method to avoid inadvertent reuse. It also adds a DCHECK to make sure WebContents* are never mis-matched in calls to ZoomBubbleView::ShowBubble().

BUG=428561
Committed: https://crrev.com/d1b6f4d89d002808689a156b739f8d7fd80104fb
Cr-Commit-Position: refs/heads/master@{#303945}

[wjmaclean, 2014-11-10 23:57:22]

wjmaclean@chromium.org changed reviewers:
+ sky@chromium.org

[wjmaclean, 2014-11-10 23:57:23]

sky@ - Regardless of whether the Clusterfuzz bug repros or not, I think this is
a sensible change to make ... what do you think?

The bug stacktrace sees the call to ZoomBubbleView::Refresh() [line 170] crash
when the (nominally) same call on ZoomBubbleView::ShowBubble() [line 63])
doesn't crash ... the only thing that has changed is a WebContent*, which is the
thing ASAN says is being used after use in Refresh() ... sounds stale to me.

[sky, 2014-11-11 00:50:52]

Seems that all the call sites use the active web contents. How could it be that
refresh wasn't called? Seems to me this would be working around things.

I can't see the bug, stupid persmissions.

[wjmaclean, 2014-11-11 13:23:31]

On 2014/11/11 00:50:52, sky wrote:
> Seems that all the call sites use the active web contents. How could it be
that
> refresh wasn't called? Seems to me this would be working around things.
> 
> I can't see the bug, stupid persmissions.

The call sites don't use the same web contents. Note that ShowBubble() is a
static method, and it's operating on zoom_bubble_ which (1) is stored in a
global var, (2) stores an un-owned copy of WebContents* web_contents. However,
the caller to ShowBubble also passes a WebContents* that is not guaranteed to be
the same one. Also, if the WebContents owned by the ZoomBubbleView is destroyed,
the ZoomBubbleView is not notified.

A possibly more principled way to deal with this would be to make ZoomBubbleView
a WebContentsObserver so it can detect the destruction of the WebContents and
delete itself when that occurs ... would that be preferable?

[wjmaclean, 2014-11-11 13:24:11]

On 2014/11/11 00:50:52, sky wrote:
> 
> I can't see the bug, stupid persmissions.

inferno@ - can we give sky@ visibility on the bug?

[sky, 2014-11-11 17:16:36]

Mark gave me access to the bug. I think the real issue here is
zoom_bubble_ is set to null in ZoomBubbleView::WindowClosing().
WindowClosing is async. I think zoom_bubble_ should be set to null
CloseBubble is called.

On Tue, Nov 11, 2014 at 5:23 AM,  <wjmaclean@chromium.org> wrote:
> On 2014/11/11 00:50:52, sky wrote:
>>
>> Seems that all the call sites use the active web contents. How could it be
>
> that
>>
>> refresh wasn't called? Seems to me this would be working around things.
>
>
>> I can't see the bug, stupid persmissions.
>
>
> The call sites don't use the same web contents. Note that ShowBubble() is a
> static method, and it's operating on zoom_bubble_ which (1) is stored in a
> global var, (2) stores an un-owned copy of WebContents* web_contents.
> However,
> the caller to ShowBubble also passes a WebContents* that is not guaranteed
> to be
> the same one. Also, if the WebContents owned by the ZoomBubbleView is
> destroyed,
> the ZoomBubbleView is not notified.
>
> A possibly more principled way to deal with this would be to make
> ZoomBubbleView
> a WebContentsObserver so it can detect the destruction of the WebContents
> and
> delete itself when that occurs ... would that be preferable?
>
> https://codereview.chromium.org/712993004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[wjmaclean, 2014-11-11 20:53:43]

On 2014/11/11 17:16:36, sky wrote:
> Mark gave me access to the bug. I think the real issue here is
> zoom_bubble_ is set to null in ZoomBubbleView::WindowClosing().
> WindowClosing is async. I think zoom_bubble_ should be set to null
> CloseBubble is called.

Hmm, I don't understand. The crash report in the bug is for an ASAN
use-after-free, not a null-pointer dereference. The report also identifies a
WebContents* as the re-used element, not a ZoomBubbleView. That suggests to me
that a stale WebContents* is the issue, but perhaps I'm missing something?

[wjmaclean, 2014-11-11 21:04:06]

On 2014/11/11 17:16:36, sky wrote:
> Mark gave me access to the bug. I think the real issue here is
> zoom_bubble_ is set to null in ZoomBubbleView::WindowClosing().
> WindowClosing is async. I think zoom_bubble_ should be set to null
> CloseBubble is called.

Btw, I'm not opposed to the change you suggest, I'm just not sure it's the
(only) problem here.

[sky, 2014-11-12 00:24:18]

It's kind of two fold. When the WebContents changes (including being
destroyed) LocationBarView closes the Bubble. Closing the bubble is
async. Only when the async operation completes do we reset
zoom_bubble_. This means if you do:
Close();
ZoomBubbleView::ShowBubble
We'll use zoom_bubble_, even though it's being closed. This is why I
say the minute we invoke Close() zoom_bubble_ should be reset. If it
had done that you would never have seen this issue. That said, I do
agree with you that it's bogus that ZoomBubbleView still references a
WebContents that may have been destroyed. I think Close() should be
set zoom_bubble_ to null and web_contents_.

On Tue, Nov 11, 2014 at 12:53 PM,  <wjmaclean@chromium.org> wrote:
> On 2014/11/11 17:16:36, sky wrote:
>>
>> Mark gave me access to the bug. I think the real issue here is
>> zoom_bubble_ is set to null in ZoomBubbleView::WindowClosing().
>> WindowClosing is async. I think zoom_bubble_ should be set to null
>> CloseBubble is called.
>
>
> Hmm, I don't understand. The crash report in the bug is for an ASAN
> use-after-free, not a null-pointer dereference. The report also identifies a
> WebContents* as the re-used element, not a ZoomBubbleView. That suggests to
> me
> that a stale WebContents* is the issue, but perhaps I'm missing something?
>
> https://codereview.chromium.org/712993004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[wjmaclean, 2014-11-12 20:00:51]

sky@ - I'm running this revised patch on the trybots, let me know if  it looks
like what you had in mind.

[sky, 2014-11-12 21:48:55]

https://codereview.chromium.org/712993004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (left):

https://codereview.chromium.org/712993004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:353: void
ZoomBubbleView::WindowClosing() {
You should still keep this. The reason you want this too is that you can end up
here if the OS triggers the close.

https://codereview.chromium.org/712993004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/712993004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:175: // Prevent
inadvertent re-use of this bubble.
This isn't entirely clear. Maybe something like Close() is async, but we don't
to use zoom_bubble_ after this. Additionally web_contents_ may have been
destroyed.

[wjmaclean, 2014-11-12 22:00:01]

PTAL?

https://codereview.chromium.org/712993004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (left):

https://codereview.chromium.org/712993004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:353: void
ZoomBubbleView::WindowClosing() {
On 2014/11/12 21:48:55, sky wrote:
> You should still keep this. The reason you want this too is that you can end
up
> here if the OS triggers the close.

Sounds fair ... didn't realize that would happen.

Restored.

https://codereview.chromium.org/712993004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/712993004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:175: // Prevent
inadvertent re-use of this bubble.
On 2014/11/12 21:48:55, sky wrote:
> This isn't entirely clear. Maybe something like Close() is async, but we don't
> to use zoom_bubble_ after this. Additionally web_contents_ may have been
> destroyed.

Done.

I assume you meant "Widget's Close() is async ..."

[sky, 2014-11-12 22:02:18]

LGTM

[wjmaclean, 2014-11-13 00:07:04]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-11-13 00:08:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/712993004/60001

[commit-bot: I haz the power, 2014-11-13 00:56:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-13 00:56:47]

Patchset 4 (id:??) landed as
https://crrev.com/d1b6f4d89d002808689a156b739f8d7fd80104fb
Cr-Commit-Position: refs/heads/master@{#303945}