Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2469123003:
Call MessageFilter clean up handlers even for pending filters (Closed)
DescriptionCall MessageFilter clean up handlers even for pending filters
MessageFilters, specifically ResourceMessageFilter, need a chance to
clean up its state on IO thread, even if the IPC channel is closed before
the connection is fully established. Though in the existing code,
ChannelProxy doesn't call clean up functions for its |pending_filters_|.
After this CL, MessageFilter::OnChannelClosing and OnFilterRemoved will
be called for all MessageFilters passed to ChannelProxy::AddFilter.
Committed: https://crrev.com/0e85f1d17a35bbe7872d110e2894010801a9917b
Cr-Commit-Position: refs/heads/master@{#430159}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 4
Dependent Patchsets: Messages
Total messages: 30 (16 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Call MessageFilter clean up handlers even for pending filters BUG= ========== to ========== Call MessageFilter clean up handlers even for pending filters 123456789012345678901234567890123456789012345678901234567890123456789012 MessageFilters, specifically ResourceMessageFilter, need a chance to clean up its state on IO thread, even if the IPC channel is closed before the connection is fully established. Though in the existing code, ChannelProxy doesn't call clean up functions for its |pending_filters_|. After this CL, MessageFilter::OnChannelClosing and OnFilterRemoved will be called for all MessageFilters passed to ChannelProxy::AddFilter. BUG= ==========
Description was changed from ========== Call MessageFilter clean up handlers even for pending filters 123456789012345678901234567890123456789012345678901234567890123456789012 MessageFilters, specifically ResourceMessageFilter, need a chance to clean up its state on IO thread, even if the IPC channel is closed before the connection is fully established. Though in the existing code, ChannelProxy doesn't call clean up functions for its |pending_filters_|. After this CL, MessageFilter::OnChannelClosing and OnFilterRemoved will be called for all MessageFilters passed to ChannelProxy::AddFilter. BUG= ========== to ========== Call MessageFilter clean up handlers even for pending filters MessageFilters, specifically ResourceMessageFilter, need a chance to clean up its state on IO thread, even if the IPC channel is closed before the connection is fully established. Though in the existing code, ChannelProxy doesn't call clean up functions for its |pending_filters_|. After this CL, MessageFilter::OnChannelClosing and OnFilterRemoved will be called for all MessageFilters passed to ChannelProxy::AddFilter. ==========
tzik@chromium.org changed reviewers: + haraken@chromium.org, rockot@chromium.org
PTAL
(I think rockot should be a better reviewer for this.)
On 2016/11/02 04:38:30, haraken wrote: > (I think rockot should be a better reviewer for this.) Yeah, I'd like rockot@ to review //ipc part and haraken@ to review //content/browser/memory part as its owner.
On 2016/11/02 04:47:01, tzik wrote: > On 2016/11/02 04:38:30, haraken wrote: > > (I think rockot should be a better reviewer for this.) > > Yeah, I'd like rockot@ to review //ipc part and haraken@ to review > //content/browser/memory part as its owner. memory/ LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2469123003/diff/20001/ipc/ipc_channel_proxy.cc File ipc/ipc_channel_proxy.cc (right): https://codereview.chromium.org/2469123003/diff/20001/ipc/ipc_channel_proxy.c... ipc/ipc_channel_proxy.cc:214: for (auto& filter : pending_filters_) { 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.
https://codereview.chromium.org/2469123003/diff/20001/ipc/ipc_channel_proxy.cc File ipc/ipc_channel_proxy.cc (right): https://codereview.chromium.org/2469123003/diff/20001/ipc/ipc_channel_proxy.c... ipc/ipc_channel_proxy.cc:214: for (auto& filter : pending_filters_) { On 2016/11/03 05:17:12, Ken Rockot wrote: > 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. So, is the comment around line 226 no longer valid now?
lgtm https://codereview.chromium.org/2469123003/diff/20001/ipc/ipc_channel_proxy.cc File ipc/ipc_channel_proxy.cc (right): https://codereview.chromium.org/2469123003/diff/20001/ipc/ipc_channel_proxy.c... ipc/ipc_channel_proxy.cc:214: for (auto& filter : pending_filters_) { On 2016/11/04 at 07:13:56, tzik wrote: > On 2016/11/03 05:17:12, Ken Rockot wrote: > > 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. > > So, is the comment around line 226 no longer valid now? Oops. Sorry, I was conflating OnChannelError and OnChannelClosed in my head. This should be fine as-is.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2469123003/#ps20001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Call MessageFilter clean up handlers even for pending filters MessageFilters, specifically ResourceMessageFilter, need a chance to clean up its state on IO thread, even if the IPC channel is closed before the connection is fully established. Though in the existing code, ChannelProxy doesn't call clean up functions for its |pending_filters_|. After this CL, MessageFilter::OnChannelClosing and OnFilterRemoved will be called for all MessageFilters passed to ChannelProxy::AddFilter. ========== to ========== Call MessageFilter clean up handlers even for pending filters MessageFilters, specifically ResourceMessageFilter, need a chance to clean up its state on IO thread, even if the IPC channel is closed before the connection is fully established. Though in the existing code, ChannelProxy doesn't call clean up functions for its |pending_filters_|. After this CL, MessageFilter::OnChannelClosing and OnFilterRemoved will be called for all MessageFilters passed to ChannelProxy::AddFilter. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Call MessageFilter clean up handlers even for pending filters MessageFilters, specifically ResourceMessageFilter, need a chance to clean up its state on IO thread, even if the IPC channel is closed before the connection is fully established. Though in the existing code, ChannelProxy doesn't call clean up functions for its |pending_filters_|. After this CL, MessageFilter::OnChannelClosing and OnFilterRemoved will be called for all MessageFilters passed to ChannelProxy::AddFilter. ========== to ========== Call MessageFilter clean up handlers even for pending filters MessageFilters, specifically ResourceMessageFilter, need a chance to clean up its state on IO thread, even if the IPC channel is closed before the connection is fully established. Though in the existing code, ChannelProxy doesn't call clean up functions for its |pending_filters_|. After this CL, MessageFilter::OnChannelClosing and OnFilterRemoved will be called for all MessageFilters passed to ChannelProxy::AddFilter. Committed: https://crrev.com/0e85f1d17a35bbe7872d110e2894010801a9917b Cr-Commit-Position: refs/heads/master@{#430159} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0e85f1d17a35bbe7872d110e2894010801a9917b Cr-Commit-Position: refs/heads/master@{#430159}
Message was sent while issue was closed.
boliu@chromium.org changed reviewers: + boliu@chromium.org
Message was sent while issue was closed.
Hi, a test seemed to have caught a real bug in this CL, can we fix/revert asap? it's a relatively important test to keep running: https://bugs.chromium.org/p/chromium/issues/detail?id=662688#c7
Message was sent while issue was closed.
https://codereview.chromium.org/2469123003/diff/20001/ipc/ipc_channel_proxy.cc File ipc/ipc_channel_proxy.cc (right): https://codereview.chromium.org/2469123003/diff/20001/ipc/ipc_channel_proxy.c... ipc/ipc_channel_proxy.cc:216: filter->OnFilterRemoved(); 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..?
Message was sent while issue was closed.
looking at the CL description, it's actually the intention to call OnFilterRemoved/Closed even if it the filter hasn't been added in the first place? Imo, this is not safe, there are lots of existing filters written with the assumption that these calls happen in pairs, and breaking that is probably going to introduce real bugs
Message was sent while issue was closed.
On 2016/11/07 18:50:46, boliu wrote: > looking at the CL description, it's actually the intention to call > OnFilterRemoved/Closed even if it the filter hasn't been added in the first > place? Imo, this is not safe, there are lots of existing filters written with > the assumption that these calls happen in pairs, and breaking that is probably > going to introduce real bugs Oh, sorry. I thought I checked all subclasses of MessageFilter to ensure it's safe to call, though I overlooked GpuChannel. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
