Call MessageFilter clean up handlers even for pending filters

ipc/ipc_channel_proxy.cc

 // Copyright (c) 2012 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.
 
 #include "ipc/ipc_channel_proxy.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <utility>
@@ skipping 193 matching lines @@
 void ChannelProxy::Context::OnChannelClosed() {
   // TODO(pkasting): Remove ScopedTracker below once crbug.com/477117 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "477117 ChannelProxy::Context::OnChannelClosed"));
   // It's okay for IPC::ChannelProxy::Close to be called more than once, which
   // would result in this branch being taken.
   if (!channel_)
     return;
 
-  for (size_t i = 0; i < filters_.size(); ++i) {
-    filters_[i]->OnChannelClosing();
-    filters_[i]->OnFilterRemoved();
+  for (auto& filter : pending_filters_) {

[Ken Rockot(use gerrit already), 2016/11/03 05:17:12]

Unlike filters_, pending_filters_ is accessed from different threads and needs
to be guarded by lock. This is a data race otherwise.

Note that this still doesn't account for filters which are added during or after
channel closure. i.e. if someone calls AddFilter on the main thread while
OnChannelClosed() is running on the IO thread (or any time after), that can
result in a filter being added to pending_filters_ (eventually moved to
filters_) and having OnFilterAdded() invoked on the filter. Such filters will
never have OnChannelClosing() or OnFilterRemoved() invoked.

You may want to address this case as well if your intent is to ensure that *all*
filters passed to AddFilter() guarantee these calls.

[tzik, 2016/11/04 07:13:56]

So, is the comment around line 226 no longer valid now?

[Ken Rockot(use gerrit already), 2016/11/04 20:05:30]

Oops. Sorry, I was conflating OnChannelError and OnChannelClosed in my head.
This should be fine as-is.

+    filter->OnChannelClosing();
+    filter->OnFilterRemoved();

[boliu, 2016/11/07 18:03:48]

the bug is filters in pending_fitlers_ here have never been added in the first
place, so calling OnFilterRemoved is wrong

I think this applies to OnChannelClosing, since OnChannelConnected never
happened either, so fix is just remove this whole block.., which reverts this
part of the CL..?

+  }
+  for (auto& filter : filters_) {
+    filter->OnChannelClosing();
+    filter->OnFilterRemoved();
   }
 
   // We don't need the filters anymore.
   message_filter_router_->Clear();
   filters_.clear();
   // We don't need the lock, because at this point, the listener thread can't
   // access it any more.
   pending_filters_.clear();
 
   ClearChannel();
@@ skipping 419 matching lines @@
   return channel->TakeClientFileDescriptor();
 }
 #endif
 
 void ChannelProxy::OnChannelInit() {
 }
 
 //-----------------------------------------------------------------------------
 
 }  // namespace IPC