ShowCreatedWindow: some speculative fixes for 680876.

content/browser/web_contents/web_contents_impl.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_impl.h"
 
 #include <stddef.h>
 
 #include <cmath>
 #include <utility>
@@ skipping 2098 matching lines @@
       WebContentsView* new_view = new_contents->view_.get();
 
       // TODO(brettw): It seems bogus that we have to call this function on the
       // newly created object and give it one of its own member variables.
       new_view->CreateViewForWidget(
           new_contents->GetRenderViewHost()->GetWidget(), false);
     }
     // 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);
   }
 
   if (delegate_) {
     delegate_->WebContentsCreated(
         this, render_process_id, params.opener_render_frame_id,
         params.frame_name, params.target_url, new_contents);
   }
@@ skipping 77 matching lines @@
   // to allow it to survive the trip without being hosted.
   base::mac::NSObjectRetain(widget_view->GetNativeView());
 #endif
 }
 
 void WebContentsImpl::ShowCreatedWindow(int process_id,
                                         int main_frame_widget_route_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())
       contents->ResumeLoadingCreatedWebContents();
 
     if (delegate) {
       delegate->AddNewContents(this, contents, disposition, initial_rect,
                                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()));
   }
 }
 
 void WebContentsImpl::ShowCreatedWidget(int process_id,
                                         int route_id,
                                         const gfx::Rect& initial_rect) {
   ShowCreatedWidget(process_id, route_id, false, initial_rect);
 }
 
@@ skipping 3178 matching lines @@
   GetMainFrame()->AddMessageToConsole(
       content::CONSOLE_MESSAGE_LEVEL_WARNING,
       base::StringPrintf("This site does not have a valid SSL "
                          "certificate! Without SSL, your site's and "
                          "visitors' data is vulnerable to theft and "
                          "tampering. Get a valid SSL certificate before"
                          " releasing your website to the public."));
 }
 
 }  // namespace content