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

content/test/test_render_frame_host.h

 // Copyright 2013 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.
 
 #ifndef CONTENT_TEST_TEST_RENDER_FRAME_HOST_H_
 #define CONTENT_TEST_TEST_RENDER_FRAME_HOST_H_
 
 #include <stdint.h>
 
 #include <vector>
 
 #include "base/macros.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/test/mock_render_process_host.h"
 #include "content/public/test/test_renderer_host.h"
 #include "content/test/test_render_view_host.h"
 #include "ui/base/page_transition_types.h"
 
 struct FrameHostMsg_DidCommitProvisionalLoad_Params;
 
 namespace content {
 
+class FrameConnectedBluetoothDevices;
+
 class TestRenderFrameHostCreationObserver : public WebContentsObserver {
  public:
   explicit TestRenderFrameHostCreationObserver(WebContents* web_contents);
   ~TestRenderFrameHostCreationObserver() override;
 
   // WebContentsObserver implementation.
   void RenderFrameCreated(RenderFrameHost* render_frame_host) override;
 
   RenderFrameHost* last_created_frame() const { return last_created_frame_; }
 
@@ skipping 66 matching lines @@
   // Simulate a renderer-initiated navigation up until commit.
   void NavigateAndCommitRendererInitiated(int page_id,
                                           bool did_create_new_entry,
                                           const GURL& url);
 
   // With the current navigation logic this method is a no-op.
   // PlzNavigate: this method simulates receiving a BeginNavigation IPC.
   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();
 
   // If set, navigations will appear to have cleared the history list in the
   // RenderFrame
   // (FrameHostMsg_DidCommitProvisionalLoad_Params::history_list_was_cleared).
   // False by default.
   void set_simulate_history_list_was_cleared(bool cleared) {
     simulate_history_list_was_cleared_ = cleared;
@@ skipping 26 matching lines @@
                                   int response_code,
                                   const ModificationCallback& callback);
 
   // Computes the page ID for a pending navigation in this RenderFrameHost;
   int32_t ComputeNextPageID();
 
   TestRenderFrameHostCreationObserver child_creation_observer_;
 
   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_;
 
   DISALLOW_COPY_AND_ASSIGN(TestRenderFrameHost);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_TEST_TEST_RENDER_FRAME_HOST_H_