mash: Split Chrome's D-Bus providers across multiple services |
||||||||||||||||||||
Issue descriptionCurrently, we have a CrosDBusService singleton under //chromeos/dbus with an implementation that grabs the org.chromium.LibCrosService D-Bus service name. Individual service providers implement CrosDBusService::ServiceProviderInterface, which has a Start method that's called so they can register their methods. CrosDBusService and all of the providers are initialized by chrome/browser/chromeos/chrome_browser_main_chromeos.cc. For mustash, Chrome and ash will be running in separate processes, so we'll need at least two separate D-Bus service names (one for each). Some providers should live in ash (e.g. display power configuration), while others should live in Chrome (e.g. proxy resolution). I don't understand why CrosDBusService is a singleton. I don't think we use it anywhere outside of chrome_browser_main_chromeos.cc. Maybe we just like singletons? :-( I'd like to change CrosDBusService to not be a singleton, and to instead take service and object names as parameters. Then we'll be able to instantiate more than one of them. After discussion with James, I think that I'm partially convinced that we should have individual service names for things like display configuration and proxy resolution, rather than just moving all of the existing providers into either org.chromium.ChromeService or org.chromium.AshService. Having more fine-grained service names gives us more flexibility to move services between processes later without needing to update code outside of Chrome. On the other hand, it'd be less obvious where a particular provider lives. If others have opinions about this, I'd like to hear them! So as for how to actually accomplish this, I think we should: a) Make CrosDBusService not be a singleton. b) Instantiate more CrosDBusServices (at least one in Chrome and one in ash). c) Start moving providers out of LibCrosService and into the new services as appropriate. c) will be a bit tricky to do without breaking things. I need to think more about whether we should instantiate two instances of a provider within Chrome/ash until we've updated all of the callers to call it via the new service, or update the callers first to automatically fail back to the old service name if the new one is unavailable.
,
Feb 14 2017
,
Feb 14 2017
,
Feb 15 2017
I'm starting on a), since it seems like we'll definitely need multiple services with different names.
,
Feb 16 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cda054bfe94bdcbdb275b563d52dc7f1d443f94b commit cda054bfe94bdcbdb275b563d52dc7f1d443f94b Author: derat <derat@chromium.org> Date: Thu Feb 16 14:39:03 2017 chromeos: Make CrosDBusService not be a singleton. Update the CrosDBusService class, currently only used to implement org.chromium.LibCrosService, so it can be instantiated multiple times for different services. This is needed for mash, where some services will be implemented in ash and others in Chrome. BUG= 692246 Review-Url: https://codereview.chromium.org/2693243002 Cr-Commit-Position: refs/heads/master@{#450957} [modify] https://crrev.com/cda054bfe94bdcbdb275b563d52dc7f1d443f94b/chrome/browser/chromeos/chrome_browser_main_chromeos.cc [modify] https://crrev.com/cda054bfe94bdcbdb275b563d52dc7f1d443f94b/chromeos/dbus/services/cros_dbus_service.cc [modify] https://crrev.com/cda054bfe94bdcbdb275b563d52dc7f1d443f94b/chromeos/dbus/services/cros_dbus_service.h [modify] https://crrev.com/cda054bfe94bdcbdb275b563d52dc7f1d443f94b/chromeos/dbus/services/cros_dbus_service_unittest.cc
,
Mar 1 2017
,
Mar 9 2017
Cc-ing teravest@, who's been looking at moving LivenessServiceProvider to its own service. I think we'll eventually want the CheckLiveness method to be part of a service that's owned by the main mustash process (is that mash right now?). session_manager uses this to know if the process that it starts is still responsive; presumably something else inside the mustash conglomerate of processes will be responsible for babysitting the browser process. So maybe this should be moved to something with a name like org.chromium.LivenessService for now, or if that's too generic, org.chromium.BrowserLivenessService. Does anyone else have opinions?
,
Mar 10 2017
There is a "root process" that runs the master mojo service manager and doesn't do much else. It's the root of the process tree when you run chrome --mash. In the interest of simplicity I might start by extracting org.chromium.LivenessService first and having it only monitor the browser, then later extend it to the root process and monitoring of the rest of the system.
,
Mar 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/074cfdfc2e36001a72f2e2dfbcd7cc91819221e4 commit 074cfdfc2e36001a72f2e2dfbcd7cc91819221e4 Author: teravest <teravest@chromium.org> Date: Mon Mar 13 17:17:41 2017 Split LivenessService out from LibCrosService. This is part of a larger effort to split LibCrosService into smaller services for mus+ash. I've added a TODO to remove the old service once we've migrated clients over to call the new one. BUG= 692246 Review-Url: https://codereview.chromium.org/2741803005 Cr-Commit-Position: refs/heads/master@{#456411} [modify] https://crrev.com/074cfdfc2e36001a72f2e2dfbcd7cc91819221e4/chrome/browser/chromeos/chrome_browser_main_chromeos.cc [modify] https://crrev.com/074cfdfc2e36001a72f2e2dfbcd7cc91819221e4/chromeos/dbus/services/liveness_service_provider.cc [modify] https://crrev.com/074cfdfc2e36001a72f2e2dfbcd7cc91819221e4/chromeos/dbus/services/liveness_service_provider.h
,
Mar 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ecf7f3deeb90473ecd3ef116301f0a02637da717 commit ecf7f3deeb90473ecd3ef116301f0a02637da717 Author: khushalsagar <khushalsagar@chromium.org> Date: Mon Mar 13 17:54:21 2017 Revert of Split LivenessService out from LibCrosService. (patchset #3 id:40001 of https://codereview.chromium.org/2741803005/ ) Reason for revert: Speculative revert for failing downstream bots: https://bugs.chromium.org/p/chromium/issues/detail?id=700986 Original issue's description: > Split LivenessService out from LibCrosService. > > This is part of a larger effort to split LibCrosService into smaller > services for mus+ash. > > I've added a TODO to remove the old service once we've migrated > clients over to call the new one. > > BUG= 692246 > > Review-Url: https://codereview.chromium.org/2741803005 > Cr-Commit-Position: refs/heads/master@{#456411} > Committed: https://chromium.googlesource.com/chromium/src/+/074cfdfc2e36001a72f2e2dfbcd7cc91819221e4 TBR=derat@chromium.org,teravest@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 692246 Review-Url: https://codereview.chromium.org/2742343004 Cr-Commit-Position: refs/heads/master@{#456422} [modify] https://crrev.com/ecf7f3deeb90473ecd3ef116301f0a02637da717/chrome/browser/chromeos/chrome_browser_main_chromeos.cc [modify] https://crrev.com/ecf7f3deeb90473ecd3ef116301f0a02637da717/chromeos/dbus/services/liveness_service_provider.cc [modify] https://crrev.com/ecf7f3deeb90473ecd3ef116301f0a02637da717/chromeos/dbus/services/liveness_service_provider.h
,
Mar 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b2a374ecdadcd8ebb5f6b9478458725ca25768e0 commit b2a374ecdadcd8ebb5f6b9478458725ca25768e0 Author: khushalsagar <khushalsagar@chromium.org> Date: Mon Mar 13 18:03:54 2017 Reland of Split LivenessService out from LibCrosService. (patchset #1 id:1 of https://codereview.chromium.org/2742343004/ ) Reason for revert: Reverting the revert. Looks like this change is unrelated. https://bugs.chromium.org/p/chromium/issues/detail?id=700823 Original issue's description: > Revert of Split LivenessService out from LibCrosService. (patchset #3 id:40001 of https://codereview.chromium.org/2741803005/ ) > > Reason for revert: > Speculative revert for failing downstream bots: https://bugs.chromium.org/p/chromium/issues/detail?id=700986 > > Original issue's description: > > Split LivenessService out from LibCrosService. > > > > This is part of a larger effort to split LibCrosService into smaller > > services for mus+ash. > > > > I've added a TODO to remove the old service once we've migrated > > clients over to call the new one. > > > > BUG= 692246 > > > > Review-Url: https://codereview.chromium.org/2741803005 > > Cr-Commit-Position: refs/heads/master@{#456411} > > Committed: https://chromium.googlesource.com/chromium/src/+/074cfdfc2e36001a72f2e2dfbcd7cc91819221e4 > > TBR=derat@chromium.org,teravest@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG= 692246 > > Review-Url: https://codereview.chromium.org/2742343004 > Cr-Commit-Position: refs/heads/master@{#456422} > Committed: https://chromium.googlesource.com/chromium/src/+/ecf7f3deeb90473ecd3ef116301f0a02637da717 TBR=derat@chromium.org,teravest@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 692246 Review-Url: https://codereview.chromium.org/2748803002 Cr-Commit-Position: refs/heads/master@{#456424} [modify] https://crrev.com/b2a374ecdadcd8ebb5f6b9478458725ca25768e0/chrome/browser/chromeos/chrome_browser_main_chromeos.cc [modify] https://crrev.com/b2a374ecdadcd8ebb5f6b9478458725ca25768e0/chromeos/dbus/services/liveness_service_provider.cc [modify] https://crrev.com/b2a374ecdadcd8ebb5f6b9478458725ca25768e0/chromeos/dbus/services/liveness_service_provider.h
,
Mar 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7d5e4125a792e09524a5942e829f2cfe738aa253 commit 7d5e4125a792e09524a5942e829f2cfe738aa253 Author: teravest <teravest@chromium.org> Date: Mon Mar 13 21:45:26 2017 Revert of Split LivenessService out from LibCrosService. (patchset #3 id:40001 of https://codereview.chromium.org/2741803005/ ) Reason for revert: Another ChromeOS change has to land which grants permissions to this name. Original issue's description: > Split LivenessService out from LibCrosService. > > This is part of a larger effort to split LibCrosService into smaller > services for mus+ash. > > I've added a TODO to remove the old service once we've migrated > clients over to call the new one. > > BUG= 692246 > > Review-Url: https://codereview.chromium.org/2741803005 > Cr-Commit-Position: refs/heads/master@{#456411} > Committed: https://chromium.googlesource.com/chromium/src/+/074cfdfc2e36001a72f2e2dfbcd7cc91819221e4 TBR=derat@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 692246 Review-Url: https://codereview.chromium.org/2751543002 Cr-Commit-Position: refs/heads/master@{#456499} [modify] https://crrev.com/7d5e4125a792e09524a5942e829f2cfe738aa253/chrome/browser/chromeos/chrome_browser_main_chromeos.cc [modify] https://crrev.com/7d5e4125a792e09524a5942e829f2cfe738aa253/chromeos/dbus/services/liveness_service_provider.cc [modify] https://crrev.com/7d5e4125a792e09524a5942e829f2cfe738aa253/chromeos/dbus/services/liveness_service_provider.h
,
Mar 14 2017
,
Mar 20 2017
I'm going to file a bug for each of the services to make it easier to track progress here.
,
Mar 20 2017
,
Mar 20 2017
,
Mar 20 2017
,
Mar 20 2017
,
Mar 20 2017
,
Mar 20 2017
,
Mar 20 2017
,
Apr 10 2017
,
Apr 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1e3f3210c07f758054460e5ca0c1bd46f9f13a3e commit 1e3f3210c07f758054460e5ca0c1bd46f9f13a3e Author: derat <derat@chromium.org> Date: Tue Apr 18 19:44:27 2017 chromeos: Custom services in ServiceProviderTestHelper. Make ServiceProviderTestHelper support arbitrary D-Bus services instead of just org.chromium.LibCrosService. This is needed by tests for providers that are getting moved to their own services. BUG= 692246 Review-Url: https://codereview.chromium.org/2824093003 Cr-Commit-Position: refs/heads/master@{#465333} [modify] https://crrev.com/1e3f3210c07f758054460e5ca0c1bd46f9f13a3e/chromeos/dbus/services/proxy_resolution_service_provider_unittest.cc [modify] https://crrev.com/1e3f3210c07f758054460e5ca0c1bd46f9f13a3e/chromeos/dbus/services/service_provider_test_helper.cc [modify] https://crrev.com/1e3f3210c07f758054460e5ca0c1bd46f9f13a3e/chromeos/dbus/services/service_provider_test_helper.h
,
May 1 2017
,
Mar 30 2018
,
Apr 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/ae2edd9a02717ddf2f173705b277b941d08eb1db commit ae2edd9a02717ddf2f173705b277b941d08eb1db Author: Daniel Erat <derat@chromium.org> Date: Mon Apr 09 22:18:43 2018 chromeos-base: Remove old 99-usb-printer.rules. Remove an ancient udev rule that looks like it was used to notify Chrome when a USB printer was connected (so Chrome could display an error?). Chrome was notified by emitting a signal on the org.chromium.LibCrosService D-Bus service, which will soon be removed. I can't find anything that consumes this signal anymore. BUG= chromium:692246 , chromium:218515 TEST=none Change-Id: If736ee97f9dc658e2458f9016eb6dabbda2320e6 Reviewed-on: https://chromium-review.googlesource.com/1001094 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Chirantan Ekbote <chirantan@chromium.org> [rename] https://crrev.com/ae2edd9a02717ddf2f173705b277b941d08eb1db/chromeos-base/chromeos-base/chromeos-base-0-r144.ebuild [delete] https://crrev.com/92a49a033b289938ad6ab6f52fb30f585c1b14e5/chromeos-base/chromeos-base/files/udev-rules/99-usb-printer.rules
,
Apr 19 2018
,
May 12 2018
,
May 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1f552ddb3033a95fec6ef56c4cc2ffb598c1234e commit 1f552ddb3033a95fec6ef56c4cc2ffb598c1234e Author: Daniel Erat <derat@chromium.org> Date: Mon May 14 23:37:26 2018 chromeos: Remove org.chromium.LibCrosService. Make Chrome stop providing an org.chromium.LibCrosService D-Bus service. All of the method calls formerly exported on this service have been moved into their own services now, and callers have been updated to use those instead. Bug: 692246 Change-Id: I30ec76570a18d669f37609d662275ed2da7c2012 Reviewed-on: https://chromium-review.googlesource.com/1056051 Reviewed-by: Satoru Takabayashi <satorux@chromium.org> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Commit-Queue: Dan Erat <derat@chromium.org> Cr-Commit-Position: refs/heads/master@{#558528} [modify] https://crrev.com/1f552ddb3033a95fec6ef56c4cc2ffb598c1234e/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
,
May 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/system_api/+/99cdcd8c556df4985f6e6e262dd6b379ecb163d2 commit 99cdcd8c556df4985f6e6e262dd6b379ecb163d2 Author: Daniel Erat <derat@chromium.org> Date: Wed May 16 12:08:32 2018 system_api: Remove org.chromium.LibCrosService constants. Remove constants from org.chromium.LibCrosService that have since been moved to org.chromium.NetworkProxyService, org.chromium.LivenessService, org.chromium.ScreenLockService, and org.chromium.KioskAppService. BUG= chromium:692246 TEST=none Change-Id: I051fb4ab4d5656dfdfe9d2582ed74f461cc916f3 Reviewed-on: https://chromium-review.googlesource.com/1059859 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: James Cook <jamescook@chromium.org> Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org> Reviewed-by: Satoru Takabayashi <satorux@chromium.org> [modify] https://crrev.com/99cdcd8c556df4985f6e6e262dd6b379ecb163d2/dbus/service_constants.h
,
May 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/74d6affc924eb1f7895c4680b7a0c6f0fed76dd7 commit 74d6affc924eb1f7895c4680b7a0c6f0fed76dd7 Author: Daniel Erat <derat@chromium.org> Date: Wed May 16 12:08:26 2018 chromeos-chrome: Stop installing LibCrosService config. Make the chromeos-chrome package not install the org.chromium.LibCrosService.conf D-Bus permission file to /etc/dbus-1/system.d. Chrome no longer creates this service. BUG= chromium:692246 TEST=none Change-Id: I72ae7c759b75a5a9c3c38019835c02c99995ec5d Reviewed-on: https://chromium-review.googlesource.com/1060514 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Steven Bennetts <stevenjb@chromium.org> [modify] https://crrev.com/74d6affc924eb1f7895c4680b7a0c6f0fed76dd7/chromeos-base/chromeos-chrome/chromeos-chrome-9999.ebuild
,
May 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/164b0812cd618f246da39f88edba318e2df98281 commit 164b0812cd618f246da39f88edba318e2df98281 Author: Daniel Erat <derat@chromium.org> Date: Thu May 17 21:08:08 2018 chromeos: Remove last bits of org.chromium.LibCrosService. Remove the org.chromium.LibCrosService.conf file that declared permissions for the now-deleted LibCrosService D-Bus service. Also update KioskInfoServiceProvider, ScreenLockServiceProvider, and LivenessServiceProvider to no longer support using arbitrary interface and method names, as we only use them in their own dedicated services now. Bug: 692246 Change-Id: I2b31f7d599f6bc0601e5ad84071abedc2a56651c Reviewed-on: https://chromium-review.googlesource.com/1064730 Reviewed-by: Steven Bennetts <stevenjb@chromium.org> Commit-Queue: Dan Erat <derat@chromium.org> Cr-Commit-Position: refs/heads/master@{#559677} [modify] https://crrev.com/164b0812cd618f246da39f88edba318e2df98281/chrome/browser/chromeos/chrome_browser_main_chromeos.cc [modify] https://crrev.com/164b0812cd618f246da39f88edba318e2df98281/chrome/browser/chromeos/dbus/kiosk_info_service_provider.cc [modify] https://crrev.com/164b0812cd618f246da39f88edba318e2df98281/chrome/browser/chromeos/dbus/kiosk_info_service_provider.h [modify] https://crrev.com/164b0812cd618f246da39f88edba318e2df98281/chrome/browser/chromeos/dbus/screen_lock_service_provider.cc [modify] https://crrev.com/164b0812cd618f246da39f88edba318e2df98281/chrome/browser/chromeos/dbus/screen_lock_service_provider.h [modify] https://crrev.com/164b0812cd618f246da39f88edba318e2df98281/chromeos/BUILD.gn [modify] https://crrev.com/164b0812cd618f246da39f88edba318e2df98281/chromeos/dbus/services/liveness_service_provider.cc [modify] https://crrev.com/164b0812cd618f246da39f88edba318e2df98281/chromeos/dbus/services/liveness_service_provider.h [delete] https://crrev.com/133fb1221e6c293dc43ff438567b2834b2e5c798/chromeos/dbus/services/org.chromium.LibCrosService.conf
,
May 17 2018
,
May 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/92971f40408a3c6e1ac1cb816cfc5d381c8627be commit 92971f40408a3c6e1ac1cb816cfc5d381c8627be Author: Daniel Erat <derat@chromium.org> Date: Fri May 18 04:26:37 2018 chromeos: Use correct constant for CheckLiveness method. Update LivenessServiceProvider to use system_api's kLivenessServiceCheckLivenessMethod constant rather than the now-deleted kCheckLiveness constant. Both have the same value, "CheckLiveness". Bug: 692246 Change-Id: Iff9d64aade81720cef62a09e0e98fa460cbca960 Reviewed-on: https://chromium-review.googlesource.com/1065491 Commit-Queue: Dan Erat <derat@chromium.org> Commit-Queue: Ryo Hashimoto <hashimoto@chromium.org> Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#559798} [modify] https://crrev.com/92971f40408a3c6e1ac1cb816cfc5d381c8627be/chromeos/dbus/services/liveness_service_provider.cc
,
May 18 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/autotest/+/61f6b07a9b9d8546be1409b5e4a77337f3a9f7e6 commit 61f6b07a9b9d8546be1409b5e4a77337f3a9f7e6 Author: Daniel Erat <derat@chromium.org> Date: Fri May 18 12:32:40 2018 autotest: Remove LibCrosService from security_DbusOwners. Remove org.chromium.LibCrosService from security_DbusOwners's baseline list of services that should be owned by the chronos user. BUG= chromium:692246 ,chromium:712861, chromium:833855 TEST=none CQ-DEPEND=I72ae7c759b75a5a9c3c38019835c02c99995ec5d Change-Id: Ib05fd84a7d88ad9125aa56d468c678841c68a4ca Reviewed-on: https://chromium-review.googlesource.com/1060469 Commit-Ready: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org> [modify] https://crrev.com/61f6b07a9b9d8546be1409b5e4a77337f3a9f7e6/client/site_tests/security_DbusOwners/baseline
,
Aug 16
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/docs/+/788e2751a18dade6608056acc91484f0e7d4c7d4 commit 788e2751a18dade6608056acc91484f0e7d4c7d4 Author: Daniel Erat <derat@chromium.org> Date: Thu Aug 16 01:44:50 2018 docs: Update service locations in dbus_in_chrome.md. Update dbus_in_chrome.md to recommend putting //chrome-dependent services in //chrome/browser/chromeos/dbus and non-//chrome-dependent services in //ash/dbus. BUG= chromium:692246 TEST=none Change-Id: I0e762e43e7a752a0468738fe8c9d3829e0ff36eb Reviewed-on: https://chromium-review.googlesource.com/1060565 Reviewed-by: Dan Erat <derat@chromium.org> Tested-by: Dan Erat <derat@chromium.org> [modify] https://crrev.com/788e2751a18dade6608056acc91484f0e7d4c7d4/dbus_in_chrome.md
,
Aug 18
Issue 843392 tracks moving service provider code out of //chromeos/dbus/services. |
||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||
Comment 1 by derat@chromium.org
, Feb 14 2017