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

content/browser/web_contents/web_contents_view_aura.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/web_contents/web_contents_view_aura.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include "base/auto_reset.h"
@@ skipping 532 matching lines @@
 
 void WebContentsViewAura::SizeChangedCommon(const gfx::Size& size) {
   if (web_contents_->GetInterstitialPage())
     web_contents_->GetInterstitialPage()->SetSize(size);
   RenderWidgetHostView* rwhv =
       web_contents_->GetRenderWidgetHostView();
   if (rwhv)
     rwhv->SetSize(size);
 }
 
-void WebContentsViewAura::EndDrag(blink::WebDragOperationsMask ops) {
+void WebContentsViewAura::EndDrag(RenderWidgetHost* source_rwh,
+                                  blink::WebDragOperationsMask ops) {
   if (!web_contents_)
     return;
 
   aura::Window* window = GetContentNativeView();
   gfx::Point screen_loc = display::Screen::GetScreen()->GetCursorScreenPoint();
   gfx::Point client_loc = screen_loc;
   aura::client::ScreenPositionClient* screen_position_client =
       aura::client::GetScreenPositionClient(window->GetRootWindow());
   if (screen_position_client)
     screen_position_client->ConvertPointFromScreen(window, &client_loc);
 
   // TODO(paulmeyer): In the OOPIF case, should |client_loc| be converted to
   // 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(
     RenderWidgetHostViewAura* view) {
   const std::string value = base::CommandLine::ForCurrentProcess()->
       GetSwitchValueASCII(switches::kOverscrollHistoryNavigation);
   if (value == "0") {
     navigation_overlay_.reset();
     return;
   }
@@ skipping 309 matching lines @@
     const gfx::ImageSkia& image,
     const gfx::Vector2d& image_offset,
     const DragEventSourceInfo& event_info,
     RenderWidgetHostImpl* source_rwh) {
   aura::Window* root_window = GetNativeView()->GetRootWindow();
   if (!aura::client::GetDragDropClient(root_window)) {
     web_contents_->SystemDragEnded(source_rwh);
     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)
     selection_controller->HideAndDisallowShowingAutomatically();
   std::unique_ptr<ui::OSExchangeData::Provider> provider =
       ui::OSExchangeDataProviderFactory::CreateProvider();
   PrepareDragData(drop_data, provider.get(), web_contents_);
 
   ui::OSExchangeData data(
       std::move(provider));  // takes ownership of |provider|.
@@ skipping 23 matching lines @@
   // Bail out immediately if the contents view window is gone. Note that it is
   // not safe to access any class members in this case since |this| may already
   // be destroyed. The local variable |drag_source| will still be valid though,
   // so we can use it to determine if the window is gone.
   if (!drag_source->window()) {
     // Note that in this case, we don't need to call SystemDragEnded() since the
     // renderer is going away.
     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) {
   current_drag_op_ = operation;
 }
 
 void WebContentsViewAura::GotFocus() {
   web_contents_->NotifyWebContentsFocused();
 }
 
@@ skipping 292 matching lines @@
                                         bool allow_multiple_selection) {
   NOTIMPLEMENTED() << " show " << items.size() << " menu items";
 }
 
 void WebContentsViewAura::HidePopupMenu() {
   NOTIMPLEMENTED();
 }
 #endif
 
 }  // namespace content