Prevented connecting drag drop events to a SwappedOut RenderViewHost in WebContentsViewGtk

content/browser/web_contents/web_contents_view_gtk.cc

Index: content/browser/web_contents/web_contents_view_gtk.cc
diff --git a/content/browser/web_contents/web_contents_view_gtk.cc b/content/browser/web_contents/web_contents_view_gtk.cc
index 7c6683530f4a60386332ae7a92a8d66f343a9ac6..a89a32faee7fb4be0cae441b984dde4de3ba37fc 100644
--- a/content/browser/web_contents/web_contents_view_gtk.cc
+++ b/content/browser/web_contents/web_contents_view_gtk.cc
@@ -223,11 +223,20 @@ RenderWidgetHostView* WebContentsViewGtk::CreateViewForWidget(
                         GDK_POINTER_MOTION_MASK);
   InsertIntoContentArea(content_view);
 
-  // Renderer target DnD.
-  drag_dest_.reset(new WebDragDestGtk(web_contents_, content_view));
-
-  if (delegate_.get())
-    drag_dest_->set_delegate(delegate_->GetDragDestDelegate());
+  if (render_widget_host->IsRenderView()) {
+    RenderViewHost* rvh = RenderViewHost::From(render_widget_host);
+    // According to WebContentsImpl::RenderViewCreated, if the RVH is swapped
+    // out on creation then we are just creating a swapped-out RVH for the
+    // opener chain that won't be used for WebUI, so it won't be expected to
+    // receive any events.

[Charlie Reis, 2013/02/27 01:42:35]

This comment doesn't make sense to me.  First, creating swapped out RVHs for the
opener chain is only one of multiple ways we could get here.  (The BrowserPlugin
also does this, which was the cause of the bug.)  Also, WebUI is unrelated to
whether it will receive events.  The only reason WebUI was mentioned in
RenderViewCreated is that there was WebUI-specific logic below the return
statement.

In other words, most of this is a red herring for why we want this logic.

The real reason is that we don't want to change any of this class's state (e.g.,
drag_dest_) for RenderViewHosts that are currently swapped out, and thus not
visible.  It will be important to update this state when we swap in a new
RenderViewHost, which happens below in RenderViewSwappedIn.

Can you put something like that just above the IsRenderView() conditional?  I
want it to be clear to future developers that any local state should not be
updated for swapped out RVHs, and drag_dest_ is just one example.

[mthiesse, 2013/02/27 14:57:13]

Done.

+    if (!static_cast<RenderViewHostImpl*>(rvh)->is_swapped_out()) {
+      // Renderer target DnD.
+      drag_dest_.reset(new WebDragDestGtk(web_contents_, content_view));
+
+      if (delegate_.get())
+        drag_dest_->set_delegate(delegate_->GetDragDestDelegate());
+    }
+  }
 
   return view;
 }
@@ -262,7 +271,20 @@ void WebContentsViewGtk::SizeContents(const gfx::Size& size) {
 void WebContentsViewGtk::RenderViewCreated(RenderViewHost* host) {
 }
 
-void WebContentsViewGtk::RenderViewSwappedIn(RenderViewHost* host) {
+void WebContentsViewGtk::RenderViewSwappedIn(RenderViewHost* new_host,
+                                             RenderViewHost* old_host) {

[Charlie Reis, 2013/02/27 01:42:35]

Shouldn't we update drag_dest_ every time we get here, regardless of whether
there's a current one or what the old host had?  If we've just changed our
RenderViewHost, then it seems like we necessarily need to change our drag_dest_.

If that's the case, then we don't need old_host in the API.

[mthiesse, 2013/02/27 14:57:13]

No, if we update the drag_dest_ every time here, just simple navigation breaks
drag and drop. Maybe you have a better idea of why this is the case than I
would?

[Charlie Reis, 2013/02/27 20:01:44]

This sounds like an issue with drag n drop, which I don't have any familiarity
with.  Can you find someone who knows that feature who might be able to figure
out what failure you're seeing?

I'm pretty sure we should have to update this every time the RenderViewHost
changes, assuming that GetNativeView() is different for each RenderViewHost. 
Otherwise drag_dest_ will have a stale native view, right?

+  // If we don't have a drag_dest yet, then we should set the drag test to the
+  // new RVH.
+  // Also, not sure if this ever happens, but if the renderViewHost we're
+  // swapping out is used by the drag_dest, then we need to reset the drag_dest
+  // to the new host.
+  if (!drag_dest_.get() || (old_host &&
+      (old_host->GetView()->GetNativeView() == drag_dest_->widget()))) {
+    drag_dest_.reset(new WebDragDestGtk(web_contents_,
+                                        new_host->GetView()->GetNativeView()));
+    if (delegate_.get())
+      drag_dest_->set_delegate(delegate_->GetDragDestDelegate());
+  }
 }
 
 WebContents* WebContentsViewGtk::web_contents() {