Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2488753002:
Ensure clean up functions of MessageFilter are paired with their initialization functions (Closed)
DescriptionEnsure clean up functions of MessageFilter are paired with their initialization functions
This is an attempt to address http://crrev.com/2469123003/#msg28.
MessageFilter::OnFilterRemoved and OnChannelClosing are now called even
when the channel is closed before it completes its initialization.
In that case, these functions are not paired to OnFilterAdded and
OnChannelOpened.
This CL makes OnFilterAdded and OnChannelConnected right before pending
filters are removed, so that they are paired with their clean up functions.
BUG=
Committed: https://crrev.com/8941d772a304208a5ad4fb47dfa3c669fb3b5d11
Cr-Commit-Position: refs/heads/master@{#431199}
Patch Set 1 #Patch Set 2 : . #
Total comments: 2
Patch Set 3 : +comment #Messages
Total messages: 28 (19 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 ========== Add MessageFilter::OnShutdown BUG= ========== to ========== Add MessageFilter::OnShutdown BUG= ==========
tzik@chromium.org changed reviewers: + boliu@chromium.org, rockot@chromium.org
Description was changed from ========== Add MessageFilter::OnShutdown BUG= ========== to ========== Add MessageFilter::OnShutdown 123456789012345678901234567890123456789012345678901234567890123456789012 This is an attempt to address http://crrev.com/2469123003/#msg28. MessageFilter::OnFilterRemoved and OnChannelClosing are now called even when the channel is closed before its initialization is finished. In that case, these functions are not paired to OnFilterAdded and OnChannelOpened. This CL reverts the previous CL, and introduces OnShutdown as another notification. BUG= ==========
Description was changed from ========== Add MessageFilter::OnShutdown 123456789012345678901234567890123456789012345678901234567890123456789012 This is an attempt to address http://crrev.com/2469123003/#msg28. MessageFilter::OnFilterRemoved and OnChannelClosing are now called even when the channel is closed before its initialization is finished. In that case, these functions are not paired to OnFilterAdded and OnChannelOpened. This CL reverts the previous CL, and introduces OnShutdown as another notification. BUG= ========== to ========== Add MessageFilter::OnShutdown This is an attempt to address http://crrev.com/2469123003/#msg28. MessageFilter::OnFilterRemoved and OnChannelClosing are now called even when the channel is closed before its initialization is finished. In that case, these functions are not paired to OnFilterAdded and OnChannelOpened. This CL reverts the previous CL, and introduces OnShutdown as another notification. BUG= ==========
Description was changed from ========== Add MessageFilter::OnShutdown This is an attempt to address http://crrev.com/2469123003/#msg28. MessageFilter::OnFilterRemoved and OnChannelClosing are now called even when the channel is closed before its initialization is finished. In that case, these functions are not paired to OnFilterAdded and OnChannelOpened. This CL reverts the previous CL, and introduces OnShutdown as another notification. BUG= ========== to ========== Add MessageFilter::OnShutdown This is an attempt to address http://crrev.com/2469123003/#msg28. MessageFilter::OnFilterRemoved and OnChannelClosing are now called even when the channel is closed before it completes its initialization. In that case, these functions are not paired to OnFilterAdded and OnChannelOpened. This CL reverts the previous CL, and introduces OnShutdown as another notification. BUG= ==========
PTAL. This is another proposal to fix the test failure after http://crrev.com/2469123003.
The semantics of all these different events seem like they would easily confuse a reader. I wonder if we could just get away with calling OnFilterAdded immediately before OnFilterRemoved etc when clearing pending filters.
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...
Description was changed from ========== Add MessageFilter::OnShutdown This is an attempt to address http://crrev.com/2469123003/#msg28. MessageFilter::OnFilterRemoved and OnChannelClosing are now called even when the channel is closed before it completes its initialization. In that case, these functions are not paired to OnFilterAdded and OnChannelOpened. This CL reverts the previous CL, and introduces OnShutdown as another notification. BUG= ========== to ========== Add MessageFilter::OnShutdown This is an attempt to address http://crrev.com/2469123003/#msg28. MessageFilter::OnFilterRemoved and OnChannelClosing are now called even when the channel is closed before it completes its initialization. In that case, these functions are not paired to OnFilterAdded and OnChannelOpened. This CL makes OnFilterAdded and OnChannelConnected right before pending filters are removed, so that they are paired with their clean up functions. BUG= ==========
Description was changed from ========== Add MessageFilter::OnShutdown This is an attempt to address http://crrev.com/2469123003/#msg28. MessageFilter::OnFilterRemoved and OnChannelClosing are now called even when the channel is closed before it completes its initialization. In that case, these functions are not paired to OnFilterAdded and OnChannelOpened. This CL makes OnFilterAdded and OnChannelConnected right before pending filters are removed, so that they are paired with their clean up functions. BUG= ========== to ========== Ensure clean up functions of MessageFilter are paired with their initialization functions This is an attempt to address http://crrev.com/2469123003/#msg28. MessageFilter::OnFilterRemoved and OnChannelClosing are now called even when the channel is closed before it completes its initialization. In that case, these functions are not paired to OnFilterAdded and OnChannelOpened. This CL makes OnFilterAdded and OnChannelConnected right before pending filters are removed, so that they are paired with their clean up functions. BUG= ==========
On 2016/11/08 17:33:04, Ken Rockot wrote: > The semantics of all these different events seem like they would easily confuse > a reader. > > I wonder if we could just get away with calling OnFilterAdded immediately before > OnFilterRemoved etc when clearing pending filters. SG. Updated to call OnChannelConnected and OnFilterAdded right before pending_filters removal.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm https://codereview.chromium.org/2488753002/diff/20001/ipc/ipc_channel_proxy.cc File ipc/ipc_channel_proxy.cc (right): https://codereview.chromium.org/2488753002/diff/20001/ipc/ipc_channel_proxy.c... ipc/ipc_channel_proxy.cc:215: filter->OnFilterAdded(nullptr); nit: I think it's worth adding a comment here to clarify why all of these are called.
https://codereview.chromium.org/2488753002/diff/20001/ipc/ipc_channel_proxy.cc File ipc/ipc_channel_proxy.cc (right): https://codereview.chromium.org/2488753002/diff/20001/ipc/ipc_channel_proxy.c... ipc/ipc_channel_proxy.cc:215: filter->OnFilterAdded(nullptr); On 2016/11/10 00:49:55, Ken Rockot wrote: > nit: I think it's worth adding a comment here to clarify why all of these are > called. Done.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org Link to the patchset: https://codereview.chromium.org/2488753002/#ps40001 (title: "+comment")
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 ========== Ensure clean up functions of MessageFilter are paired with their initialization functions This is an attempt to address http://crrev.com/2469123003/#msg28. MessageFilter::OnFilterRemoved and OnChannelClosing are now called even when the channel is closed before it completes its initialization. In that case, these functions are not paired to OnFilterAdded and OnChannelOpened. This CL makes OnFilterAdded and OnChannelConnected right before pending filters are removed, so that they are paired with their clean up functions. BUG= ========== to ========== Ensure clean up functions of MessageFilter are paired with their initialization functions This is an attempt to address http://crrev.com/2469123003/#msg28. MessageFilter::OnFilterRemoved and OnChannelClosing are now called even when the channel is closed before it completes its initialization. In that case, these functions are not paired to OnFilterAdded and OnChannelOpened. This CL makes OnFilterAdded and OnChannelConnected right before pending filters are removed, so that they are paired with their clean up functions. BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Ensure clean up functions of MessageFilter are paired with their initialization functions This is an attempt to address http://crrev.com/2469123003/#msg28. MessageFilter::OnFilterRemoved and OnChannelClosing are now called even when the channel is closed before it completes its initialization. In that case, these functions are not paired to OnFilterAdded and OnChannelOpened. This CL makes OnFilterAdded and OnChannelConnected right before pending filters are removed, so that they are paired with their clean up functions. BUG= ========== to ========== Ensure clean up functions of MessageFilter are paired with their initialization functions This is an attempt to address http://crrev.com/2469123003/#msg28. MessageFilter::OnFilterRemoved and OnChannelClosing are now called even when the channel is closed before it completes its initialization. In that case, these functions are not paired to OnFilterAdded and OnChannelOpened. This CL makes OnFilterAdded and OnChannelConnected right before pending filters are removed, so that they are paired with their clean up functions. BUG= Committed: https://crrev.com/8941d772a304208a5ad4fb47dfa3c669fb3b5d11 Cr-Commit-Position: refs/heads/master@{#431199} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8941d772a304208a5ad4fb47dfa3c669fb3b5d11 Cr-Commit-Position: refs/heads/master@{#431199}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2497843002/ by tzik@chromium.org. The reason for reverting is: This causes crashes on SyncMessageFilter::OnFilterAdded. BUG=664511. |
