Make BrowserMessageFilter not derive from IPC::ChannelProxy::MessageFilter. This allows us to hide …

content/public/browser/browser_message_filter.cc

Index: content/public/browser/browser_message_filter.cc
===================================================================
--- content/public/browser/browser_message_filter.cc	(revision 225319)
+++ content/public/browser/browser_message_filter.cc	(working copy)
@@ -18,52 +18,88 @@
 
 namespace content {
 
-BrowserMessageFilter::BrowserMessageFilter()
-    : channel_(NULL),
-#if defined(OS_WIN)
-      peer_handle_(base::kNullProcessHandle),
-#endif
-      peer_pid_(base::kNullProcessId) {
-}
+class BrowserMessageFilter::Internal : public IPC::ChannelProxy::MessageFilter {
+ public:
+  explicit Internal(BrowserMessageFilter* filter) : filter_(filter) {
+  }
 
-void BrowserMessageFilter::OnFilterAdded(IPC::Channel* channel) {
-  channel_ = channel;
-}
+ private:
+  virtual ~Internal() {
+    filter_->internal_ = NULL;

[scherkus (not reviewing), 2013/09/26 19:57:37]

sanity check: this should be fine since GetFilter() should only be called during
creation?

[jam, 2013/09/26 20:22:39]

(removed now that I changed the lifetime)

+  }
 
-void BrowserMessageFilter::OnChannelClosing() {
-  channel_ = NULL;
-}
+  // IPC::ChannelProxy::MessageFilter implementation:
+  virtual void OnFilterAdded(IPC::Channel* channel) OVERRIDE {
+    filter_->channel_ = channel;
+    filter_->OnFilterAdded(channel);
+  }
 
-void BrowserMessageFilter::OnChannelConnected(int32 peer_pid) {
-  peer_pid_ = peer_pid;
-}
+  virtual void OnFilterRemoved() OVERRIDE {
+    filter_->OnFilterRemoved();
+  }
 
-bool BrowserMessageFilter::OnMessageReceived(const IPC::Message& message) {
-  BrowserThread::ID thread = BrowserThread::IO;
-  OverrideThreadForMessage(message, &thread);
+  virtual void OnChannelClosing() OVERRIDE {
+    filter_->channel_ = NULL;
+    filter_->OnChannelClosing();
+  }
 
-  if (thread == BrowserThread::IO) {
-    scoped_refptr<base::TaskRunner> runner =
-        OverrideTaskRunnerForMessage(message);
-    if (runner.get()) {
-      runner->PostTask(
-          FROM_HERE,
-          base::Bind(base::IgnoreResult(&BrowserMessageFilter::DispatchMessage),
-                     this,
-                     message));
+  virtual void OnChannelConnected(int32 peer_pid) OVERRIDE {
+    filter_->peer_pid_ = peer_pid;
+    filter_->OnChannelConnected(peer_pid);
+  }
+
+  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE {
+    BrowserThread::ID thread = BrowserThread::IO;
+    filter_->OverrideThreadForMessage(message, &thread);
+
+    if (thread == BrowserThread::IO) {
+      scoped_refptr<base::TaskRunner> runner =
+          filter_->OverrideTaskRunnerForMessage(message);
+      if (runner.get()) {
+        runner->PostTask(
+            FROM_HERE,
+            base::Bind(
+                base::IgnoreResult(&Internal::DispatchMessage), this, message));
+        return true;
+      }
+      return DispatchMessage(message);
+    }
+
+    if (thread == BrowserThread::UI &&
+        !BrowserMessageFilter::CheckCanDispatchOnUI(message, filter_)) {
       return true;
     }
-    return DispatchMessage(message);
-  }
 
-  if (thread == BrowserThread::UI && !CheckCanDispatchOnUI(message, this))
+    BrowserThread::PostTask(
+        thread, FROM_HERE,
+        base::Bind(
+          base::IgnoreResult(&Internal::DispatchMessage), this, message));

[scherkus (not reviewing), 2013/09/26 19:57:37]

indent

[jam, 2013/09/26 20:22:39]

Done.

     return true;
+  }
 
-  BrowserThread::PostTask(
-      thread, FROM_HERE,
-      base::Bind(base::IgnoreResult(&BrowserMessageFilter::DispatchMessage),
-                 this, message));
-  return true;
+  // Dispatches a message to the derived class.
+  bool DispatchMessage(const IPC::Message& message) {
+    bool message_was_ok = true;
+    bool rv = filter_->OnMessageReceived(message, &message_was_ok);
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO) || rv) <<
+        "Must handle messages that were dispatched to another thread!";
+    if (!message_was_ok) {
+      content::RecordAction(UserMetricsAction("BadMessageTerminate_BMF"));
+      filter_->BadMessageReceived();
+    }
+
+    return rv;
+  }
+
+  scoped_refptr<BrowserMessageFilter> filter_;
+};

[scherkus (not reviewing), 2013/09/26 19:57:37]

DISALLOW ?

[jam, 2013/09/26 20:22:39]

Done.

+
+BrowserMessageFilter::BrowserMessageFilter()
+    : internal_(new Internal(this)), channel_(NULL),

[scherkus (not reviewing), 2013/09/26 19:57:37]

I believe we'll leak internal_ if someone creates a BMF but never adds it
anywhere since the refcount will never get incremented

do we care about that case?

[jam, 2013/09/26 20:22:39]

good point, I didn't consider that.
at least one test turned out to be failing because of this:
ResolveProxyMsgHelperTest.CancelPendingRequests. but also we don't want memory
leaks in unittests. so i've switched ownership to be the other way around, like
you had in your cl. in retrospect this makes more sense, since then we don't
have to worry about the naked pointer ever being null.

+#if defined(OS_WIN)
+      peer_handle_(base::kNullProcessHandle),
+#endif
+      peer_pid_(base::kNullProcessId) {
 }
 
 base::ProcessHandle BrowserMessageFilter::PeerHandle() {
@@ -80,6 +116,11 @@
 #endif
 }
 
+
+void BrowserMessageFilter::OnDestruct() const {
+  delete this;
+}
+
 bool BrowserMessageFilter::Send(IPC::Message* message) {
   if (message->is_sync()) {
     // We don't support sending synchronous messages from the browser.  If we
@@ -106,10 +147,6 @@
   return false;
 }
 
-void BrowserMessageFilter::OverrideThreadForMessage(const IPC::Message& message,
-                                                    BrowserThread::ID* thread) {
-}
-
 base::TaskRunner* BrowserMessageFilter::OverrideTaskRunnerForMessage(
     const IPC::Message& message) {
   return NULL;
@@ -152,17 +189,8 @@
 #endif
 }
 
-bool BrowserMessageFilter::DispatchMessage(const IPC::Message& message) {
-  bool message_was_ok = true;
-  bool rv = OnMessageReceived(message, &message_was_ok);
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO) || rv) <<
-      "Must handle messages that were dispatched to another thread!";
-  if (!message_was_ok) {
-    content::RecordAction(UserMetricsAction("BadMessageTerminate_BMF"));
-    BadMessageReceived();
-  }
-
-  return rv;
+IPC::ChannelProxy::MessageFilter* BrowserMessageFilter::GetFilter() {
+  return internal_;
 }
 
 }  // namespace content