Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(18)

Issue 2859343003: Enable overriding interface binders for any services running in current process. (Closed)

Created:
3 years, 7 months ago by leonhsl(Using Gerrit)
Modified:
3 years, 7 months ago
CC:
chromium-reviews, darin-cc_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Enable overriding interface binders for any services running in current process. This CL - holds a 'service name <--> binder registry' map inside service manager client library, at run time it acts as a global variable in any process linking with service manager client library. - lets ServiceContext::OnBindInterface() use the global map to intercept requests before dispatching them to target Services, that is to say, such interception may happen for any Services running in current process. - exposes {SetGlobalBinder,ClearGlobalBinders}ForTesting to set/clear binders in the global map. - adds a VibrationTest browser test to show the usage of the new infra described above. BUG=717377, 717378 TEST=content_browsertests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2859343003 Cr-Commit-Position: refs/heads/master@{#471216} Committed: https://chromium.googlesource.com/chromium/src/+/d25e37df0f08a754fbc11041451a5ce7ca7d043d

Patch Set 1 #

Total comments: 13

Patch Set 2 : Address comments from rockot@ #

Patch Set 3 : Override by service identity --> service name #

Total comments: 8

Patch Set 4 : Modify GenericInterfaceBinder to use full signature callback #

Total comments: 2

Patch Set 5 : Address nit #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+222 lines, -11 lines) Patch
M content/browser/frame_host/render_frame_host_impl.cc View 1 1 chunk +6 lines, -3 lines 0 comments Download
A content/browser/vibration_browsertest.cc View 1 2 3 4 1 chunk +90 lines, -0 lines 0 comments Download
M content/test/BUILD.gn View 2 chunks +3 lines, -0 lines 0 comments Download
M services/service_manager/public/cpp/binder_registry.h View 1 2 chunks +8 lines, -3 lines 0 comments Download
M services/service_manager/public/cpp/binder_registry.cc View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download
M services/service_manager/public/cpp/interface_binder.h View 1 2 3 2 chunks +8 lines, -1 line 0 comments Download
M services/service_manager/public/cpp/interface_binder.cc View 1 2 3 2 chunks +27 lines, -4 lines 0 comments Download
M services/service_manager/public/cpp/service_context.h View 1 2 3 2 chunks +15 lines, -0 lines 0 comments Download
M services/service_manager/public/cpp/service_context.cc View 1 2 3 3 chunks +57 lines, -0 lines 1 comment Download

Messages

Total messages: 72 (41 generated)
leonhsl(Using Gerrit)
This CL just adds the basic support into service manager client library, and adds a ...
3 years, 7 months ago (2017-05-05 11:46:57 UTC) #12
blundell
This looks great to me. I'll defer to Ken on the exact placement of the ...
3 years, 7 months ago (2017-05-05 12:56:07 UTC) #13
leonhsl(Using Gerrit)
Friendly ping Ken:) Thanks. https://codereview.chromium.org/2859343003/diff/20001/services/service_manager/public/cpp/service_context.h File services/service_manager/public/cpp/service_context.h (right): https://codereview.chromium.org/2859343003/diff/20001/services/service_manager/public/cpp/service_context.h#newcode63 services/service_manager/public/cpp/service_context.h:63: static void ClearGlobalBinders(const Identity& service); ...
3 years, 7 months ago (2017-05-09 00:08:55 UTC) #14
Ken Rockot(use gerrit already)
Very sorry for the delay. I had somehow flipped the "already reviewed" bit in my ...
3 years, 7 months ago (2017-05-09 15:29:34 UTC) #15
Ken Rockot(use gerrit already)
On 2017/05/09 at 15:29:34, Ken Rockot wrote: > Very sorry for the delay. I had ...
3 years, 7 months ago (2017-05-09 15:31:10 UTC) #16
leonhsl(Using Gerrit)
Thanks Ken a lot for review! https://codereview.chromium.org/2859343003/diff/20001/services/service_manager/public/cpp/service_context.h File services/service_manager/public/cpp/service_context.h (right): https://codereview.chromium.org/2859343003/diff/20001/services/service_manager/public/cpp/service_context.h#newcode45 services/service_manager/public/cpp/service_context.h:45: class TestApi { ...
3 years, 7 months ago (2017-05-09 23:31:54 UTC) #17
blundell
https://codereview.chromium.org/2859343003/diff/20001/services/service_manager/public/cpp/service_context.h File services/service_manager/public/cpp/service_context.h (right): https://codereview.chromium.org/2859343003/diff/20001/services/service_manager/public/cpp/service_context.h#newcode56 services/service_manager/public/cpp/service_context.h:56: const Identity& service, On 2017/05/09 23:31:53, leonhsl wrote: > ...
3 years, 7 months ago (2017-05-10 08:54:40 UTC) #26
leonhsl(Using Gerrit)
Ps#2 addressed all comments except the work replacing key type from service identity to service ...
3 years, 7 months ago (2017-05-10 09:57:11 UTC) #27
leonhsl(Using Gerrit)
Uploaded ps#3 to override binders by service name, PTAnL, Thanks. https://codereview.chromium.org/2859343003/diff/20001/services/service_manager/public/cpp/service_context.h File services/service_manager/public/cpp/service_context.h (right): https://codereview.chromium.org/2859343003/diff/20001/services/service_manager/public/cpp/service_context.h#newcode56 ...
3 years, 7 months ago (2017-05-11 02:21:47 UTC) #33
Ken Rockot(use gerrit already)
LGTM with nits and one more substantial request (see comments) https://codereview.chromium.org/2859343003/diff/70001/services/service_manager/public/cpp/interface_binder.h File services/service_manager/public/cpp/interface_binder.h (right): https://codereview.chromium.org/2859343003/diff/70001/services/service_manager/public/cpp/interface_binder.h#newcode92 ...
3 years, 7 months ago (2017-05-11 02:32:08 UTC) #34
leonhsl(Using Gerrit)
Uploaded ps#4 to address comments, PTAnL, Thanks! https://codereview.chromium.org/2859343003/diff/70001/services/service_manager/public/cpp/interface_binder.h File services/service_manager/public/cpp/interface_binder.h (right): https://codereview.chromium.org/2859343003/diff/70001/services/service_manager/public/cpp/interface_binder.h#newcode92 services/service_manager/public/cpp/interface_binder.h:92: class FullCallbackBinder ...
3 years, 7 months ago (2017-05-11 03:10:05 UTC) #40
jam
what do you want me to review? if content rs, then lgtm
3 years, 7 months ago (2017-05-11 14:56:03 UTC) #47
Ken Rockot(use gerrit already)
Still LGTM https://codereview.chromium.org/2859343003/diff/110001/content/browser/vibration_browsertest.cc File content/browser/vibration_browsertest.cc (right): https://codereview.chromium.org/2859343003/diff/110001/content/browser/vibration_browsertest.cc#newcode66 content/browser/vibration_browsertest.cc:66: // Quit the run loop. nit: Unnecessary ...
3 years, 7 months ago (2017-05-11 15:38:16 UTC) #48
leonhsl(Using Gerrit)
Thanks all for kindly review! Send ps#5 to CQ now. https://codereview.chromium.org/2859343003/diff/110001/content/browser/vibration_browsertest.cc File content/browser/vibration_browsertest.cc (right): https://codereview.chromium.org/2859343003/diff/110001/content/browser/vibration_browsertest.cc#newcode66 ...
3 years, 7 months ago (2017-05-12 02:03:00 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2859343003/130001
3 years, 7 months ago (2017-05-12 02:04:11 UTC) #52
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/451772)
3 years, 7 months ago (2017-05-12 03:30:56 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2859343003/130001
3 years, 7 months ago (2017-05-12 03:32:48 UTC) #56
commit-bot: I haz the power
Committed patchset #5 (id:130001) as https://chromium.googlesource.com/chromium/src/+/d25e37df0f08a754fbc11041451a5ce7ca7d043d
3 years, 7 months ago (2017-05-12 04:16:33 UTC) #59
blundell
This is great, thanks! This functionality is really important :).
3 years, 7 months ago (2017-05-12 06:56:18 UTC) #60
henrika (OOO until Aug 14)
A revert of this CL (patchset #5 id:130001) has been created in https://codereview.chromium.org/2882613002/ by henrika@chromium.org. ...
3 years, 7 months ago (2017-05-12 07:59:54 UTC) #61
leonhsl(Using Gerrit)
On 2017/05/12 07:59:54, henrika wrote: > A revert of this CL (patchset #5 id:130001) has ...
3 years, 7 months ago (2017-05-12 10:14:43 UTC) #62
blundell
On 2017/05/12 10:14:43, leonhsl wrote: > On 2017/05/12 07:59:54, henrika wrote: > > A revert ...
3 years, 7 months ago (2017-05-15 08:11:50 UTC) #63
leonhsl(Using Gerrit)
On 2017/05/15 08:11:50, blundell wrote: > On 2017/05/12 10:14:43, leonhsl wrote: > > On 2017/05/12 ...
3 years, 7 months ago (2017-05-15 09:51:33 UTC) #64
leonhsl(Using Gerrit)
On 2017/05/15 09:51:33, leonhsl wrote: > On 2017/05/15 08:11:50, blundell wrote: > > On 2017/05/12 ...
3 years, 7 months ago (2017-05-15 09:53:42 UTC) #65
leonhsl(Using Gerrit)
As we can't reproduce the failure and it's hard to identify the root cause, no ...
3 years, 7 months ago (2017-05-16 09:54:53 UTC) #66
blundell
Oof, I spent a bunch of time debugging this today and got to the bottom ...
3 years, 7 months ago (2017-05-16 13:05:19 UTC) #67
Ken Rockot(use gerrit already)
There are, but to it's definitely the right thing to do. Please let me take ...
3 years, 7 months ago (2017-05-16 14:27:10 UTC) #68
blundell
https://codereview.chromium.org/2859343003/diff/130001/services/service_manager/public/cpp/service_context.cc File services/service_manager/public/cpp/service_context.cc (right): https://codereview.chromium.org/2859343003/diff/130001/services/service_manager/public/cpp/service_context.cc#newcode22 services/service_manager/public/cpp/service_context.cc:22: base::LazyInstance<std::unique_ptr<ServiceNameToBinderRegistryMap>>::Leaky As an aside, I don't see that the ...
3 years, 7 months ago (2017-05-16 15:00:14 UTC) #69
Ken Rockot(use gerrit already)
I've made my first attempt at componentizing the service manager public libraries. Let's see if ...
3 years, 7 months ago (2017-05-16 15:31:37 UTC) #70
leonhsl(Using Gerrit)
Thank you all very much for your time and analysis! Sorry we should have avoided ...
3 years, 7 months ago (2017-05-17 03:39:50 UTC) #71
blundell
3 years, 7 months ago (2017-05-17 08:57:43 UTC) #72
Message was sent while issue was closed.
On 2017/05/17 03:39:50, leonhsl wrote:
> Thank you all very much for your time and analysis! Sorry we should have
avoided
> such a problem... if I had thought more about linking for global variables or
if
> I had been using component build. Thanks again!

You don't have to apologize for anything here, this was a subtle issue and we
apparently don't have any trybots that run tests in the component build, which
is what allowed the change to land in the first place.

It *is* a good idea to use component build locally though:

(1) It's significantly faster to link (that's it's raison d'etre)
(2) It ensures that you'll catch compile/runtime issues that crop up only in
component build, whereas AFAIK there are no such issues that crop up only in
non-component build

Powered by Google App Engine
This is Rietveld 408576698