New issue
Advanced search Search tips

Issue 908319 link

Starred by 1 user
Status: Started
Owner:
rockot@google.com
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 904240



Sign in to add a comment

ServiceManagerListener API needs cleanup

Project Member Reported by rockot@google.com, Nov 26 
Methods and types are poorly named. There is also some redundancy in service instance state tracking, as "idle" and "stopped" instances don't really exist.**

Changes to make:

- Remove unused "id" field from RunningServiceInfo
- Rename RunningServiceInfo to ServiceInstanceInfo
- ServiceManagerListener should be renamed to ServiceInstanceObserver
- Methods should be renamed and refactored:
    - OnInit(RunningServiceInfo) => OnServiceInstanceObserverAdded(ServiceInstanceInfo)
    - OnServiceCreated(RunningServiceInfo) => OnServiceInstanceCreated(Identity)
    - OnServiceFailedToStart(Identity) => OnServiceInstanceFailedToStart(Identity)
    - OnServiceStarted(Identity, pid) => OnServiceInstanceStarted(Identity)
    - OnServicePIDReceived(Identity, pid) => OnServiceInstancePidKnown(Identity, pid)
    - OnServiceStopped(Identity) => OnServiceInstanceStopped(Identity)
- Guarantee strict ordering of OnServiceInstancePidKnown following OnServiceStarted

** Idle instances always change to "starting" state immediately upon construction, and stopped instances are always destroyed immediately. As such, no instance can ever be observed in "idle" or "stopped" states.
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 27 

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

commit 3bdf26ad8f19ee9b629e09eb04ecc95c870fe772
Author: Ken Rockot <rockot@google.com>
Date: Tue Nov 27 18:10:27 2018

Fix raciness in AudioServiceListener

This fixes potential race conditions in AudioServiceListener resulting
from the fact that a service instance can become unreachable (and may
therefore be replaced with a new one) before it is officially designated
as "stopped" by the Service Manager, even in the case of singleton
services.

The ServiceManagerListener API is augmented to convey a service
instance's current running state via RunningServiceInfo. This is used
by AudioServiceListener to more accurately track the running audio
service instance.

The listener now tracks the Identity of the most recently created
audio service instance and responds to events accordingly.

This also updates the logic and comments in resource_coordinator's
ProcessMap to accurately reflect current guarantees of the
ServiceManagerListener API, since these have changed slightly.

NOTRY=true

Bug:  818593 , 907898 ,908319
Change-Id: If3e8a35aa90db72a34c8f5f2d458f0f24c3447eb
Reviewed-on: https://chromium-review.googlesource.com/c/1350649
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Olga Sharonova <olka@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: oysteine <oysteine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611193}
[modify] https://crrev.com/3bdf26ad8f19ee9b629e09eb04ecc95c870fe772/content/browser/renderer_host/media/audio_service_listener.cc
[modify] https://crrev.com/3bdf26ad8f19ee9b629e09eb04ecc95c870fe772/content/browser/renderer_host/media/audio_service_listener.h
[modify] https://crrev.com/3bdf26ad8f19ee9b629e09eb04ecc95c870fe772/content/browser/renderer_host/media/audio_service_listener_unittest.cc
[modify] https://crrev.com/3bdf26ad8f19ee9b629e09eb04ecc95c870fe772/services/resource_coordinator/memory_instrumentation/process_map.cc
[modify] https://crrev.com/3bdf26ad8f19ee9b629e09eb04ecc95c870fe772/services/service_manager/public/mojom/service_manager.mojom
[modify] https://crrev.com/3bdf26ad8f19ee9b629e09eb04ecc95c870fe772/services/service_manager/service_manager.cc
[modify] https://crrev.com/3bdf26ad8f19ee9b629e09eb04ecc95c870fe772/services/service_manager/service_manager.h

Comment 2 by rockot@google.com, Jan 17 (5 days ago)

Blocking: 904240
►

Sign in to add a comment