Fix crash when destroying a RenderWidgetHost that holds the pointer

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 1665 matching lines @@
       delegate_->ExitFullscreenModeForTab(this);
     FOR_EACH_OBSERVER(WebContentsObserver,
                       observers_,
                       DidDestroyFullscreenWidget());
     fullscreen_widget_process_id_ = ChildProcessHost::kInvalidUniqueID;
     fullscreen_widget_routing_id_ = MSG_ROUTING_NONE;
     if (fullscreen_widget_had_focus_at_shutdown_)
       view_->RestoreFocus();
   }
 
-  if (mouse_lock_widget_ == render_widget_host)
-    mouse_lock_widget_ = nullptr;
+  DCHECK(mouse_lock_widget_ != render_widget_host);

[Charlie Reis, 2016/07/06 17:41:47]

DCHECKs aren't compiled into release builds, so this makes me nervous about
having a dangling pointer here.  Is there something new that guarantees this
case won't happen?  Was it happening before?

[lfg, 2016/07/07 20:17:35]

Let's change it to a CHECK then, so we can force a crash if that ever happens,
then we can let it bake for a while in Canary.

This was the cause of the bug -- cleaning up the mouse_lock_widget_ while it
held the lock (when it tried to unlock, it would crash).

[Charlie Reis, 2016/07/07 21:47:12]

Acknowledged.

 }
 
 void WebContentsImpl::RenderWidgetGotFocus(
     RenderWidgetHostImpl* render_widget_host) {
   // Notify the observers if an embedded fullscreen widget was focused.
   if (delegate_ && render_widget_host && delegate_->EmbedsFullscreenWidget() &&
       render_widget_host->GetView() == GetFullscreenRenderWidgetHostView()) {
     NotifyWebContentsFocused();
   }
 }
@@ skipping 200 matching lines @@
 
 void WebContentsImpl::LostMouseLock(RenderWidgetHostImpl* render_widget_host) {
   CHECK(mouse_lock_widget_);
   mouse_lock_widget_->SendMouseLockLost();
   mouse_lock_widget_ = nullptr;
 
   if (delegate_)
     delegate_->LostMouseLock();
 }
 
+bool WebContentsImpl::HasMouseLock(RenderWidgetHostImpl* render_widget_host) {
+  return mouse_lock_widget_ == render_widget_host &&
+         GetTopLevelRenderWidgetHostView()->IsMouseLocked();

[Charlie Reis, 2016/07/06 17:41:47]

This is confusing to me at first glance.  Why is it asking the top-level RWHV if
it's locked, when RWH might not be top-level?

Can you add a comment if this is really how things work?

[lfg, 2016/07/07 20:17:35]

There's two parts to this. First the mouse_lock_widget_ in WebContents only
keeps track of which widget requested the lock (In RequestToLockMouse, it's set
before the delegate approves the request).

The top-level RWHV is the platform RWHV, which actually asks the OS to lock the
mouse. This will return if the mouse actually got locked.

I'll add comments.

[Charlie Reis, 2016/07/07 21:47:12]

Acknowledged.

+}
+
 void WebContentsImpl::ForwardCompositorProto(
     RenderWidgetHostImpl* render_widget_host,
     const std::vector<uint8_t>& proto) {
   if (delegate_)
     delegate_->ForwardCompositorProto(render_widget_host, proto);
 }
 
 void WebContentsImpl::OnRenderFrameProxyVisibilityChanged(bool visible) {
   if (visible)
     WasShown();
@@ skipping 1037 matching lines @@
 }
 
 gfx::Size WebContentsImpl::GetPreferredSize() const {
   return capturer_count_ == 0 ? preferred_size_ : preferred_size_for_capture_;
 }
 
 bool WebContentsImpl::GotResponseToLockMouseRequest(bool allowed) {
   if (GetBrowserPluginGuest())
     return GetBrowserPluginGuest()->LockMouse(allowed);
 
-  if (mouse_lock_widget_)
-    return mouse_lock_widget_->GotResponseToLockMouseRequest(allowed);
+  if (mouse_lock_widget_ &&
+      mouse_lock_widget_->GotResponseToLockMouseRequest(allowed))
+    return true;
+
+  mouse_lock_widget_ = nullptr;
   return false;
 }
 
 bool WebContentsImpl::HasOpener() const {
   return GetOpener() != NULL;
 }
 
 WebContentsImpl* WebContentsImpl::GetOpener() const {
   FrameTreeNode* opener_ftn = frame_tree_.root()->opener();
   return opener_ftn ? FromFrameTreeNode(opener_ftn) : nullptr;
@@ skipping 2148 matching lines @@
   for (RenderViewHost* render_view_host : render_view_host_set)
     render_view_host->OnWebkitPreferencesChanged();
 }
 
 void WebContentsImpl::SetJavaScriptDialogManagerForTesting(
     JavaScriptDialogManager* dialog_manager) {
   dialog_manager_ = dialog_manager;
 }
 
 }  // namespace content