Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2795413002:
Makes ServiceManagerConnectionImpl queue requests (Closed)
DescriptionMakes ServiceManagerConnectionImpl queue requests
Without this it's possible for CreateService() to be called before the
request handler.
BUG=707321
TEST=none
R=ben@chromium.org
Review-Url: https://codereview.chromium.org/2795413002
Cr-Commit-Position: refs/heads/master@{#461925}
Committed: https://chromium.googlesource.com/chromium/src/+/26b9dcec30000075deb5bf3455e99961c4db8b17
Patch Set 1 #Patch Set 2 : merge #Messages
Total messages: 19 (9 generated)
Description was changed from ========== revert revert done fix R=ben@chromium.org BUG= ========== to ========== Makes ServiceManagerConnectionImpl queue requests Without this it's possible for CreateService() to be called before the request handler. BUG=707321 TEST=none R=ben@chromium.org ==========
As this is blocking usage on a device I'll work on a test separately.
The CQ bit was checked by sky@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
lgtm
The CQ bit was checked by sky@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ben@chromium.org Link to the patchset: https://codereview.chromium.org/2795413002/#ps20001 (title: "merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/04 at 23:12:38, commit-bot wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... I don't think we really want to do this, but lgtm for now if it unblocks stuff. Does not seem like it should be difficult to just register services earlier instead?
On 2017/04/04 23:16:45, Ken Rockot (OOO til Apr 10) wrote: > On 2017/04/04 at 23:12:38, commit-bot wrote: > > CQ is trying da patch. Follow status at > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > I don't think we really want to do this, but lgtm for now if it unblocks stuff. > Does not seem like it should be difficult to just register services earlier > instead? Can you outline why that is? Getting registration to work in a particular order seems very difficult and extremely fragile when you have multiple processes.
On 2017/04/05 00:00:22, sky wrote: > On 2017/04/04 23:16:45, Ken Rockot (OOO til Apr 10) wrote: > > On 2017/04/04 at 23:12:38, commit-bot wrote: > > > CQ is trying da patch. Follow status at > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > I don't think we really want to do this, but lgtm for now if it unblocks > stuff. > > Does not seem like it should be difficult to just register services earlier > > instead? > > Can you outline why that is? Getting registration to work in a particular order > seems very difficult and extremely fragile when you have multiple processes. I'll look into when Profile is registering it's services. If it's done before Start, then it seems this shouldn't necessary.
On 2017/04/05 00:09:36, sky wrote: > On 2017/04/05 00:00:22, sky wrote: > > On 2017/04/04 23:16:45, Ken Rockot (OOO til Apr 10) wrote: > > > On 2017/04/04 at 23:12:38, commit-bot wrote: > > > > CQ is trying da patch. Follow status at > > > > > > > > > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... > > > > > > I don't think we really want to do this, but lgtm for now if it unblocks > > stuff. > > > Does not seem like it should be difficult to just register services earlier > > > instead? > > > > Can you outline why that is? Getting registration to work in a particular > order > > seems very difficult and extremely fragile when you have multiple processes. > > I'll look into when Profile is registering it's services. If it's done before > Start, then it seems this shouldn't necessary. It's in ProfileImpl::RegisterInProcessServices.
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491347512278600,
"parent_rev": "486716780e8483c70c50c1fc50ecca4112efb5aa", "commit_rev":
"26b9dcec30000075deb5bf3455e99961c4db8b17"}
Message was sent while issue was closed.
Description was changed from ========== Makes ServiceManagerConnectionImpl queue requests Without this it's possible for CreateService() to be called before the request handler. BUG=707321 TEST=none R=ben@chromium.org ========== to ========== Makes ServiceManagerConnectionImpl queue requests Without this it's possible for CreateService() to be called before the request handler. BUG=707321 TEST=none R=ben@chromium.org Review-Url: https://codereview.chromium.org/2795413002 Cr-Commit-Position: refs/heads/master@{#461925} Committed: https://chromium.googlesource.com/chromium/src/+/26b9dcec30000075deb5bf3455e9... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/26b9dcec30000075deb5bf3455e9...
Message was sent while issue was closed.
On 2017/04/05 00:59:05, commit-bot: I haz the power wrote: > Committed patchset #2 (id:20001) as > https://chromium.googlesource.com/chromium/src/+/26b9dcec30000075deb5bf3455e9... Ok, this is definitely the wrong fix. I'll revert it as soon as figure out the right fix. Will move discussion to email to figure out the right fix.
Message was sent while issue was closed.
On 2017/04/06 02:18:20, sky wrote: > On 2017/04/05 00:59:05, commit-bot: I haz the power wrote: > > Committed patchset #2 (id:20001) as > > > https://chromium.googlesource.com/chromium/src/+/26b9dcec30000075deb5bf3455e9... > > Ok, this is definitely the wrong fix. I'll revert it as soon as figure out the > right fix. Will move discussion to email to figure out the right fix. Jon reverted this as part of the real fix. Which is here: https://codereview.chromium.org/2802093005/ . |
