bluetooth: Don't call methods of WebContentsImpl while it's being destroyed.

content/test/test_render_frame_host.h

Index: content/test/test_render_frame_host.h
diff --git a/content/test/test_render_frame_host.h b/content/test/test_render_frame_host.h
index f1718467fb7fcff9d4f4834464261bbe99f1c29a..e5130eff94833ba51dcfa1a46f7bbe8c821c63cd 100644
--- a/content/test/test_render_frame_host.h
+++ b/content/test/test_render_frame_host.h
@@ -21,6 +21,8 @@ struct FrameHostMsg_DidCommitProvisionalLoad_Params;
 
 namespace content {
 
+class FrameConnectedBluetoothDevices;
+
 class TestRenderFrameHostCreationObserver : public WebContentsObserver {
  public:
   explicit TestRenderFrameHostCreationObserver(WebContents* web_contents);
@@ -107,6 +109,9 @@ class TestRenderFrameHost : public RenderFrameHostImpl,
   void SendRendererInitiatedNavigationRequest(const GURL& url,
                                               bool has_user_gesture);
 
+  void SetFrameConnectedBluetoothDevices(
+      std::unique_ptr<FrameConnectedBluetoothDevices> connected_devices);
+
   void DidChangeOpener(int opener_routing_id);
 
   void DidEnforceStrictMixedContentChecking();
@@ -153,6 +158,10 @@ class TestRenderFrameHost : public RenderFrameHostImpl,
 
   std::string contents_mime_type_;
 
+  // This exists to test destruction of an instance of this type while
+  // the owning WebContentsImpl is being destroyed.
+  std::unique_ptr<FrameConnectedBluetoothDevices> connected_devices_;

[ncarter (slow), 2016/06/06 22:52:25]

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!

[ortuno, 2016/06/06 23:01:18]

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.
Ahh I see. Could you recommend another test object where we could add our object
to test the case described above?

[ncarter (slow), 2016/06/08 19:15:35]

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?

[ortuno, 2016/06/09 17:40:02]

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!

+
   // See set_simulate_history_list_was_cleared() above.
   bool simulate_history_list_was_cleared_;