Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2112923002:
Fix crash when destroying a RenderWidgetHost that holds the pointer (Closed)
|
Created:
4 years, 5 months ago by lfg Modified:
4 years, 5 months ago CC:
chromium-reviews, creis+watch_chromium.org, yusukes+watch_chromium.org, shuchen+watch_chromium.org, nasko+codewatch_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix crash when destroying a RenderWidgetHost that holds the
pointer lock.
This CL fixes two separate issues. One is that when a
CrossProcessFrameConnector is being destroyed, if the
RenderWidgetHostViewChildFrame holds the mouse lock it must
unlock the mouse.
The second issue is that when flash holds a fullscreen
pointer lock and releases it, the WebContents is notified
of the lost mouse lock, even though it was never notified
that the mouse lock was acquired (because flash has extra
privileges to acquire the lock).
BUG=619571
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/ad824435a11fa19a1707c5e4af5dc405fe4fe708
Cr-Commit-Position: refs/heads/master@{#405195}
Patch Set 1 #
Total comments: 11
Patch Set 2 : addressing comments #Patch Set 3 : fixing flash fullscreen #Patch Set 4 : send privileged request through webcontents #
Total comments: 6
Patch Set 5 : rebase #Patch Set 6 : addressing comments #Patch Set 7 : rebase #Messages
Total messages: 44 (24 generated)
Description was changed from ========== Fix crash when destroying a RenderWidgetHost that holds the pointer lock. BUG=619571 ========== to ========== Fix crash when destroying a RenderWidgetHost that holds the pointer lock. BUG=619571 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
lfg@chromium.org changed reviewers: + nasko@chromium.org
Nasko, please take a look.
lfg@chromium.org changed reviewers: + creis@chromium.org
+Charlie, can you review since nasko@ is away?
Thanks for working on the fix here. I'm not sure I understand it, though. Can you add more to the CL description about what the fix is doing? https://codereview.chromium.org/2112923002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2112923002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_widget_host_view_child_frame.cc:82: UnlockMouse(); This seems a bit random to me. I wouldn't expect this method to have a bunch of feature-specific reset code here, or if it did, there would likely be more than just mouse lock. Is there a more general place that should trigger this? If not, can you add a comment about what sorts of things need resetting when the frame connector changes? https://codereview.chromium.org/2112923002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2112923002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_delegate.h:171: virtual bool HasMouseLock(RenderWidgetHostImpl* render_widget_host) = 0; Maybe we should have a default implementation that returns false, to avoid lots of the boilerplate overrides in this CL. https://codereview.chromium.org/2112923002/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2112923002/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:1686: DCHECK(mouse_lock_widget_ != render_widget_host); 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? https://codereview.chromium.org/2112923002/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:1909: GetTopLevelRenderWidgetHostView()->IsMouseLocked(); 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?
Oh, and is there a way to add a test for this? It looks like all the test changes were just adding a default implementation for the new method.
Description was changed from ========== Fix crash when destroying a RenderWidgetHost that holds the pointer lock. BUG=619571 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix crash when destroying a RenderWidgetHost that holds the pointer lock. BUG=619571 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Fix crash when destroying a RenderWidgetHost that holds the pointer lock. BUG=619571 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix crash when destroying a RenderWidgetHost that holds the pointer lock. This CL fixes two separate issues. One is that when a CrossProcessFrameConnector is being destroyed, if the RenderWidgetHostViewChildFrame holds the mouse lock it must unlock the mouse. The second issue is that when flash holds a fullscreen pointer lock and releases it, the WebContents is notified of the lost mouse lock, even though it was never notifies that the mouse lock was acquired (because flash has extra privileges to acquire the lock). BUG=619571 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Fix crash when destroying a RenderWidgetHost that holds the pointer lock. This CL fixes two separate issues. One is that when a CrossProcessFrameConnector is being destroyed, if the RenderWidgetHostViewChildFrame holds the mouse lock it must unlock the mouse. The second issue is that when flash holds a fullscreen pointer lock and releases it, the WebContents is notified of the lost mouse lock, even though it was never notifies that the mouse lock was acquired (because flash has extra privileges to acquire the lock). BUG=619571 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix crash when destroying a RenderWidgetHost that holds the pointer lock. This CL fixes two separate issues. One is that when a CrossProcessFrameConnector is being destroyed, if the RenderWidgetHostViewChildFrame holds the mouse lock it must unlock the mouse. The second issue is that when flash holds a fullscreen pointer lock and releases it, the WebContents is notified of the lost mouse lock, even though it was never notified that the mouse lock was acquired (because flash has extra privileges to acquire the lock). BUG=619571 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Please take a look. I've updated the CL description and added a test. I've also found another issue with flash plugins going fullscreen and requesting pointer lock -- they are privileged RWH which can auto-approve the mouse lock request and they were not notifying the WebContents (there's no test for this though, this is pretty hard as I would need a way to request pointer lock from a flash plugin). https://codereview.chromium.org/2112923002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2112923002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_widget_host_view_child_frame.cc:82: UnlockMouse(); On 2016/07/06 17:41:47, Charlie Reis wrote: > This seems a bit random to me. I wouldn't expect this method to have a bunch of > feature-specific reset code here, or if it did, there would likely be more than > just mouse lock. Is there a more general place that should trigger this? If > not, can you add a comment about what sorts of things need resetting when the > frame connector changes? For both Aura and Mac, this cleanup happens in the destructor. The problem in RWHVChildFrame, is that after it loses the CrossProcessFrameConnector there's no way to walk the frame tree anymore, so I thought this was the next best place to have the cleanup. I think the option to unlock it in the destructor would be possible by using a virtual in RWHDelegate, and do the unlock through WebContents, but that seems like a much worse option than doing it here (since we would have to add a bunch of code to RenderWidgetHost that's specific to the child frame case). I'll add a comment about cleaning up things that need to walk the frame tree. https://codereview.chromium.org/2112923002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2112923002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_delegate.h:171: virtual bool HasMouseLock(RenderWidgetHostImpl* render_widget_host) = 0; On 2016/07/06 17:41:47, Charlie Reis wrote: > Maybe we should have a default implementation that returns false, to avoid lots > of the boilerplate overrides in this CL. Done. I had that before, but clang complained about non-empty functions not being allowed in headers -- but I just realized that there's also a .cc implementation for RenderWidgetHostDelegate. https://codereview.chromium.org/2112923002/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2112923002/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:1686: DCHECK(mouse_lock_widget_ != render_widget_host); On 2016/07/06 17:41:47, Charlie Reis wrote: > 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? 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). https://codereview.chromium.org/2112923002/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:1909: GetTopLevelRenderWidgetHostView()->IsMouseLocked(); On 2016/07/06 17:41:47, Charlie Reis wrote: > 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? 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.
Thanks. (Sounds subtle to get this right!) LGTM with nits. https://codereview.chromium.org/2112923002/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2112923002/diff/1/content/browser/frame_host/... content/browser/frame_host/render_widget_host_view_child_frame.cc:82: UnlockMouse(); On 2016/07/07 20:17:35, lfg wrote: > On 2016/07/06 17:41:47, Charlie Reis wrote: > > This seems a bit random to me. I wouldn't expect this method to have a bunch > of > > feature-specific reset code here, or if it did, there would likely be more > than > > just mouse lock. Is there a more general place that should trigger this? If > > not, can you add a comment about what sorts of things need resetting when the > > frame connector changes? > > For both Aura and Mac, this cleanup happens in the destructor. The problem in > RWHVChildFrame, is that after it loses the CrossProcessFrameConnector there's no > way to walk the frame tree anymore, so I thought this was the next best place to > have the cleanup. I think the option to unlock it in the destructor would be > possible by using a virtual in RWHDelegate, and do the unlock through > WebContents, but that seems like a much worse option than doing it here (since > we would have to add a bunch of code to RenderWidgetHost that's specific to the > child frame case). > > I'll add a comment about cleaning up things that need to walk the frame tree. Acknowledged. https://codereview.chromium.org/2112923002/diff/1/content/browser/web_content... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2112923002/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:1686: DCHECK(mouse_lock_widget_ != render_widget_host); On 2016/07/07 20:17:35, lfg wrote: > On 2016/07/06 17:41:47, Charlie Reis wrote: > > 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? > > 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). Acknowledged. https://codereview.chromium.org/2112923002/diff/1/content/browser/web_content... content/browser/web_contents/web_contents_impl.cc:1909: GetTopLevelRenderWidgetHostView()->IsMouseLocked(); On 2016/07/07 20:17:35, lfg wrote: > On 2016/07/06 17:41:47, Charlie Reis wrote: > > 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? > > 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. Acknowledged. https://codereview.chromium.org/2112923002/diff/60001/chrome/browser/site_per... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/2112923002/diff/60001/chrome/browser/site_per... chrome/browser/site_per_process_interactive_browsertest.cc:767: "removeChild(document.querySelector('iframe'))")); Maybe it's worth adding a comment that this used to crash due to https://crbug.com/619571. Also, is it useful to check mouse_locked again after this? https://codereview.chromium.org/2112923002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2112923002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_child_frame.cc:83: // needs to be done through the CrossProcessFrameConnector. nit: s/Cleanup/Clean up/ nit: Add "before it's gone." nit: Blank line between lines 83 and 84. https://codereview.chromium.org/2112923002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2112923002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_delegate.h:147: // widget host. Can you mention what privileged means?
lfg@chromium.org changed reviewers: + alexmos@chromium.org
alexmos@chromium.org: Please review changes in site_per_process_interactive_browsertest.cc https://codereview.chromium.org/2112923002/diff/60001/chrome/browser/site_per... File chrome/browser/site_per_process_interactive_browsertest.cc (right): https://codereview.chromium.org/2112923002/diff/60001/chrome/browser/site_per... chrome/browser/site_per_process_interactive_browsertest.cc:767: "removeChild(document.querySelector('iframe'))")); On 2016/07/07 21:47:13, Charlie Reis wrote: > Maybe it's worth adding a comment that this used to crash due to > https://crbug.com/619571. Done. > Also, is it useful to check mouse_locked again after this? Done. https://codereview.chromium.org/2112923002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2112923002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_child_frame.cc:83: // needs to be done through the CrossProcessFrameConnector. On 2016/07/07 21:47:13, Charlie Reis wrote: > nit: s/Cleanup/Clean up/ > nit: Add "before it's gone." > nit: Blank line between lines 83 and 84. Done. https://codereview.chromium.org/2112923002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/2112923002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_delegate.h:147: // widget host. On 2016/07/07 21:47:13, Charlie Reis wrote: > Can you mention what privileged means? Done.
Test LGTM.
The CQ bit was checked by lfg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2112923002/#ps100001 (title: "addressing comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lfg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lfg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lfg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lfg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lfg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2112923002/#ps120001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix crash when destroying a RenderWidgetHost that holds the pointer lock. This CL fixes two separate issues. One is that when a CrossProcessFrameConnector is being destroyed, if the RenderWidgetHostViewChildFrame holds the mouse lock it must unlock the mouse. The second issue is that when flash holds a fullscreen pointer lock and releases it, the WebContents is notified of the lost mouse lock, even though it was never notified that the mouse lock was acquired (because flash has extra privileges to acquire the lock). BUG=619571 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix crash when destroying a RenderWidgetHost that holds the pointer lock. This CL fixes two separate issues. One is that when a CrossProcessFrameConnector is being destroyed, if the RenderWidgetHostViewChildFrame holds the mouse lock it must unlock the mouse. The second issue is that when flash holds a fullscreen pointer lock and releases it, the WebContents is notified of the lost mouse lock, even though it was never notified that the mouse lock was acquired (because flash has extra privileges to acquire the lock). BUG=619571 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Fix crash when destroying a RenderWidgetHost that holds the pointer lock. This CL fixes two separate issues. One is that when a CrossProcessFrameConnector is being destroyed, if the RenderWidgetHostViewChildFrame holds the mouse lock it must unlock the mouse. The second issue is that when flash holds a fullscreen pointer lock and releases it, the WebContents is notified of the lost mouse lock, even though it was never notified that the mouse lock was acquired (because flash has extra privileges to acquire the lock). BUG=619571 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix crash when destroying a RenderWidgetHost that holds the pointer lock. This CL fixes two separate issues. One is that when a CrossProcessFrameConnector is being destroyed, if the RenderWidgetHostViewChildFrame holds the mouse lock it must unlock the mouse. The second issue is that when flash holds a fullscreen pointer lock and releases it, the WebContents is notified of the lost mouse lock, even though it was never notified that the mouse lock was acquired (because flash has extra privileges to acquire the lock). BUG=619571 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/ad824435a11fa19a1707c5e4af5dc405fe4fe708 Cr-Commit-Position: refs/heads/master@{#405195} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ad824435a11fa19a1707c5e4af5dc405fe4fe708 Cr-Commit-Position: refs/heads/master@{#405195} |
