New issue
Advanced search Search tips

Issue 831333 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Task



Sign in to add a comment

Initialize some D-Bus services within ash

Project Member Reported by derat@chromium.org, Apr 10 2018

Issue description

In chrome/browser/chromeos/chrome_browser_main_chromeos.cc:

- ChromeBrowserMainPartsChromeos::PreEarlyInitialization creates DBusPreEarlyInit
- DBusPreEarlyInit's c'tor calls DBusThreadManager::Initialize
- ChromeBrowserMainPartsChromeos::PostMainMessageLoopStart creates internal::DBusServices
- DBusServices constructs/owns a bunch of *ServiceProvider classes and exports them using chromeos::CrosDBusService (from chromeos/dbus/services/cros_dbus_service.h)

There are a few spots in //ash that also call DBusThreadManager::Initialize:

- ash/shell/content/client/shell_browser_main_parts.cc
- ash/window_manager_service.cc (only if not already initialized)

DBusThreadManager owns classes from //chromeos/dbus that implement chromeos::DBusClient and are used to call outside services. It already has some support for instantiating different sets of clients for Chrome vs. ash (tracked by issue 647367, I think).

Ash doesn't have support for initializing the service providers that derive from chromeos::CrosDBusService, though. These live in //chromeos/dbus/services and //chrome/browser/chromeos/dbus, but they're all instantiated by chrome_browser_main_chromeos.cc. This is despite the fact that some of them don't require Chrome.

Ash probably needs its own DBusServices class that gets initialized early on. It should own providers that live entirely within //chromeos/dbus/services (i.e. no Delegate interfaces implemented within //chrome) and providers that have delegates that are implemented within //ash. In the latter case, we should consider just getting rid of the delegates and moving the providers somewhere under //ash (//ash/dbus or //ash/system/dbus?).

 Issue 692246  tracks creating individual D-Bus services for everything that Chrome/ash do so that it's easier to distribute functionality across different processes. That's almost done.
 

Comment 1 by derat@chromium.org, Apr 10 2018

Cc: satorux@chromium.org steve...@chromium.org
AshDBusServices was introduced here for a new (not in Chrome) service:
https://chromium-review.googlesource.com/c/chromium/src/+/998081

Does it make sense to migrate any services that can be ash only to AshDBusServices?

Comment 3 by derat@chromium.org, Apr 12 2018

> Does it make sense to migrate any services that can be ash only to AshDBusServices?

Yes, I think so. That brings us closer to the Chrome-not-necessarily-always-running glorious future.
Status: Assigned (was: Untriaged)
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4204e85d87563d5d028182d7ec5c1f64697cc17c

commit 4204e85d87563d5d028182d7ec5c1f64697cc17c
Author: Jeffrey Kardatzke <jkardatzke@google.com>
Date: Sat Apr 14 17:43:16 2018

Add ash D-Bus framework & service for opening URLs with Chrome

This adds a small D-Bus service framework to ash and then adds an
initial service that has one call "OpenUrl" which will then
open a new tab in Chrome with that URL. This is intended to be used by
Chrome OS clients that want to open a URL with Chrome; initial usage
will be from VMs.

Bug:  822496 
Bug:  831333 
Test: Verified with dbus-send that links open properly
Change-Id: I648af624817cd387f30aba751c7f465e0dcc60e0
Reviewed-on: https://chromium-review.googlesource.com/998081
Commit-Queue: Jeffrey Kardatzke <jkardatzke@google.com>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550900}
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/BUILD.gn
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/DEPS
[add] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/dbus/DEPS
[add] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/dbus/ash_dbus_services.cc
[add] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/dbus/ash_dbus_services.h
[add] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/dbus/url_handler_service_provider.cc
[add] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/dbus/url_handler_service_provider.h
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/new_window_controller.cc
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/new_window_controller.h
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/public/interfaces/new_window.mojom
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/shell.cc
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/shell.h
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/chrome/browser/ui/ash/chrome_new_window_client.cc
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/chrome/browser/ui/ash/chrome_new_window_client.h

Project Member

Comment 6 by bugdroid1@chromium.org, Apr 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/26e00c44ee54f161eb59936baced6b089adc55b3

commit 26e00c44ee54f161eb59936baced6b089adc55b3
Author: Blake O'Hare <blakeo@chromium.org>
Date: Mon Apr 16 06:49:06 2018

Revert "Add ash D-Bus framework & service for opening URLs with Chrome"

This reverts commit 4204e85d87563d5d028182d7ec5c1f64697cc17c.

Reason for revert: Multiple people are noticing the following fatal error on startup. It's preventing Chrome OS from starting (noticed on an Eve and a Cyan). Testing with and without this change indicates that this CL is the cause:

[5643:5757:0416/141408.095095:ERROR:bus.cc(551)] Failed to get the ownership of org.chromium.UrlHandlerService: Connection ":1.102" is not allowed to own the service "org.chromium.UrlHandlerService" due to security policies in the configuration file
[5643:5643:0416/141408.181175:FATAL:cros_dbus_service.cc(77)] Failed to own: org.chromium.UrlHandlerService

Original change's description:
> Add ash D-Bus framework & service for opening URLs with Chrome
> 
> This adds a small D-Bus service framework to ash and then adds an
> initial service that has one call "OpenUrl" which will then
> open a new tab in Chrome with that URL. This is intended to be used by
> Chrome OS clients that want to open a URL with Chrome; initial usage
> will be from VMs.
> 
> Bug:  822496 
> Bug:  831333 
> Test: Verified with dbus-send that links open properly
> Change-Id: I648af624817cd387f30aba751c7f465e0dcc60e0
> Reviewed-on: https://chromium-review.googlesource.com/998081
> Commit-Queue: Jeffrey Kardatzke <jkardatzke@google.com>
> Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
> Reviewed-by: Dan Erat <derat@chromium.org>
> Reviewed-by: Dominick Ng <dominickn@chromium.org>
> Reviewed-by: Mike West <mkwst@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#550900}

TBR=derat@chromium.org,xiyuan@chromium.org,hashimoto@chromium.org,stevenjb@chromium.org,sky@chromium.org,jorgelo@chromium.org,benwells@chromium.org,mnissler@chromium.org,dominickn@chromium.org,jkardatzke@google.com,mkwst@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  822496 ,  831333 
Change-Id: Ie7ea0e469116c7b1968cf929c378d9bceaeb9e1d
Reviewed-on: https://chromium-review.googlesource.com/1013799
Reviewed-by: Blake O'Hare <blakeo@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Blake O'Hare <blakeo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550944}
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/ash/BUILD.gn
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/ash/DEPS
[delete] https://crrev.com/9b98a25a5d7df1f1b2b1f40bf20294ede0f18dbd/ash/dbus/DEPS
[delete] https://crrev.com/9b98a25a5d7df1f1b2b1f40bf20294ede0f18dbd/ash/dbus/ash_dbus_services.cc
[delete] https://crrev.com/9b98a25a5d7df1f1b2b1f40bf20294ede0f18dbd/ash/dbus/ash_dbus_services.h
[delete] https://crrev.com/9b98a25a5d7df1f1b2b1f40bf20294ede0f18dbd/ash/dbus/url_handler_service_provider.cc
[delete] https://crrev.com/9b98a25a5d7df1f1b2b1f40bf20294ede0f18dbd/ash/dbus/url_handler_service_provider.h
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/ash/new_window_controller.cc
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/ash/new_window_controller.h
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/ash/public/interfaces/new_window.mojom
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/ash/shell.cc
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/ash/shell.h
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/chrome/browser/ui/ash/chrome_new_window_client.cc
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/chrome/browser/ui/ash/chrome_new_window_client.h

Just FYI, there are some old design docs on dbus in mustash:
go/dbus-in-mustash

"Client code structure": https://docs.google.com/document/d/16pmp3Vpe1fIzdjakwyEw2Fn2HjX5WmWyS8S5on9H7a4/edit#

Dunno if they are still relevant, but that was our thinking at the time.

Comment 8 by derat@chromium.org, Apr 17 2018

#7: Thanks. I think that this bug corresponds to the "LibCrosService (in chrome)" section of go/dbus-in-mustash. This is tracking the services that Chrome currently exposes to other processes on the system (as opposed to the client classes that Chrome uses to talk to services exposed by various daemons).
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f108f0254bcf7f69e81fafa5592b266429e50197

commit f108f0254bcf7f69e81fafa5592b266429e50197
Author: Jeffrey Kardatzke <jkardatzke@google.com>
Date: Tue Apr 17 17:52:35 2018

Reland: Add ash D-Bus framework & service for opening URLs with Chrome

This adds a small D-Bus service framework to ash and then adds an
initial service that has one call "OpenUrl" which will then
open a new tab in Chrome with that URL. This is intended to be used by
Chrome OS clients that want to open a URL with Chrome; initial usage
will be from VMs.

Relanding crrev.com/c/998081, fixed missing D-Bus .conf file
TBR=stevenjb@chromium.org,dominickn@chromium.org,hashimoto@chromium.org,mkwst@chromium.org

Bug:  822496 
Bug:  831333 
Test: Verified with dbus-send that links open properly
Change-Id: I5e6d9c716ef6da88b184e8e05582825d9547946e
Reviewed-on: https://chromium-review.googlesource.com/1014424
Commit-Queue: Jeffrey Kardatzke <jkardatzke@google.com>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551383}
[modify] https://crrev.com/f108f0254bcf7f69e81fafa5592b266429e50197/ash/BUILD.gn
[modify] https://crrev.com/f108f0254bcf7f69e81fafa5592b266429e50197/ash/DEPS
[add] https://crrev.com/f108f0254bcf7f69e81fafa5592b266429e50197/ash/dbus/DEPS
[add] https://crrev.com/f108f0254bcf7f69e81fafa5592b266429e50197/ash/dbus/ash_dbus_services.cc
[add] https://crrev.com/f108f0254bcf7f69e81fafa5592b266429e50197/ash/dbus/ash_dbus_services.h
[add] https://crrev.com/f108f0254bcf7f69e81fafa5592b266429e50197/ash/dbus/org.chromium.UrlHandlerService.conf
[add] https://crrev.com/f108f0254bcf7f69e81fafa5592b266429e50197/ash/dbus/url_handler_service_provider.cc
[add] https://crrev.com/f108f0254bcf7f69e81fafa5592b266429e50197/ash/dbus/url_handler_service_provider.h
[modify] https://crrev.com/f108f0254bcf7f69e81fafa5592b266429e50197/ash/new_window_controller.cc
[modify] https://crrev.com/f108f0254bcf7f69e81fafa5592b266429e50197/ash/new_window_controller.h
[modify] https://crrev.com/f108f0254bcf7f69e81fafa5592b266429e50197/ash/public/interfaces/new_window.mojom
[modify] https://crrev.com/f108f0254bcf7f69e81fafa5592b266429e50197/ash/shell.cc
[modify] https://crrev.com/f108f0254bcf7f69e81fafa5592b266429e50197/ash/shell.h
[modify] https://crrev.com/f108f0254bcf7f69e81fafa5592b266429e50197/chrome/browser/ui/ash/chrome_new_window_client.cc
[modify] https://crrev.com/f108f0254bcf7f69e81fafa5592b266429e50197/chrome/browser/ui/ash/chrome_new_window_client.h

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4204e85d87563d5d028182d7ec5c1f64697cc17c

commit 4204e85d87563d5d028182d7ec5c1f64697cc17c
Author: Jeffrey Kardatzke <jkardatzke@google.com>
Date: Sat Apr 14 17:43:16 2018

Add ash D-Bus framework & service for opening URLs with Chrome

This adds a small D-Bus service framework to ash and then adds an
initial service that has one call "OpenUrl" which will then
open a new tab in Chrome with that URL. This is intended to be used by
Chrome OS clients that want to open a URL with Chrome; initial usage
will be from VMs.

Bug:  822496 
Bug:  831333 
Test: Verified with dbus-send that links open properly
Change-Id: I648af624817cd387f30aba751c7f465e0dcc60e0
Reviewed-on: https://chromium-review.googlesource.com/998081
Commit-Queue: Jeffrey Kardatzke <jkardatzke@google.com>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550900}
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/BUILD.gn
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/DEPS
[add] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/dbus/DEPS
[add] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/dbus/ash_dbus_services.cc
[add] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/dbus/ash_dbus_services.h
[add] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/dbus/url_handler_service_provider.cc
[add] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/dbus/url_handler_service_provider.h
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/new_window_controller.cc
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/new_window_controller.h
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/public/interfaces/new_window.mojom
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/shell.cc
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/ash/shell.h
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/chrome/browser/ui/ash/chrome_new_window_client.cc
[modify] https://crrev.com/4204e85d87563d5d028182d7ec5c1f64697cc17c/chrome/browser/ui/ash/chrome_new_window_client.h

Project Member

Comment 11 by bugdroid1@chromium.org, Apr 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/26e00c44ee54f161eb59936baced6b089adc55b3

commit 26e00c44ee54f161eb59936baced6b089adc55b3
Author: Blake O'Hare <blakeo@chromium.org>
Date: Mon Apr 16 06:49:06 2018

Revert "Add ash D-Bus framework & service for opening URLs with Chrome"

This reverts commit 4204e85d87563d5d028182d7ec5c1f64697cc17c.

Reason for revert: Multiple people are noticing the following fatal error on startup. It's preventing Chrome OS from starting (noticed on an Eve and a Cyan). Testing with and without this change indicates that this CL is the cause:

[5643:5757:0416/141408.095095:ERROR:bus.cc(551)] Failed to get the ownership of org.chromium.UrlHandlerService: Connection ":1.102" is not allowed to own the service "org.chromium.UrlHandlerService" due to security policies in the configuration file
[5643:5643:0416/141408.181175:FATAL:cros_dbus_service.cc(77)] Failed to own: org.chromium.UrlHandlerService

Original change's description:
> Add ash D-Bus framework & service for opening URLs with Chrome
> 
> This adds a small D-Bus service framework to ash and then adds an
> initial service that has one call "OpenUrl" which will then
> open a new tab in Chrome with that URL. This is intended to be used by
> Chrome OS clients that want to open a URL with Chrome; initial usage
> will be from VMs.
> 
> Bug:  822496 
> Bug:  831333 
> Test: Verified with dbus-send that links open properly
> Change-Id: I648af624817cd387f30aba751c7f465e0dcc60e0
> Reviewed-on: https://chromium-review.googlesource.com/998081
> Commit-Queue: Jeffrey Kardatzke <jkardatzke@google.com>
> Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
> Reviewed-by: Dan Erat <derat@chromium.org>
> Reviewed-by: Dominick Ng <dominickn@chromium.org>
> Reviewed-by: Mike West <mkwst@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#550900}

TBR=derat@chromium.org,xiyuan@chromium.org,hashimoto@chromium.org,stevenjb@chromium.org,sky@chromium.org,jorgelo@chromium.org,benwells@chromium.org,mnissler@chromium.org,dominickn@chromium.org,jkardatzke@google.com,mkwst@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  822496 ,  831333 
Change-Id: Ie7ea0e469116c7b1968cf929c378d9bceaeb9e1d
Reviewed-on: https://chromium-review.googlesource.com/1013799
Reviewed-by: Blake O'Hare <blakeo@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Blake O'Hare <blakeo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550944}
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/ash/BUILD.gn
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/ash/DEPS
[delete] https://crrev.com/9b98a25a5d7df1f1b2b1f40bf20294ede0f18dbd/ash/dbus/DEPS
[delete] https://crrev.com/9b98a25a5d7df1f1b2b1f40bf20294ede0f18dbd/ash/dbus/ash_dbus_services.cc
[delete] https://crrev.com/9b98a25a5d7df1f1b2b1f40bf20294ede0f18dbd/ash/dbus/ash_dbus_services.h
[delete] https://crrev.com/9b98a25a5d7df1f1b2b1f40bf20294ede0f18dbd/ash/dbus/url_handler_service_provider.cc
[delete] https://crrev.com/9b98a25a5d7df1f1b2b1f40bf20294ede0f18dbd/ash/dbus/url_handler_service_provider.h
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/ash/new_window_controller.cc
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/ash/new_window_controller.h
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/ash/public/interfaces/new_window.mojom
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/ash/shell.cc
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/ash/shell.h
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/chrome/browser/ui/ash/chrome_new_window_client.cc
[modify] https://crrev.com/26e00c44ee54f161eb59936baced6b089adc55b3/chrome/browser/ui/ash/chrome_new_window_client.h

Components: Internals>Services>Ash
Labels: -Proj-Mustash-Mash
Status: Fixed (was: Assigned)

Sign in to add a comment