Fix crash where a RenderWidgetHost can be deleted in the middle of a

Fix crash where a RenderWidgetHost can be deleted in the middle of a
drag operation.

BUG=667188
Committed: https://crrev.com/b51498b854667a13515edd78b2a1120cce172f56
Cr-Commit-Position: refs/heads/master@{#435758}

[lfg, 2016-11-30 23:05:02]

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

[commit-bot: I haz the power, 2016-11-30 23:05:33]

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-11-30 23:53:49]

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

[commit-bot: I haz the power, 2016-11-30 23:53:50]

Dry run: This issue passed the CQ dry run.

[lfg, 2016-12-01 00:42:16]

lfg@chromium.org changed reviewers:
+ lukasza@google.com

[lfg, 2016-12-01 00:42:17]

Hey Lukasz, can you take a look? This should fix the crashes on SystemDragEnded.

I don't think this affects Mac because WebDestDragMac's lifetime is managed by
the system (its an NSObject), and the RenderWidgetHost is already de-referenced
using a weak ptr.

I also filed issue 670123 to write a test as a follow-up.

[dcheng, 2016-12-01 00:58:34]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-12-01 00:58:34]

Is part of the problem here that the WebContentsViewAura is destroyed, so the
member access is unsafe?

(Also, can this be reproed by a window that closes itself during the middle of
drag and drop? We've had similar bugs in the past)

[lfg, 2016-12-01 01:08:28]

On 2016/12/01 00:58:34, dcheng wrote:
> Is part of the problem here that the WebContentsViewAura is destroyed, so the
> member access is unsafe?
> 
> (Also, can this be reproed by a window that closes itself during the middle of
> drag and drop? We've had similar bugs in the past)

This is caused by the fact that RenderWidgetHost can be destroyed in the
middle of drag/drop, however, I've also added extra checks for the validity of
the WebContents, because WebContentsViewAura::EndDrag has them, so it likely
makes sense for the place that calls EndDrag() to also have it (I think those
are only needed for BrowserPlugin, according to the blame).

The issue with the window being closed is handled by the
if (!drag_source->window()) immediately above the changes I've made (and
that's why this can't be reproduced by doing window.close()).

[dcheng, 2016-12-01 01:10:42]

Right, but this was previously already stored in a WeakPtr. So if it wasn't a
problem with dereferencing a member on a deleted object, I would have expected
it to be sufficient to simply check that the weak pointer still points to a live
object.

[lfg, 2016-12-01 16:56:29]

On 2016/12/01 01:10:42, dcheng wrote:
> Right, but this was previously already stored in a WeakPtr. So if it wasn't a
> problem with dereferencing a member on a deleted object, I would have expected
> it to be sufficient to simply check that the weak pointer still points to a
live
> object.

What do you mean by "this was previously already stored in a WeakPtr"?

Before Paul's change, the RenderWidgetHost was dereferenced by using
WebContentsImpl::GetRenderViewHost()->GetWidget()So I think in that case we
would be sending the DragSourceSystemDragEnded to the wrong widget (for
example, in case the tab navigated cross-process).

[Łukasz Anforowicz, 2016-12-01 20:22:45]

lukasza@chromium.org changed reviewers:
+ lukasza@chromium.org

[Łukasz Anforowicz, 2016-12-01 20:22:46]

Thanks for looking into this crash.  The CL looks mostly good to me, but I
wonder if you could please look into adding an extra comment and moving the
SystemDragEnded into the EndDrag method as I am suggesting below.

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_view_aura.cc (left):

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_view_aura.cc:899: drag_start_rwh_ =
source_rwh->GetWeakPtr();
Could you please add a comment explaining why we need to get a weak pointer
here? (i.e. explaining that |source_rwh| might be destroyed during the call to
StartDragAndDrop (which won't return until the end of the drag-and-drop).

I also think it would be useful (but don't have a strong opinion about adding a
comment) to explain in what scenario |source_rwh| might be destroyed while this
method is running.  I remember you mentioned that closing the window is handled
elsewhere, so it might be worth it to describe the remaining scenario here.

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_view_aura.cc:944:
web_contents_->SystemDragEnded(source_rwh);
Oh, so this was the main problem:
- EndDrag was already working with WeakPtr, so it was safe;
- but the call to SystemDragEnded above uses an unprotected, raw pointer.

The above seems to match the callstack in https://crbug.com/667188.  For a
moment I was (similarily to Daniel) confused why we also need to change EndDrag
method above, but not everything makes sense :-)

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_view_aura.cc:900: auto
source_rwh_weak_ptr = source_rwh->GetWeakPtr();
nit?: I guess this is a matter of personal/subjective taste, but I would prefer
to spell out the type here.  I like the usage of auto when dealing with
iterators, but here (as a reader) I would appreciate not having to think at all
what the type of this variable is.

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_view_aura.cc:946:
web_contents_->SystemDragEnded(source_rwh_weak_ptr.get());
I wonder if the call to SystemDragEnded could/should be moved to the end of
EndDrag method.  Arguments for doing this:

- Keeping all of "end drag" related processing in one place
- EndDrag already checks for a null |web_contents_|.

[Łukasz Anforowicz, 2016-12-01 20:23:45]

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_view_aura.cc (left):

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_view_aura.cc:944:
web_contents_->SystemDragEnded(source_rwh);
On 2016/12/01 20:22:46, Łukasz Anforowicz wrote:
> Oh, so this was the main problem:
> - EndDrag was already working with WeakPtr, so it was safe;
> - but the call to SystemDragEnded above uses an unprotected, raw pointer.
> 
> The above seems to match the callstack in https://crbug.com/667188.  For a
> moment I was (similarily to Daniel) confused why we also need to change
EndDrag
> method above, but not everything makes sense :-)

s/not/now/ :-P

[lfg, 2016-12-01 20:56:33]

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_view_aura.cc (left):

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_view_aura.cc:899: drag_start_rwh_ =
source_rwh->GetWeakPtr();
On 2016/12/01 20:22:46, Łukasz Anforowicz wrote:
> Could you please add a comment explaining why we need to get a weak pointer
> here? (i.e. explaining that |source_rwh| might be destroyed during the call to
> StartDragAndDrop (which won't return until the end of the drag-and-drop).
> 
> I also think it would be useful (but don't have a strong opinion about adding
a
> comment) to explain in what scenario |source_rwh| might be destroyed while
this
> method is running.  I remember you mentioned that closing the window is
handled
> elsewhere, so it might be worth it to describe the remaining scenario here.

Done.

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_view_aura.cc:944:
web_contents_->SystemDragEnded(source_rwh);
On 2016/12/01 20:23:45, Łukasz Anforowicz wrote:
> On 2016/12/01 20:22:46, Łukasz Anforowicz wrote:
> > Oh, so this was the main problem:
> > - EndDrag was already working with WeakPtr, so it was safe;
> > - but the call to SystemDragEnded above uses an unprotected, raw pointer.
> > 
> > The above seems to match the callstack in https://crbug.com/667188.  For a
> > moment I was (similarily to Daniel) confused why we also need to change
> EndDrag
> > method above, but not everything makes sense :-)
> 
> s/not/now/ :-P

Right, the other reason I changed EndDrag is to get rid of the member variable,
as it is not really necessary -- we only need to keep track of the source_rwh
while the drag is happening, so a stack variable is sufficient.

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_view_aura.cc:900: auto
source_rwh_weak_ptr = source_rwh->GetWeakPtr();
On 2016/12/01 20:22:46, Łukasz Anforowicz wrote:
> nit?: I guess this is a matter of personal/subjective taste, but I would
prefer
> to spell out the type here.  I like the usage of auto when dealing with
> iterators, but here (as a reader) I would appreciate not having to think at
all
> what the type of this variable is.

I usually use auto when the type is clear (in this case, GetWeakPtr() should
return a WeakPtr to the type). But I don't feel strong about this, changed to
explicitly show the type.

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_view_aura.cc:946:
web_contents_->SystemDragEnded(source_rwh_weak_ptr.get());
On 2016/12/01 20:22:46, Łukasz Anforowicz wrote:
> I wonder if the call to SystemDragEnded could/should be moved to the end of
> EndDrag method.  Arguments for doing this:
> 
> - Keeping all of "end drag" related processing in one place
> - EndDrag already checks for a null |web_contents_|.

Done.

[lfg, 2016-12-01 20:56:36]

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

[lfg, 2016-12-01 20:56:45]

The CQ bit was unchecked by lfg@chromium.org

[lfg, 2016-12-01 20:57:07]

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

[commit-bot: I haz the power, 2016-12-01 20:57:33]

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

[Łukasz Anforowicz, 2016-12-01 21:14:28]

Thanks - LGTM.

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_view_aura.cc (left):

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_view_aura.cc:944:
web_contents_->SystemDragEnded(source_rwh);
On 2016/12/01 20:56:33, lfg wrote:
> On 2016/12/01 20:23:45, Łukasz Anforowicz wrote:
> > On 2016/12/01 20:22:46, Łukasz Anforowicz wrote:
> > > Oh, so this was the main problem:
> > > - EndDrag was already working with WeakPtr, so it was safe;
> > > - but the call to SystemDragEnded above uses an unprotected, raw pointer.
> > > 
> > > The above seems to match the callstack in https://crbug.com/667188.  For a
> > > moment I was (similarily to Daniel) confused why we also need to change
> > EndDrag
> > > method above, but not everything makes sense :-)
> > 
> > s/not/now/ :-P
> 
> Right, the other reason I changed EndDrag is to get rid of the member
variable,
> as it is not really necessary -- we only need to keep track of the source_rwh
> while the drag is happening, so a stack variable is sufficient.

Yes - that makes total sense.

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_view_aura.cc:900: auto
source_rwh_weak_ptr = source_rwh->GetWeakPtr();
On 2016/12/01 20:56:33, lfg wrote:
> On 2016/12/01 20:22:46, Łukasz Anforowicz wrote:
> > nit?: I guess this is a matter of personal/subjective taste, but I would
> prefer
> > to spell out the type here.  I like the usage of auto when dealing with
> > iterators, but here (as a reader) I would appreciate not having to think at
> all
> > what the type of this variable is.
> 
> I usually use auto when the type is clear (in this case, GetWeakPtr() should
> return a WeakPtr to the type). But I don't feel strong about this, changed to
> explicitly show the type.

Thanks.  The WeakPtr part was obvious, but it wasn't immediately obvious what is
the type being pointed to (even if I know that |rwh| might stand for
RenderWidgetHost, there is some ambiguity wrt RenderWidgetHost*Impl*).

[dcheng, 2016-12-01 21:16:31]

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_view_aura.cc (left):

https://codereview.chromium.org/2543803002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_view_aura.cc:944:
web_contents_->SystemDragEnded(source_rwh);
On 2016/12/01 20:56:33, lfg wrote:
> On 2016/12/01 20:23:45, Łukasz Anforowicz wrote:
> > On 2016/12/01 20:22:46, Łukasz Anforowicz wrote:
> > > Oh, so this was the main problem:
> > > - EndDrag was already working with WeakPtr, so it was safe;
> > > - but the call to SystemDragEnded above uses an unprotected, raw pointer.
> > > 
> > > The above seems to match the callstack in https://crbug.com/667188.  For a
> > > moment I was (similarily to Daniel) confused why we also need to change
> > EndDrag
> > > method above, but not everything makes sense :-)
> > 
> > s/not/now/ :-P
> 
> Right, the other reason I changed EndDrag is to get rid of the member
variable,
> as it is not really necessary -- we only need to keep track of the source_rwh
> while the drag is happening, so a stack variable is sufficient.

Thanks for clearing this up: it wasn't clear to me if this was just incidental
cleanup or part of a deeper fix.

https://codereview.chromium.org/2543803002/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/2543803002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:904: // For example, the
RenderWidgetHost can be deleted if a cross-proces transfer
Nit: proces => process

https://codereview.chromium.org/2543803002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:905: // happens while
dragging, since the RenderWidgetHost is deleted in that case.
Can it be more mundane than that? What if that subtree of frames is removed from
the DOM during drag processing?

[lfg, 2016-12-01 21:21:09]

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

[lfg, 2016-12-01 21:21:32]

https://codereview.chromium.org/2543803002/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/2543803002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:904: // For example, the
RenderWidgetHost can be deleted if a cross-proces transfer
On 2016/12/01 21:16:31, dcheng wrote:
> Nit: proces => process

Done.

https://codereview.chromium.org/2543803002/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_view_aura.cc:905: // happens while
dragging, since the RenderWidgetHost is deleted in that case.
On 2016/12/01 21:16:31, dcheng wrote:
> Can it be more mundane than that? What if that subtree of frames is removed
from
> the DOM during drag processing?

Yes, but this can only happen with OOPIFs. The only way I found for this to
happen without OOPIFs is a cross-process transfer on a main frame. (But I still
suspect that there must be easier ways).

[commit-bot: I haz the power, 2016-12-01 21:21:36]

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

[dcheng, 2016-12-01 21:22:42]

lgtm

+pwnall as FYI

[lfg, 2016-12-01 21:30:36]

lfg@chromium.org changed reviewers:
+ avi@chromium.org

[lfg, 2016-12-01 21:30:36]

+avi for owners review.

[commit-bot: I haz the power, 2016-12-01 22:06:31]

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

[commit-bot: I haz the power, 2016-12-01 22:06:32]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-12-01 23:23:58]

On 2016/12/01 21:30:36, lfg wrote:
> +avi for owners review.

Owner approval; LGTM

The title here is 💯.

[lfg, 2016-12-01 23:25:53]

The CQ bit was checked by lfg@chromium.org

[lfg, 2016-12-01 23:25:54]

The patchset sent to the CQ was uploaded after l-g-t-m from lukasza@chromium.org
Link to the patchset: https://codereview.chromium.org/2543803002/#ps40001
(title: "addressing comments")

[commit-bot: I haz the power, 2016-12-01 23:26:18]

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-12-01 23:33:20]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480634753851980,
"parent_rev": "e0456a2af00b9ab4e9156db21178fd8fad891b3c", "commit_rev":
"458c7d0562b43a807e2089380deefdf7f05a1273"}

[commit-bot: I haz the power, 2016-12-01 23:33:45]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-01 23:35:53]

Description was changed from

==========
Fix crash where a RenderWidgetHost can be deleted in the middle of a
drag operation.

BUG=667188
==========

to

==========
Fix crash where a RenderWidgetHost can be deleted in the middle of a
drag operation.

BUG=667188

Committed: https://crrev.com/b51498b854667a13515edd78b2a1120cce172f56
Cr-Commit-Position: refs/heads/master@{#435758}
==========

[commit-bot: I haz the power, 2016-12-01 23:35:54]

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

[Charlie Reis, 2016-12-01 23:41:17]

Description was changed from

==========
Fix crash where a RenderWidgetHost can be deleted in the middle of a
drag operation.

BUG=667188

Committed: https://crrev.com/b51498b854667a13515edd78b2a1120cce172f56
Cr-Commit-Position: refs/heads/master@{#435758}
==========

to

==========
Fix crash where a RenderWidgetHost can be deleted in the middle of a
drag operation.

BUG=667188

Committed: https://crrev.com/b51498b854667a13515edd78b2a1120cce172f56
Cr-Commit-Position: refs/heads/master@{#435758}
==========