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

content/browser/web_contents/web_contents_view_aura.cc

Index: content/browser/web_contents/web_contents_view_aura.cc
diff --git a/content/browser/web_contents/web_contents_view_aura.cc b/content/browser/web_contents/web_contents_view_aura.cc
index 091cb0e4712f47a583e7b9c0df27ca3e7a58d3c0..9155ae2d1f8b60bc536080b82bbd416b3a6cb1a9 100644
--- a/content/browser/web_contents/web_contents_view_aura.cc
+++ b/content/browser/web_contents/web_contents_view_aura.cc
@@ -550,7 +550,8 @@ void WebContentsViewAura::SizeChangedCommon(const gfx::Size& size) {
     rwhv->SetSize(size);
 }
 
-void WebContentsViewAura::EndDrag(blink::WebDragOperationsMask ops) {
+void WebContentsViewAura::EndDrag(RenderWidgetHost* source_rwh,
+                                  blink::WebDragOperationsMask ops) {
   if (!web_contents_)
     return;
 
@@ -566,7 +567,7 @@ void WebContentsViewAura::EndDrag(blink::WebDragOperationsMask ops) {
   // the coordinates local to |drag_start_rwh_|? See crbug.com/647249.
   web_contents_->DragSourceEndedAt(client_loc.x(), client_loc.y(),
                                    screen_loc.x(), screen_loc.y(), ops,
-                                   drag_start_rwh_.get());
+                                   source_rwh);
 }
 
 void WebContentsViewAura::InstallOverscrollControllerDelegate(
@@ -896,7 +897,7 @@ void WebContentsViewAura::StartDragging(
     return;
   }
 
-  drag_start_rwh_ = source_rwh->GetWeakPtr();

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

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.

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

Done.

+  auto source_rwh_weak_ptr = source_rwh->GetWeakPtr();

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

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.

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

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.

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

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

 
   ui::TouchSelectionController* selection_controller = GetSelectionController();
   if (selection_controller)
@@ -940,8 +941,9 @@ void WebContentsViewAura::StartDragging(
     return;
   }
 
-  EndDrag(ConvertToWeb(result_op));
-  web_contents_->SystemDragEnded(source_rwh);

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

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

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

s/not/now/ :-P

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

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.

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

Yes - that makes total sense.

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

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

+  EndDrag(source_rwh_weak_ptr.get(), ConvertToWeb(result_op));
+  if (web_contents_)
+    web_contents_->SystemDragEnded(source_rwh_weak_ptr.get());

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

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

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

Done.

 }
 
 void WebContentsViewAura::UpdateDragCursor(blink::WebDragOperation operation) {