Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2038843003:
bluetooth: Don't call methods of WebContentsImpl while it's being destroyed. (Closed)
|
Created:
4 years, 6 months ago by ortuno Modified:
4 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, ortuno+watch_chromium.org, scheib+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Don't call methods of WebContentsImpl while it's being destroyed.
BUG=615319
Committed: https://crrev.com/467e5793a107a79b36c914a97d6bbbfda7430360
Cr-Commit-Position: refs/heads/master@{#399091}
Patch Set 1 #Patch Set 2 : Fix dcheck #
Total comments: 10
Patch Set 3 : Address jyasskin's comments #
Total comments: 8
Patch Set 4 : Address moar comments #
Total comments: 6
Patch Set 5 : Remove test #
Messages
Total messages: 25 (8 generated)
ortuno@chromium.org changed reviewers: + jyasskin@chromium.org
jyasskin: PTAL. This is as straightforward as I could get it.
https://codereview.chromium.org/2038843003/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/frame_connected_bluetooth_devices.cc (right): https://codereview.chromium.org/2038843003/diff/20001/content/browser/bluetoo... content/browser/bluetooth/frame_connected_bluetooth_devices.cc:104: DCHECK(!web_contents_impl_->IsBeingDestroyed()); Because this has security implications, make it a CHECK. https://codereview.chromium.org/2038843003/diff/20001/content/browser/bluetoo... content/browser/bluetooth/frame_connected_bluetooth_devices.cc:109: if (web_contents_impl_->IsBeingDestroyed()) { Would it make more sense to put this check in WebContentsImpl::DecrementBluetoothConnectedDeviceCount()? It's still alive at this point, and can protect itself against calling out to its clients. https://codereview.chromium.org/2038843003/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/frame_connected_bluetooth_devices_unittest.cc (right): https://codereview.chromium.org/2038843003/diff/20001/content/browser/bluetoo... content/browser/bluetooth/frame_connected_bluetooth_devices_unittest.cc:357: TEST_F(FrameConnectedBluetoothDevicesTest, WebContentsImpl_Destruction) { It's good to write the overall meaning of the test in its name if possible. e.g. "DestroyedByWebContentsImplDestruction"? https://codereview.chromium.org/2038843003/diff/20001/content/browser/bluetoo... content/browser/bluetooth/frame_connected_bluetooth_devices_unittest.cc:358: // Tests that we don't crash when FrameConnectedBluetoothDeviceTest is Did you mean FrameConnectedBluetoothDevices here? https://codereview.chromium.org/2038843003/diff/20001/content/browser/bluetoo... content/browser/bluetooth/frame_connected_bluetooth_devices_unittest.cc:359: // destroyed while WebContentsImpl is being destroyed. It's not just "while", right? It's when FrameConnectedBluetoothDevices contains a connection when WebContentsImpl's destruction causes it to be destroyed.
https://codereview.chromium.org/2038843003/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/frame_connected_bluetooth_devices.cc (right): https://codereview.chromium.org/2038843003/diff/20001/content/browser/bluetoo... content/browser/bluetooth/frame_connected_bluetooth_devices.cc:104: DCHECK(!web_contents_impl_->IsBeingDestroyed()); On 2016/06/06 at 16:55:36, Jeffrey Yasskin wrote: > Because this has security implications, make it a CHECK. Done. https://codereview.chromium.org/2038843003/diff/20001/content/browser/bluetoo... content/browser/bluetooth/frame_connected_bluetooth_devices.cc:109: if (web_contents_impl_->IsBeingDestroyed()) { On 2016/06/06 at 16:55:36, Jeffrey Yasskin wrote: > Would it make more sense to put this check in WebContentsImpl::DecrementBluetoothConnectedDeviceCount()? It's still alive at this point, and can protect itself against calling out to its clients. I think so. Done. https://codereview.chromium.org/2038843003/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/frame_connected_bluetooth_devices_unittest.cc (right): https://codereview.chromium.org/2038843003/diff/20001/content/browser/bluetoo... content/browser/bluetooth/frame_connected_bluetooth_devices_unittest.cc:357: TEST_F(FrameConnectedBluetoothDevicesTest, WebContentsImpl_Destruction) { On 2016/06/06 at 16:55:36, Jeffrey Yasskin wrote: > It's good to write the overall meaning of the test in its name if possible. e.g. "DestroyedByWebContentsImplDestruction"? Done. https://codereview.chromium.org/2038843003/diff/20001/content/browser/bluetoo... content/browser/bluetooth/frame_connected_bluetooth_devices_unittest.cc:358: // Tests that we don't crash when FrameConnectedBluetoothDeviceTest is On 2016/06/06 at 16:55:36, Jeffrey Yasskin wrote: > Did you mean FrameConnectedBluetoothDevices here? Done. https://codereview.chromium.org/2038843003/diff/20001/content/browser/bluetoo... content/browser/bluetooth/frame_connected_bluetooth_devices_unittest.cc:359: // destroyed while WebContentsImpl is being destroyed. On 2016/06/06 at 16:55:36, Jeffrey Yasskin wrote: > It's not just "while", right? It's when FrameConnectedBluetoothDevices contains a connection when WebContentsImpl's destruction causes it to be destroyed. Done.
lgtm https://codereview.chromium.org/2038843003/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2038843003/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4993: // Trying to update invalidate the tab state while being destroyed grammar: "update invalidate" https://codereview.chromium.org/2038843003/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:5006: // Trying to update invalidate the tab state while being destroyed grammar: "update invalidate" https://codereview.chromium.org/2038843003/diff/40001/content/test/test_rende... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/2038843003/diff/40001/content/test/test_rende... content/test/test_render_frame_host.h:13: #include "content/browser/bluetooth/frame_connected_bluetooth_devices.h" You could get by with just a forward declaration in the header, since the destructor and the unique_ptr<> use are in the .cc file. https://codereview.chromium.org/2038843003/diff/40001/content/test/test_rende... content/test/test_render_frame_host.h:160: std::unique_ptr<FrameConnectedBluetoothDevices> connected_devices_; Maybe comment that this exists to test destruction of an instance of this type? I'm not sure that the owners will want that comment, but it's a somewhat unusual reason to have a member here.
Thanks! https://codereview.chromium.org/2038843003/diff/40001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2038843003/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:4993: // Trying to update invalidate the tab state while being destroyed On 2016/06/06 at 20:32:20, Jeffrey Yasskin wrote: > grammar: "update invalidate" Done. https://codereview.chromium.org/2038843003/diff/40001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:5006: // Trying to update invalidate the tab state while being destroyed On 2016/06/06 at 20:32:20, Jeffrey Yasskin wrote: > grammar: "update invalidate" Done. Sorry! https://codereview.chromium.org/2038843003/diff/40001/content/test/test_rende... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/2038843003/diff/40001/content/test/test_rende... content/test/test_render_frame_host.h:13: #include "content/browser/bluetooth/frame_connected_bluetooth_devices.h" On 2016/06/06 at 20:32:20, Jeffrey Yasskin wrote: > You could get by with just a forward declaration in the header, since the destructor and the unique_ptr<> use are in the .cc file. Done. https://codereview.chromium.org/2038843003/diff/40001/content/test/test_rende... content/test/test_render_frame_host.h:160: std::unique_ptr<FrameConnectedBluetoothDevices> connected_devices_; On 2016/06/06 at 20:32:20, Jeffrey Yasskin wrote: > Maybe comment that this exists to test destruction of an instance of this type? I'm not sure that the owners will want that comment, but it's a somewhat unusual reason to have a member here. Done.
ortuno@chromium.org changed reviewers: + nick@chromium.org
nick: PTAL
https://codereview.chromium.org/2038843003/diff/60001/content/test/test_rende... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/2038843003/diff/60001/content/test/test_rende... content/test/test_render_frame_host.h:163: std::unique_ptr<FrameConnectedBluetoothDevices> connected_devices_; I'm not sure why this is needed here, since you never use its value. If it's just a matter of not leaking memory, seems like you could just put it in a unique_ptr in the test code? TestRenderFrameHost is the place to stub out actual behaviors of RenderFrameHostImpl that would be problematic in a unittest. Where possible you should avoid doing this, since every such stub makes the test a little less realistic -- which is to say, it's best to depend on the actual behavior of WebContentsImpl/RenderFrameHostImpl, particularly for ownership+cleanup!
https://codereview.chromium.org/2038843003/diff/60001/content/test/test_rende... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/2038843003/diff/60001/content/test/test_rende... content/test/test_render_frame_host.h:163: std::unique_ptr<FrameConnectedBluetoothDevices> connected_devices_; On 2016/06/06 at 22:52:25, ncarter wrote: > I'm not sure why this is needed here, since you never use its value. If it's just a matter of not leaking memory, seems like you could just put it in a unique_ptr in the test code? > The particular case we are trying to hit is when an instance of our object gets destroyed as part of the destruction of WebContentsImpl. For that we need to add our object to an object owned by WebContentsImpl, in this case we added to TestRenderFrameHost. > TestRenderFrameHost is the place to stub out actual behaviors of RenderFrameHostImpl that would be problematic in a unittest. Where possible you should avoid doing this, since every such stub makes the test a little less realistic -- which is to say, it's best to depend on the actual behavior of WebContentsImpl/RenderFrameHostImpl, particularly for ownership+cleanup! Ahh I see. Could you recommend another test object where we could add our object to test the case described above?
https://codereview.chromium.org/2038843003/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2038843003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:755: // The following two methods have no effect while IsBeingDestroyed is true. This is an implementation detail that doesn't belong in the header comment. The header comment should describe the purpose of these methods. https://codereview.chromium.org/2038843003/diff/60001/content/test/test_rende... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/2038843003/diff/60001/content/test/test_rende... content/test/test_render_frame_host.h:163: std::unique_ptr<FrameConnectedBluetoothDevices> connected_devices_; On 2016/06/06 23:01:18, ortuno wrote: > On 2016/06/06 at 22:52:25, ncarter wrote: > > I'm not sure why this is needed here, since you never use its value. If it's > just a matter of not leaking memory, seems like you could just put it in a > unique_ptr in the test code? > > > > The particular case we are trying to hit is when an instance of our object gets > destroyed as part of the destruction of WebContentsImpl. For that we need to add > our object to an object owned by WebContentsImpl, in this case we added to > TestRenderFrameHost. > > > TestRenderFrameHost is the place to stub out actual behaviors of > RenderFrameHostImpl that would be problematic in a unittest. Where possible you > should avoid doing this, since every such stub makes the test a little less > realistic -- which is to say, it's best to depend on the actual behavior of > WebContentsImpl/RenderFrameHostImpl, particularly for ownership+cleanup! > > Ahh I see. Could you recommend another test object where we could add our object > to test the case described above? Sorry for the delay in replying to this. Let's split the fix off separate from the tests, and do a little more work on these unit tests, as described in the rest of these comments. So, to be clear, my problem with implementing the tests this way, is that it feels like we're recreating the bug here, in test code, just to test the bug. A test for this fix ought to go through whatever mechanism actually frees FrameConnectedBluetoothDevices in practice, rather than simulating that mechanism. I wasn't sure how to do this with a mojo service, so I patched in your change and played around with it. It turns out to be not that hard to mock out a call to CreateWebBluetoothService(), so I think this is a good way forward. As a bonus, you wind up with a WebBluetoothServicePtr that you can use to actually send Mojo messages to the frame. I expect that structuring your unit tests that way (where you actually create a WebBluetoothServiceImpl, and write the test in terms of mojo calls methods) might improve the value of these tests. I've uploaded a patch here https://codereview.chromium.org/2050803002. It passes these tests but is definitely not landable -- see the TODOs in the code. Also, FYI, I think I am a little out of date, since I still have a GetBluetoothDispatcherHost() function locally. If you agree with that general approach, please take over that patch?
https://codereview.chromium.org/2038843003/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2038843003/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl.h:755: // The following two methods have no effect while IsBeingDestroyed is true. On 2016/06/08 at 19:15:35, ncarter wrote: > This is an implementation detail that doesn't belong in the header comment. The header comment should describe the purpose of these methods. Done. https://codereview.chromium.org/2038843003/diff/60001/content/test/test_rende... File content/test/test_render_frame_host.h (right): https://codereview.chromium.org/2038843003/diff/60001/content/test/test_rende... content/test/test_render_frame_host.h:163: std::unique_ptr<FrameConnectedBluetoothDevices> connected_devices_; On 2016/06/08 at 19:15:35, ncarter wrote: > On 2016/06/06 23:01:18, ortuno wrote: > > On 2016/06/06 at 22:52:25, ncarter wrote: > > > I'm not sure why this is needed here, since you never use its value. If it's > > just a matter of not leaking memory, seems like you could just put it in a > > unique_ptr in the test code? > > > > > > > The particular case we are trying to hit is when an instance of our object gets > > destroyed as part of the destruction of WebContentsImpl. For that we need to add > > our object to an object owned by WebContentsImpl, in this case we added to > > TestRenderFrameHost. > > > > > TestRenderFrameHost is the place to stub out actual behaviors of > > RenderFrameHostImpl that would be problematic in a unittest. Where possible you > > should avoid doing this, since every such stub makes the test a little less > > realistic -- which is to say, it's best to depend on the actual behavior of > > WebContentsImpl/RenderFrameHostImpl, particularly for ownership+cleanup! > > > > Ahh I see. Could you recommend another test object where we could add our object > > to test the case described above? > > Sorry for the delay in replying to this. Let's split the fix off separate from the tests, and do a little more work on these unit tests, as described in the rest of these comments. > > So, to be clear, my problem with implementing the tests this way, is that it feels like we're recreating the bug here, in test code, just to test the bug. > > A test for this fix ought to go through whatever mechanism actually frees FrameConnectedBluetoothDevices in practice, rather than simulating that mechanism. > > I wasn't sure how to do this with a mojo service, so I patched in your change and played around with it. It turns out to be not that hard to mock out a call to CreateWebBluetoothService(), so I think this is a good way forward. As a bonus, you wind up with a WebBluetoothServicePtr that you can use to actually send Mojo messages to the frame. I expect that structuring your unit tests that way (where you actually create a WebBluetoothServiceImpl, and write the test in terms of mojo calls methods) might improve the value of these tests. > > I've uploaded a patch here https://codereview.chromium.org/2050803002. It passes these tests but is definitely not landable -- see the TODOs in the code. Also, FYI, I think I am a little out of date, since I still have a GetBluetoothDispatcherHost() function locally. > > If you agree with that general approach, please take over that patch? Thanks for the patch! I really appreciate it :) I did consider the approach taken in the patch but the work to fix the bad cast seemed non-trivial and I didn't want to land this fix without a test but I also wanted to land it fast since it's somewhat security sensitive. If you are OK with landing the fix with no test I will work on fixing the bad cast and take the approach from the patch for the tests. Thanks again for the patch!
lgtm
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jyasskin@chromium.org Link to the patchset: https://codereview.chromium.org/2038843003/#ps80001 (title: "Remove test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038843003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by ortuno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038843003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Don't call methods of WebContentsImpl while it's being destroyed. Also adds a test to excercise that code path. BUG=615319 ========== to ========== bluetooth: Don't call methods of WebContentsImpl while it's being destroyed. Also adds a test to excercise that code path. BUG=615319 Committed: https://crrev.com/467e5793a107a79b36c914a97d6bbbfda7430360 Cr-Commit-Position: refs/heads/master@{#399091} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/467e5793a107a79b36c914a97d6bbbfda7430360 Cr-Commit-Position: refs/heads/master@{#399091}
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Don't call methods of WebContentsImpl while it's being destroyed. Also adds a test to excercise that code path. BUG=615319 Committed: https://crrev.com/467e5793a107a79b36c914a97d6bbbfda7430360 Cr-Commit-Position: refs/heads/master@{#399091} ========== to ========== bluetooth: Don't call methods of WebContentsImpl while it's being destroyed. BUG=615319 Committed: https://crrev.com/467e5793a107a79b36c914a97d6bbbfda7430360 Cr-Commit-Position: refs/heads/master@{#399091} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
