ShowCreatedWindow: some speculative fixes for 680876.

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 7c02ea952f1947a89bcb3bb6ccf7665ba9b45549..7461c85e52013b06ccf25af5dd7281e0ee079e34 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -2116,6 +2116,8 @@ void WebContentsImpl::CreateNewWindow(
     // Save the created window associated with the route so we can show it
     // later.
     DCHECK_NE(MSG_ROUTING_NONE, main_frame_widget_route_id);
+    CHECK(RenderWidgetHostImpl::FromID(render_process_id,
+                                       main_frame_widget_route_id));
     pending_contents_[std::make_pair(
         render_process_id, main_frame_widget_route_id)] = new_contents;
     AddDestructionObserver(new_contents);
@@ -2213,9 +2215,13 @@ void WebContentsImpl::ShowCreatedWindow(int process_id,
                                         WindowOpenDisposition disposition,
                                         const gfx::Rect& initial_rect,
                                         bool user_gesture) {
+  RenderWidgetHostImpl* rwh =
+      RenderWidgetHostImpl::FromID(process_id, main_frame_widget_route_id);
+  if (!rwh)
+    return;

[alexmos, 2017/01/18 23:23:47]

Would it help at all to add a DumpWithoutCrashing to see if we in fact are ever
getting here with a null |rwh|?

[ncarter (slow), 2017/01/19 22:04:49]

Thanks for this suggestion. I've added a DumpWithoutCrashing here, but only in
the case where contents != nullptr, since I'm hoping we don't crash otherwise.

   WebContentsImpl* contents =
       GetCreatedWindow(process_id, main_frame_widget_route_id);
-  if (contents) {
+  if (contents && contents->GetMainFrame()->GetRenderWidgetHost() == rwh) {

[alexmos, 2017/01/18 23:23:47]

How do you think we could get to the state where these are not equal?  The main
frame swapping to a new RFH/RVH before we get the show IPC?  I thought that
shouldn't be possible since the renderer calls show()  synchronously after
createWindow()?

[ncarter (slow), 2017/01/18 23:59:16]

Really, I'm grasping at straws here: I don't really expect this case to trigger.
But the idea is that the delegate did something screwy with the pending
contents; it gets a handle to the WebContents via the WebContentsCreated
delegate call, and it goes e.g. here:
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
.

A delegate-specific behavior is my best explanation for this being
android-specific. The crash really isn't happening on other platforms.

I don't yet see any smoking guns, but it's at least possible there's something
in Android-land that would be queuing up an operation here that would
immediately commit a navigation?

[alexmos, 2017/01/19 01:25:20]

Agreed - an Android delegate somehow triggering a commit upon observing WC
creation is a reasonable theory.  Question is - wouldn't we still want to show
it in that case?  Presumably, it committed something useful in that new WC?

[ncarter (slow), 2017/01/19 22:04:49]

I undid the equality check since I agree with you about theoretical risk, and I
didn't have a strong explanation for it.

     WebContentsDelegate* delegate = GetDelegate();
     contents->is_resume_pending_ = true;
     if (!delegate || delegate->ShouldResumeRequestsForCreatedWindow())
@@ -2226,8 +2232,6 @@ void WebContentsImpl::ShowCreatedWindow(int process_id,
                                user_gesture, NULL);
     }
 
-    RenderWidgetHostImpl* rwh = contents->GetMainFrame()->GetRenderWidgetHost();
-    DCHECK_EQ(main_frame_widget_route_id, rwh->GetRoutingID());
     rwh->Send(new ViewMsg_Move_ACK(rwh->GetRoutingID()));
   }
 }