New issue
Advanced search Search tips

Issue 904148 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Dec 3
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Connector.StartService is poorly named and misused as a result

Project Member Reported by rockot@google.com, Nov 10

Issue description

It should be called something like WarmService or EnsureMatchingServiceInstanceRunning or something like that. I think the intent should always be limited to pre-rolling service instance startup when a client knows it's going to need to use it soon.

Currently it gets used to mean "I expect this service to not be running now, and I want to make it run so that it does something on startup."

Services should generally not do interesting things on startup though (at least not things which are interesting from any client's perspective), because most clients can't and shouldn't be able to reason about when service instances are going to be started. Trying to elicit meaningful side effects by invoking StartService() -- beyond the intent of "make sure this is running ASAP because I'm gonna use its interfaces" -- is likely to eventually lead to confusing or incorrect behavior.

It's also problematic because in general services should kill themselves when they realize they're idle. For most services, "idle" means that the last known client has closed the last interface pipe to the service. If no client ever acquires a pipe, there's no chance to observe that event. So if StartService were inadvertently used with a service which controls its lifecycle this way, we'd end up with stale leaky processes.

Fortunately it's only used today in cases where either someone is going to imminently bind an interface (like with Network Service) or with UI-facing services whose lifetime is controlled by user actions alone.

In the latter cases, we should instead add an explicit interface to elicit the desired behavior. We could for example define a common interface like:

  module ui.mojom;
  interface UserApplication {
    // Ensures that any toplevel UI elements (e.g. main window) are created
    // for a UI-driven service. Once this is called, the service instance
    // may be expected to run until explicitly terminated through user action.
    EnsureUiVisible();
  };

And have things like quick_launch and tap_visualizer implement this rather than showing their UI in Service.OnStart() or whatever. Then when we want those services to show UI, we can do:

  ui::mojom::UserApplicationPtr application;
  connector->BindInterface(
      service_manager::ServiceFilter::ByName(quick_launch::mojom::kServiceName),
      mojo::MakeRequest(&application));
  application->EnsureUiVisible();

This is obviously more verbose than connector->StartService(...) but it's also more technically correct and therefore less error-prone and confusing in the limit.

Note that mash defines Launchable which is quite similar in spirit, but I would expect us to define something more generic at the //ui layer.

Once the aforementioned interface is defined and used in place of StartService where appropriate, we can justify renaming StartService to WarmService or something similar, and avoid this confusion moving forward.
 
Status: Fixed (was: Untriaged)
Project Member

Comment 2 by bugdroid1@chromium.org, Jan 2

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

commit 1d323b303e9dffc236aff587691a6aa156193ad1
Author: James Cook <jamescook@chromium.org>
Date: Wed Jan 02 19:42:29 2019

chromeos: Don't use WarmService to start tap_visualizer

Instead define an explicit Show() method to start the UI. See bug for
details.

TBR=sky@chromium.org

Bug:  904148 
Test: Run chrome --show-taps, see touch point circles
Change-Id: I9d718097be9bb685e1e218780489fffce386a622
Reviewed-on: https://chromium-review.googlesource.com/c/1385013
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#619458}
[modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/components/tap_visualizer/BUILD.gn
[modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/components/tap_visualizer/manifest.json
[modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/components/tap_visualizer/public/mojom/BUILD.gn
[rename] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/components/tap_visualizer/public/mojom/tap_visualizer.mojom
[modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/components/tap_visualizer/tap_visualizer_app.cc
[modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/components/tap_visualizer/tap_visualizer_app.h
[modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/components/tap_visualizer/tap_visualizer_app_unittest.cc
[modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/shell.cc
[modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/shell/ash_content_browser_manifest_overlay.json
[modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/shell/content/client/shell_browser_main_parts.cc
[modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/shell/content/client/shell_content_browser_client.cc
[modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/ash/shell/content/client/shell_main_delegate.cc
[modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/chrome/browser/ash_service_registry.cc
[modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/chrome/browser/chrome_content_browser_manifest_overlay.json
[modify] https://crrev.com/1d323b303e9dffc236aff587691a6aa156193ad1/chrome/utility/mash_service_factory.cc

Sign in to add a comment