Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2255493004:
Synchronously disable ConnectionFilterImpl when its RPHI cleans up (Closed)
|
Created:
4 years, 4 months ago by Ken Rockot(use gerrit already) Modified:
4 years, 4 months ago Reviewers:
Ben Goodger (Google) CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, jam, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@fix-connection-filter-race Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSynchronously disable ConnectionFilterImpl when its RPHI cleans up
Prevents ConnectionFilterImpl from servicing any new interface
requests after RPHI::Cleanup is run.
Also adds an AddUIThreadInterface helper to RPHI which registers
a UI thread interface binder that will only run as long as the
render process for which it was added is still alive (i.e. until
the next call to Cleanup).
BUG=
R=ben@chromium.org
Committed: https://crrev.com/81dcc5ed8f118ef9838fc9e17486a7c080af8794
Cr-Commit-Position: refs/heads/master@{#412873}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #
Messages
Total messages: 26 (19 generated)
The CQ bit was checked by rockot@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.
The CQ bit was checked by rockot@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 checked by rockot@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 ========== Synchronously remove ConnectionFilters from MojoShellConnectionImpl Changes RemoveConnectionFilter to synchronously remove the filter while still ensuring it's deleted on the IO thread if necessary. BUG= R=ben@chromium.org ========== to ========== Prevents ConnectionFilterImpl from servicing any new interface requests after RPHI::Cleanup is run. BUG= R=ben@chromium.org ==========
Description was changed from ========== Prevents ConnectionFilterImpl from servicing any new interface requests after RPHI::Cleanup is run. BUG= R=ben@chromium.org ========== to ========== Synchronously disable ConnectionFilterImpl when its RPHI cleans up Prevents ConnectionFilterImpl from servicing any new interface requests after RPHI::Cleanup is run. BUG= R=ben@chromium.org ==========
This does what I think Darin wanted, but after thinking about it some more, it seems like the wrong thing to do. We need to limit the (callable) lifetime of the registered callbacks themselves. Since they're posted to the UI MessageLoop by the ConnectionFilterImpl from the IO thread, they may be posted before Cleanup runs, but run after.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Description was changed from ========== Synchronously disable ConnectionFilterImpl when its RPHI cleans up Prevents ConnectionFilterImpl from servicing any new interface requests after RPHI::Cleanup is run. BUG= R=ben@chromium.org ========== to ========== Synchronously disable ConnectionFilterImpl when its RPHI cleans up Prevents ConnectionFilterImpl from servicing any new interface requests after RPHI::Cleanup is run. Also adds an AddUIThreadInterface helper to RPHI which registers a UI thread interface binder that will only run as long as the render process for which it was added is still alive (i.e. until the next call to Cleanup). BUG= R=ben@chromium.org ==========
I added some more magic to limit UI thread interface binder lifetime so they're never called after Cleanup. On one hand, adds ugly template code to the header; on the other hand, easy to use and does what we want. Take another look?
The CQ bit was checked by rockot@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.
fun lgtm
The CQ bit was checked by rockot@chromium.org
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.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Synchronously disable ConnectionFilterImpl when its RPHI cleans up Prevents ConnectionFilterImpl from servicing any new interface requests after RPHI::Cleanup is run. Also adds an AddUIThreadInterface helper to RPHI which registers a UI thread interface binder that will only run as long as the render process for which it was added is still alive (i.e. until the next call to Cleanup). BUG= R=ben@chromium.org ========== to ========== Synchronously disable ConnectionFilterImpl when its RPHI cleans up Prevents ConnectionFilterImpl from servicing any new interface requests after RPHI::Cleanup is run. Also adds an AddUIThreadInterface helper to RPHI which registers a UI thread interface binder that will only run as long as the render process for which it was added is still alive (i.e. until the next call to Cleanup). BUG= R=ben@chromium.org Committed: https://crrev.com/81dcc5ed8f118ef9838fc9e17486a7c080af8794 Cr-Commit-Position: refs/heads/master@{#412873} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/81dcc5ed8f118ef9838fc9e17486a7c080af8794 Cr-Commit-Position: refs/heads/master@{#412873} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
