Fix crash when destroying a RenderWidgetHost that holds the pointer

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 2978fb9e1c1e386f7eec5be92e6fee02b2192435..aebfc621b04a5717759089417a4d33e4f38161fa 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -1683,8 +1683,7 @@ void WebContentsImpl::RenderWidgetDeleted(
       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(
@@ -1905,6 +1904,11 @@ void WebContentsImpl::LostMouseLock(RenderWidgetHostImpl* render_widget_host) {
     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) {
@@ -2962,8 +2966,11 @@ 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;
 }