|
|
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. |
DescriptionEnable 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
Messages
Total messages: 72 (41 generated)
The CQ bit was checked by leon.han@intel.com 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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by leon.han@intel.com 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...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Enable overriding interface binders for any services running in current process. BUG=717377 TEST=content_browsertests ========== to ========== Enable overriding interface binders for any services running in current process. This CL - holds a 'service identity <--> 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 ServiceContext::TestApi interface to set/clear binders to the global map. - adds a VibrationTest browser test to show the usage of the new infra described above. BUG=717377 TEST=content_browsertests ==========
leon.han@intel.com changed reviewers: + blundell@chromium.org, jam@chromium.org, rockot@chromium.org
This CL just adds the basic support into service manager client library, and adds a content browser test VibrationTest as an example, because Device Service runs also in browser process, we can directly call ServiceContext::TestApi to override its interface binders during setting up browser test. Further, according from Ken's suggestions, to override interfaces of some Services running in processes other than browser, we can expose an appropriate (e.g.) RegisterTestServiceInterfaces method on ContentRenderer/Gpu/UtilityClient, and let content_shell implementation leverage the ServiceContext::TestApi of this CL to do actual work. WDYT? Please help to correct my understanding above if it makes non-sense.. Thanks!
This looks great to me. I'll defer to Ken on the exact placement of the new TestAPI class. https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... File services/service_manager/public/cpp/service_context.h (right): https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... services/service_manager/public/cpp/service_context.h:63: static void ClearGlobalBinders(const Identity& service); nit: Maybe ClearGlobalBindersForService()?
Friendly ping Ken:) Thanks. https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... File services/service_manager/public/cpp/service_context.h (right): https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... services/service_manager/public/cpp/service_context.h:63: static void ClearGlobalBinders(const Identity& service); On 2017/05/05 12:56:07, blundell wrote: > nit: Maybe ClearGlobalBindersForService()? Acknowledged.
Very sorry for the delay. I had somehow flipped the "already reviewed" bit in my head. General approach looks great, just some questions/comments https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... File services/service_manager/public/cpp/service_context.h (right): https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... services/service_manager/public/cpp/service_context.h:45: class TestApi { nit: I'm not really a fan of these TestApi subclasses. I'd rather we just introduce a minimal set of testing methods directly on ServiceContext, e.g.: ServiceContext::SetGlobalBinderForTesting() ServiceContext::ClearGlobalBindersForTesting() https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... services/service_manager/public/cpp/service_context.h:56: const Identity& service, I wonder if we really want a specific identity to override against, or if we want to override all requests for |interface_name| to all instances of (service.name()), in which case we should take a std::string (name) instead of a full Identity. I suspect that's what we want. https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... services/service_manager/public/cpp/service_context.h:58: const base::Callback<void(mojo::ScopedMessagePipeHandle)>& binder, In keeping with existing binder callbacks today, this should be a base::Callback<void(const service_manager::BindSourceInfo& source_info, const std::string& interface_name, mojo::ScopedMessagePipeHandle pipe)>; In fact you could update the (currently unused) BinderRegistry::Binder alias to that signature and use it here (and possibly elsewhere). Even if most uses of the test API will ignore source_info and interface_name, this it a bit more flexibility.
On 2017/05/09 at 15:29:34, Ken Rockot wrote: > Very sorry for the delay. I had somehow flipped the "already reviewed" bit in my head. > > General approach looks great, just some questions/comments > > https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... > File services/service_manager/public/cpp/service_context.h (right): > > https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... > services/service_manager/public/cpp/service_context.h:45: class TestApi { > nit: I'm not really a fan of these TestApi subclasses. I'd rather we just introduce a minimal set of testing methods directly on ServiceContext, e.g.: > > ServiceContext::SetGlobalBinderForTesting() > ServiceContext::ClearGlobalBindersForTesting() > > https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... > services/service_manager/public/cpp/service_context.h:56: const Identity& service, > I wonder if we really want a specific identity to override against, or if we want to override all requests for |interface_name| to all instances of (service.name()), in which case we should take a std::string (name) instead of a full Identity. I suspect that's what we want. > > https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... > services/service_manager/public/cpp/service_context.h:58: const base::Callback<void(mojo::ScopedMessagePipeHandle)>& binder, > In keeping with existing binder callbacks today, this should be a > > base::Callback<void(const service_manager::BindSourceInfo& source_info, > const std::string& interface_name, > mojo::ScopedMessagePipeHandle pipe)>; > > In fact you could update the (currently unused) BinderRegistry::Binder alias to that signature and use it here (and possibly elsewhere). Turns out it is actually used in one place (RenderFrameHostImpl) today, but that seems easy enough to adapt. > > Even if most uses of the test API will ignore source_info and interface_name, this it a bit more flexibility.
Thanks Ken a lot for review! https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... File services/service_manager/public/cpp/service_context.h (right): https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... services/service_manager/public/cpp/service_context.h:45: class TestApi { On 2017/05/09 15:29:34, Ken Rockot wrote: > nit: I'm not really a fan of these TestApi subclasses. I'd rather we just > introduce a minimal set of testing methods directly on ServiceContext, e.g.: > > ServiceContext::SetGlobalBinderForTesting() > ServiceContext::ClearGlobalBindersForTesting() Acknowledged. https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... services/service_manager/public/cpp/service_context.h:56: const Identity& service, On 2017/05/09 15:29:34, Ken Rockot wrote: > I wonder if we really want a specific identity to override against, or if we > want to override all requests for |interface_name| to all instances of > (service.name()), in which case we should take a std::string (name) instead of a > full Identity. I suspect that's what we want. Yeah I was afraid in future some test cases may want to override against a specific instance of service, though I have no clear image about such test cases:) so I chose to make this API more flexible considering that it will be a public API from service manager client library. User can still override for all instances of service "Foo" with an Identity("Foo"). Or we're sure that definitely there have no such needs? https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... services/service_manager/public/cpp/service_context.h:58: const base::Callback<void(mojo::ScopedMessagePipeHandle)>& binder, On 2017/05/09 15:29:34, Ken Rockot wrote: > In keeping with existing binder callbacks today, this should be a > > base::Callback<void(const service_manager::BindSourceInfo& source_info, > const std::string& interface_name, > mojo::ScopedMessagePipeHandle pipe)>; > > In fact you could update the (currently unused) BinderRegistry::Binder alias to > that signature and use it here (and possibly elsewhere). > > Even if most uses of the test API will ignore source_info and interface_name, > this it a bit more flexibility. Acknowledged.
Description was changed from ========== Enable overriding interface binders for any services running in current process. This CL - holds a 'service identity <--> 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 ServiceContext::TestApi interface to set/clear binders to the global map. - adds a VibrationTest browser test to show the usage of the new infra described above. BUG=717377 TEST=content_browsertests ========== to ========== Enable overriding interface binders for any services running in current process. This CL - holds a 'service identity <--> 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 ServiceContext::TestApi interface to set/clear binders to the global map. - adds a VibrationTest browser test to show the usage of the new infra described above. BUG=717377 TEST=content_browsertests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by leon.han@intel.com 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 leon.han@intel.com 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...
Patchset #2 (id:30001) has been deleted
Description was changed from ========== Enable overriding interface binders for any services running in current process. This CL - holds a 'service identity <--> 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 ServiceContext::TestApi interface to set/clear binders to the global map. - adds a VibrationTest browser test to show the usage of the new infra described above. BUG=717377 TEST=content_browsertests CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Enable overriding interface binders for any services running in current process. This CL - holds a 'service identity <--> 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 ServiceContext::TestApi interface to set/clear binders to 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 ==========
Description was changed from ========== Enable overriding interface binders for any services running in current process. This CL - holds a 'service identity <--> 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 ServiceContext::TestApi interface to set/clear binders to 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 ========== to ========== Enable overriding interface binders for any services running in current process. This CL - holds a 'service identity <--> 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 ==========
https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... File services/service_manager/public/cpp/service_context.h (right): https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... services/service_manager/public/cpp/service_context.h:56: const Identity& service, On 2017/05/09 23:31:53, leonhsl wrote: > On 2017/05/09 15:29:34, Ken Rockot wrote: > > I wonder if we really want a specific identity to override against, or if we > > want to override all requests for |interface_name| to all instances of > > (service.name()), in which case we should take a std::string (name) instead of > a > > full Identity. I suspect that's what we want. > > Yeah I was afraid in future some test cases may want to override against a > specific instance of service, though I have no clear image about such test > cases:) so I chose to make this API more flexible considering that it will be a > public API from service manager client library. User can still override for all > instances of service "Foo" with an Identity("Foo"). > Or we're sure that definitely there have no such needs? Let's leave it simple for now and it will be easy enough to introduce the flexibility later if needed.
Ps#2 addressed all comments except the work replacing key type from service identity to service name, I might have no enough time to complete it today, PTAnL at other parts firstly :) https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... File services/service_manager/public/cpp/service_context.h (right): https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... services/service_manager/public/cpp/service_context.h:45: class TestApi { On 2017/05/09 15:29:34, Ken Rockot wrote: > nit: I'm not really a fan of these TestApi subclasses. I'd rather we just > introduce a minimal set of testing methods directly on ServiceContext, e.g.: > > ServiceContext::SetGlobalBinderForTesting() > ServiceContext::ClearGlobalBindersForTesting() Done. https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... services/service_manager/public/cpp/service_context.h:56: const Identity& service, On 2017/05/10 08:54:39, blundell wrote: > On 2017/05/09 23:31:53, leonhsl wrote: > > On 2017/05/09 15:29:34, Ken Rockot wrote: > > > I wonder if we really want a specific identity to override against, or if we > > > want to override all requests for |interface_name| to all instances of > > > (service.name()), in which case we should take a std::string (name) instead > of > > a > > > full Identity. I suspect that's what we want. > > > > Yeah I was afraid in future some test cases may want to override against a > > specific instance of service, though I have no clear image about such test > > cases:) so I chose to make this API more flexible considering that it will be > a > > public API from service manager client library. User can still override for > all > > instances of service "Foo" with an Identity("Foo"). > > Or we're sure that definitely there have no such needs? > > Let's leave it simple for now and it will be easy enough to introduce the > flexibility later if needed. I see, will update CL soon. https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... services/service_manager/public/cpp/service_context.h:58: const base::Callback<void(mojo::ScopedMessagePipeHandle)>& binder, On 2017/05/09 15:29:34, Ken Rockot wrote: > In keeping with existing binder callbacks today, this should be a > > base::Callback<void(const service_manager::BindSourceInfo& source_info, > const std::string& interface_name, > mojo::ScopedMessagePipeHandle pipe)>; > > In fact you could update the (currently unused) BinderRegistry::Binder alias to > that signature and use it here (and possibly elsewhere). > > Even if most uses of the test API will ignore source_info and interface_name, > this it a bit more flexibility. Done and Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by leon.han@intel.com 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 ========== Enable overriding interface binders for any services running in current process. This CL - holds a 'service identity <--> 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 ========== to ========== 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 ==========
Uploaded ps#3 to override binders by service name, PTAnL, Thanks. https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... File services/service_manager/public/cpp/service_context.h (right): https://codereview.chromium.org/2859343003/diff/20001/services/service_manage... services/service_manager/public/cpp/service_context.h:56: const Identity& service, On 2017/05/10 09:57:11, leonhsl wrote: > On 2017/05/10 08:54:39, blundell wrote: > > On 2017/05/09 23:31:53, leonhsl wrote: > > > On 2017/05/09 15:29:34, Ken Rockot wrote: > > > > I wonder if we really want a specific identity to override against, or if > we > > > > want to override all requests for |interface_name| to all instances of > > > > (service.name()), in which case we should take a std::string (name) > instead > > of > > > a > > > > full Identity. I suspect that's what we want. > > > > > > Yeah I was afraid in future some test cases may want to override against a > > > specific instance of service, though I have no clear image about such test > > > cases:) so I chose to make this API more flexible considering that it will > be > > a > > > public API from service manager client library. User can still override for > > all > > > instances of service "Foo" with an Identity("Foo"). > > > Or we're sure that definitely there have no such needs? > > > > Let's leave it simple for now and it will be easy enough to introduce the > > flexibility later if needed. > > I see, will update CL soon. Done.
LGTM with nits and one more substantial request (see comments) https://codereview.chromium.org/2859343003/diff/70001/services/service_manage... File services/service_manager/public/cpp/interface_binder.h (right): https://codereview.chromium.org/2859343003/diff/70001/services/service_manage... services/service_manager/public/cpp/interface_binder.h:92: class FullCallbackBinder : public InterfaceBinder { Could you please just modify GenericCallbackBinder to instead use the full callback signature? It can have a constructor which still takes a base::Callback<void(mojo::ScopedMessagePipeHandle)>, and we can just adapt that to the full signature by binding a private helper method. https://codereview.chromium.org/2859343003/diff/70001/services/service_manage... File services/service_manager/public/cpp/service_context.cc (right): https://codereview.chromium.org/2859343003/diff/70001/services/service_manage... services/service_manager/public/cpp/service_context.cc:63: if (!g_overridden_binder_registries.Get()) { nit: no {} necessary https://codereview.chromium.org/2859343003/diff/70001/services/service_manage... File services/service_manager/public/cpp/service_context.h (right): https://codereview.chromium.org/2859343003/diff/70001/services/service_manage... services/service_manager/public/cpp/service_context.h:109: // This is a process-wide override, means that |binder| can intercept requests nit: means=meaning https://codereview.chromium.org/2859343003/diff/70001/services/service_manage... services/service_manager/public/cpp/service_context.h:119: // SetGlobalBinderForTesting() before. nit: can remove "before"
The CQ bit was checked by leon.han@intel.com 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 leon.han@intel.com to run a CQ dry run
Patchset #4 (id:90001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Uploaded ps#4 to address comments, PTAnL, Thanks! https://codereview.chromium.org/2859343003/diff/70001/services/service_manage... File services/service_manager/public/cpp/interface_binder.h (right): https://codereview.chromium.org/2859343003/diff/70001/services/service_manage... services/service_manager/public/cpp/interface_binder.h:92: class FullCallbackBinder : public InterfaceBinder { On 2017/05/11 02:32:08, Ken Rockot wrote: > Could you please just modify GenericCallbackBinder to instead use the full > callback signature? It can have a constructor which still takes a > base::Callback<void(mojo::ScopedMessagePipeHandle)>, and we can just adapt that > to the full signature by binding a private helper method. Done. Thank you very much! I should have done such way. https://codereview.chromium.org/2859343003/diff/70001/services/service_manage... File services/service_manager/public/cpp/service_context.cc (right): https://codereview.chromium.org/2859343003/diff/70001/services/service_manage... services/service_manager/public/cpp/service_context.cc:63: if (!g_overridden_binder_registries.Get()) { On 2017/05/11 02:32:08, Ken Rockot wrote: > nit: no {} necessary Done. https://codereview.chromium.org/2859343003/diff/70001/services/service_manage... File services/service_manager/public/cpp/service_context.h (right): https://codereview.chromium.org/2859343003/diff/70001/services/service_manage... services/service_manager/public/cpp/service_context.h:109: // This is a process-wide override, means that |binder| can intercept requests On 2017/05/11 02:32:08, Ken Rockot wrote: > nit: means=meaning Done. https://codereview.chromium.org/2859343003/diff/70001/services/service_manage... services/service_manager/public/cpp/service_context.h:119: // SetGlobalBinderForTesting() before. On 2017/05/11 02:32:08, Ken Rockot wrote: > nit: can remove "before" Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by leon.han@intel.com 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.
what do you want me to review? if content rs, then lgtm
Still LGTM https://codereview.chromium.org/2859343003/diff/110001/content/browser/vibrat... File content/browser/vibration_browsertest.cc (right): https://codereview.chromium.org/2859343003/diff/110001/content/browser/vibrat... content/browser/vibration_browsertest.cc:66: // Quit the run loop. nit: Unnecessary comment, and I think too specific anyway (maybe a future test will bind a different operation in this callback).
Thanks all for kindly review! Send ps#5 to CQ now. https://codereview.chromium.org/2859343003/diff/110001/content/browser/vibrat... File content/browser/vibration_browsertest.cc (right): https://codereview.chromium.org/2859343003/diff/110001/content/browser/vibrat... content/browser/vibration_browsertest.cc:66: // Quit the run loop. On 2017/05/11 15:38:16, Ken Rockot wrote: > nit: Unnecessary comment, and I think too specific anyway (maybe a future test > will bind a different operation in this callback). Done.
The CQ bit was checked by leon.han@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/2859343003/#ps130001 (title: "Address nit")
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
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_...)
The CQ bit was checked by leon.han@intel.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 130001, "attempt_start_ts": 1494559941649660, "parent_rev": "9f244bd91e540e716aa156a4ee29cfb47558fed4", "commit_rev": "d25e37df0f08a754fbc11041451a5ce7ca7d043d"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/d25e37df0f08a754fbc11041451a... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:130001) as https://chromium.googlesource.com/chromium/src/+/d25e37df0f08a754fbc11041451a...
Message was sent while issue was closed.
This is great, thanks! This functionality is really important :).
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:130001) has been created in https://codereview.chromium.org/2882613002/ by henrika@chromium.org. The reason for reverting is: VibrationTest.Vibrate times out in browser_side_navigation_content_browsertests on Linux. See https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.linux%2FLinux_T....
Message was sent while issue was closed.
On 2017/05/12 07:59:54, henrika wrote: > A revert of this CL (patchset #5 id:130001) has been created in > https://codereview.chromium.org/2882613002/ by mailto:henrika@chromium.org. > > The reason for reverting is: VibrationTest.Vibrate times out in > browser_side_navigation_content_browsertests on Linux. > > See > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.linux%2FLinux_T.... Hi, I tried the following command line locally, it still succeeds, although it failed on Linux_Tests__dbg__1_ bot, which I can't find in trybots list.. I'm suspecting about 'browser_side_navigation_*', although I'm running content_browsertests with the flag --enable-browser-side-navigation, do we still need to set some 'browser_side_navigation_*' flags when building content_browsertests? Thanks! ./content_browsertests --brave-new-test-launcher --data-path=/b/s/w/itUCiS_v/.org.chromium.Chromium.JaWF4Z/dcTSFlO --enable-browser-side-navigation --gtest_also_run_disabled_tests --gtest_filter=VibrationTest.Vibrate --single_process --test-launcher-bot-mode --test-launcher-summary-output=/b/s/w/ioKHcJ3W/output.json --use-fake-device-for-media-stream
Message was sent while issue was closed.
On 2017/05/12 10:14:43, leonhsl wrote: > On 2017/05/12 07:59:54, henrika wrote: > > A revert of this CL (patchset #5 id:130001) has been created in > > https://codereview.chromium.org/2882613002/ by mailto:henrika@chromium.org. > > > > The reason for reverting is: VibrationTest.Vibrate times out in > > browser_side_navigation_content_browsertests on Linux. > > > > See > > > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.linux%2FLinux_T.... > > Hi, I tried the following command line locally, it still succeeds, although it > failed on Linux_Tests__dbg__1_ bot, which I can't find in trybots list.. > I'm suspecting about 'browser_side_navigation_*', although I'm running > content_browsertests with the flag --enable-browser-side-navigation, do we still > need to set some 'browser_side_navigation_*' flags when building > content_browsertests? Thanks! > > ./content_browsertests --brave-new-test-launcher > --data-path=/b/s/w/itUCiS_v/.org.chromium.Chromium.JaWF4Z/dcTSFlO > --enable-browser-side-navigation --gtest_also_run_disabled_tests > --gtest_filter=VibrationTest.Vibrate --single_process --test-launcher-bot-mode > --test-launcher-summary-output=/b/s/w/ioKHcJ3W/output.json > --use-fake-device-for-media-stream I looked at this for a while and I don't see anything obvious. The only thing that I can think of is that maybe there's some subtle issue with tests running in parallel and global state. Could you try replicating the bot run locally, i.e., ./content_browsertests --brave-new-test-launcher --test-launcher-bot-mode --enable-browser-side-navigation --test-launcher-summary-output=/b/s/w/ioKHcJ3W/output.json Verify that it says "Using 4 parallel jobs" at the top
Message was sent while issue was closed.
On 2017/05/15 08:11:50, blundell wrote: > On 2017/05/12 10:14:43, leonhsl wrote: > > On 2017/05/12 07:59:54, henrika wrote: > > > A revert of this CL (patchset #5 id:130001) has been created in > > > https://codereview.chromium.org/2882613002/ by mailto:henrika@chromium.org. > > > > > > The reason for reverting is: VibrationTest.Vibrate times out in > > > browser_side_navigation_content_browsertests on Linux. > > > > > > See > > > > > > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.linux%2FLinux_T.... > > > > Hi, I tried the following command line locally, it still succeeds, although it > > failed on Linux_Tests__dbg__1_ bot, which I can't find in trybots list.. > > I'm suspecting about 'browser_side_navigation_*', although I'm running > > content_browsertests with the flag --enable-browser-side-navigation, do we > still > > need to set some 'browser_side_navigation_*' flags when building > > content_browsertests? Thanks! > > > > ./content_browsertests --brave-new-test-launcher > > --data-path=/b/s/w/itUCiS_v/.org.chromium.Chromium.JaWF4Z/dcTSFlO > > --enable-browser-side-navigation --gtest_also_run_disabled_tests > > --gtest_filter=VibrationTest.Vibrate --single_process --test-launcher-bot-mode > > --test-launcher-summary-output=/b/s/w/ioKHcJ3W/output.json > > --use-fake-device-for-media-stream > > I looked at this for a while and I don't see anything obvious. The only thing > that I can think of is that maybe there's some subtle issue with tests running > in parallel and global state. Could you try replicating the bot run locally, > i.e., > > ./content_browsertests --brave-new-test-launcher --test-launcher-bot-mode > --enable-browser-side-navigation > --test-launcher-summary-output=/b/s/w/ioKHcJ3W/output.json > > Verify that it says "Using 4 parallel jobs" at the top Thanks Colin a lot for help! I tried above command for 3 times, with "Using 44 parallel jobs" being verified. The 1st and 3rd try caused my Ubuntu hang and I had to restart Ubuntu.. The 2nd time it completed, reporting 200+ failed cases, but VibrationTest passed.
Message was sent while issue was closed.
On 2017/05/15 09:51:33, leonhsl wrote: > On 2017/05/15 08:11:50, blundell wrote: > > On 2017/05/12 10:14:43, leonhsl wrote: > > > On 2017/05/12 07:59:54, henrika wrote: > > > > A revert of this CL (patchset #5 id:130001) has been created in > > > > https://codereview.chromium.org/2882613002/ by > mailto:henrika@chromium.org. > > > > > > > > The reason for reverting is: VibrationTest.Vibrate times out in > > > > browser_side_navigation_content_browsertests on Linux. > > > > > > > > See > > > > > > > > > > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.linux%2FLinux_T.... > > > > > > Hi, I tried the following command line locally, it still succeeds, although > it > > > failed on Linux_Tests__dbg__1_ bot, which I can't find in trybots list.. > > > I'm suspecting about 'browser_side_navigation_*', although I'm running > > > content_browsertests with the flag --enable-browser-side-navigation, do we > > still > > > need to set some 'browser_side_navigation_*' flags when building > > > content_browsertests? Thanks! > > > > > > ./content_browsertests --brave-new-test-launcher > > > --data-path=/b/s/w/itUCiS_v/.org.chromium.Chromium.JaWF4Z/dcTSFlO > > > --enable-browser-side-navigation --gtest_also_run_disabled_tests > > > --gtest_filter=VibrationTest.Vibrate --single_process > --test-launcher-bot-mode > > > --test-launcher-summary-output=/b/s/w/ioKHcJ3W/output.json > > > --use-fake-device-for-media-stream > > > > I looked at this for a while and I don't see anything obvious. The only thing > > that I can think of is that maybe there's some subtle issue with tests running > > in parallel and global state. Could you try replicating the bot run locally, > > i.e., > > > > ./content_browsertests --brave-new-test-launcher --test-launcher-bot-mode > > --enable-browser-side-navigation > > --test-launcher-summary-output=/b/s/w/ioKHcJ3W/output.json > > > > Verify that it says "Using 4 parallel jobs" at the top > > Thanks Colin a lot for help! > I tried above command for 3 times, with "Using 44 parallel jobs" being verified. > The 1st and 3rd try caused my Ubuntu hang and I had to restart Ubuntu.. > The 2nd time it completed, reporting 200+ failed cases, but VibrationTest > passed. And those 200+ failed cases seems are all TIMED OUT.
Message was sent while issue was closed.
As we can't reproduce the failure and it's hard to identify the root cause, no any hints by reading codes through,, could I reland this infra CL by replacing the suspect VibrationTest with BatteryMonitorTest written in https://codereview.chromium.org/2882633002/? Let's see what will happen then? ;-) Landing this infra will unblock other end-to-end browser tests, and we can focus on VibrationTest's possible problem later.
Message was sent while issue was closed.
Oof, I spent a bunch of time debugging this today and got to the bottom of it. Nasty. The problem is the component build. service_context.cc is in a static_library, which is linked into multiple components in the component build. Each of those components gets its own copy of |g_overridden_binder_registries|. content_browser is in a different component than vibration_browsertest.cc, so when you set the override in the latter, it's into a different map than the former is checking. Presumably, this didn't show up for you locally because you're not using component build. For the same reason, it didn't show up on the trybots but showed up on the linux_dbg main waterfall bot. The only "easy" solution that I know of is to make //services/service_manager/public/cpp into a component, so that that code only gets linked in once per executable. Ken, are there any gotchas/reasons not to do that?
Message was sent while issue was closed.
There are, but to it's definitely the right thing to do. Please let me take a crack at it today. Fortunately I have landed support for generated mojom component targets, and that should simplify making the client lib a component too. Thanks for figuring this out... component build strikes again. On May 16, 2017 6:05 AM, <blundell@chromium.org> wrote: > Oof, I spent a bunch of time debugging this today and got to the bottom of > it. > Nasty. > > The problem is the component build. service_context.cc is in a > static_library, > which is linked into multiple components in the component build. Each of > those > components gets its own copy of |g_overridden_binder_registries|. > content_browser is in a different component than vibration_browsertest.cc, > so > when you set the override in the latter, it's into a different map than the > former is checking. > > Presumably, this didn't show up for you locally because you're not using > component build. For the same reason, it didn't show up on the trybots but > showed up on the linux_dbg main waterfall bot. > > The only "easy" solution that I know of is to make > //services/service_manager/public/cpp into a component, so that that code > only > gets linked in once per executable. Ken, are there any gotchas/reasons not > to do > that? > > https://codereview.chromium.org/2859343003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
https://codereview.chromium.org/2859343003/diff/130001/services/service_manag... File services/service_manager/public/cpp/service_context.cc (right): https://codereview.chromium.org/2859343003/diff/130001/services/service_manag... 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 LazyInstance needs to hold a unique_ptr as opposed to just directly holding a ServiceNameToBinderRegistryMap object.
Message was sent while issue was closed.
I've made my first attempt at componentizing the service manager public libraries. Let's see if it's green: https://chromium-review.googlesource.com/506598 On Tue, May 16, 2017 at 8:00 AM, <blundell@chromium.org> wrote: > > 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 LazyInstance needs to hold a > unique_ptr as opposed to just directly holding a > ServiceNameToBinderRegistryMap object. > > https://codereview.chromium.org/2859343003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
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!
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 |