Clear Mojo bindings for associated interfaces in OnFilterRemoved()

This clears the bindings right when the filter is removed, instead
of when the filter is destructed.

Speculative fix for the bug where apparently Mojo messages arrive after
the ServiceWorkerDispatcherHost has been destroyed.

Bug: 736203
Change-Id: Ifae0db23a6a17cfa30b558f714916001d37f6571
Reviewed-on: https://chromium-review.googlesource.com/564746
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486781}

content/browser/browser_associated_interface_unittest.cc

@@ -92,6 +92,12 @@ class TestDriverMessageFilter
     return true;
   }
 
+  void OnFilterRemoved() override {
+    // Check that the bindings are cleared by
+    // BrowserAssociatedInterface::ClearBindings() callbacks.
+    EXPECT_FALSE(internal_state_->bindings_.get());
+  }
+
   // mojom::BrowserAssociatedInterfaceTestDriver:
   void ExpectString(const std::string& expected) override {
     next_expected_string_ = expected;

content/public/browser/browser_associated_interface.h

@@ -58,14 +58,15 @@ class BrowserAssociatedInterface {
     internal_state_->Initialize();
     filter->AddAssociatedInterface(
         Interface::Name_,
-        base::Bind(&InternalState::BindRequest, internal_state_));
+        base::Bind(&InternalState::BindRequest, internal_state_),
+        base::BindOnce(&InternalState::ClearBindings, internal_state_));
   }
 
-  ~BrowserAssociatedInterface() {
-    internal_state_->ShutDown();
-  }
+  ~BrowserAssociatedInterface() { internal_state_->ClearBindings(); }
 
  private:
+  friend class TestDriverMessageFilter;
+
   class InternalState : public base::RefCountedThreadSafe<InternalState> {
    public:
     explicit InternalState(Interface* impl) : impl_(impl) {}
@@ -79,10 +80,11 @@ class BrowserAssociatedInterface {
       bindings_.reset(new mojo::AssociatedBindingSet<Interface>);
     }
 
-    void ShutDown() {
+    void ClearBindings() {
       if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
-        BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
-                                base::Bind(&InternalState::ShutDown, this));
+        BrowserThread::PostTask(
+            BrowserThread::IO, FROM_HERE,
+            base::Bind(&InternalState::ClearBindings, this));
         return;
       }
       bindings_.reset();
@@ -99,6 +101,7 @@ class BrowserAssociatedInterface {
 
    private:
     friend class base::RefCountedThreadSafe<InternalState>;
+    friend class TestDriverMessageFilter;
 
     ~InternalState() {}
 

content/public/browser/browser_message_filter.cc

@@ -37,7 +37,12 @@ class BrowserMessageFilter::Internal : public IPC::MessageFilter {
     filter_->OnFilterAdded(channel);
   }
 
-  void OnFilterRemoved() override { filter_->OnFilterRemoved(); }
+  void OnFilterRemoved() override {
+    for (auto& callback : filter_->filter_removed_callbacks_)
+      std::move(callback).Run();
+    filter_->filter_removed_callbacks_.clear();
+    filter_->OnFilterRemoved();
+  }
 
   void OnChannelClosing() override {
     filter_->sender_ = nullptr;
@@ -112,8 +117,10 @@ BrowserMessageFilter::BrowserMessageFilter(
 
 void BrowserMessageFilter::AddAssociatedInterface(
     const std::string& name,
-    const IPC::ChannelProxy::GenericAssociatedInterfaceFactory& factory) {
+    const IPC::ChannelProxy::GenericAssociatedInterfaceFactory& factory,
+    base::OnceClosure filter_removed_callback) {
   associated_interfaces_.emplace_back(name, factory);
+  filter_removed_callbacks_.emplace_back(std::move(filter_removed_callback));
 }
 
 base::ProcessHandle BrowserMessageFilter::PeerHandle() {

content/public/browser/browser_message_filter.h

@@ -85,9 +85,13 @@ class CONTENT_EXPORT BrowserMessageFilter
 
   // Adds an associated interface factory to this filter. Must be called before
   // RegisterAssociatedInterfaces().
+  //
+  // |filter_removed_callback| is called on the IO thread when this filter is
+  // removed.
   void AddAssociatedInterface(
       const std::string& name,
-      const IPC::ChannelProxy::GenericAssociatedInterfaceFactory& factory);
+      const IPC::ChannelProxy::GenericAssociatedInterfaceFactory& factory,
+      const base::OnceClosure filter_removed_callback);
 
   // Can be called on any thread, after OnChannelConnected is called.
   base::ProcessHandle PeerHandle();
@@ -142,6 +146,9 @@ class CONTENT_EXPORT BrowserMessageFilter
   std::vector<std::pair<std::string,
                         IPC::ChannelProxy::GenericAssociatedInterfaceFactory>>
       associated_interfaces_;
+
+  // Callbacks to be called in OnFilterRemoved().
+  std::vector<base::OnceClosure> filter_removed_callbacks_;
 };
 
 struct BrowserMessageFilterTraits {