New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 848198 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug


Participants' hotlists:
Audio-Service


Sign in to add a comment

UMA stat Media.AudioService.ObservedProcessTerminationStatus does not collect data on any platform

Project Member Reported by marinaciocea@chromium.org, May 31 2018

Issue description

Project Member

Comment 1 by bugdroid1@chromium.org, Jun 1 2018

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

commit 492c922d15b9a4be21139a31736b538bc2a37f76
Author: Marina Ciocea <marinaciocea@chromium.org>
Date: Fri Jun 01 09:26:04 2018

Fix Media.AudioService.ObservedProcessTerminationStatus UMA stat collection.

Add audio service listener to browser child process listeners and reset |process_id_| when
audio process crashes, is killed or disconnected, instead of when service is stopped.

When running audio service out of process there are 4 possible ways to terminate audio process,
as shown in this[1] state diagram: audio service is stopped, audio process is killed,
audio process crashed or browser is shutdown. Notes:
- BrowserChildProcessHostDisconnected always gets called on BrowserChildProcessHostImpl destructor.
  Resetting |process_id_| on kill/crash ensures kDisconnect is not logged to UMA stat on kill/crash.
- On browser shutdown, if audio service does not call quit closure when it shuts down (i.e. when
  quit timeout is not set), UMA stat is not logged.
- When running audio service in process, |process_id_| is always kNullProcessId, and UMA stat is
  not logged.

[1] https://docs.google.com/drawings/d/1HfT5YIXWh3F4ScP6otnjOCfh1ewQmd7HUfVVvsdKtUc/edit?usp=sharing

Unrelated change: removed connector member from listener, it was only used in constructor.

Bug:  848198 
Change-Id: Iadec84cd2bc72ea05fcc88eeb86333fa0f57f592
Reviewed-on: https://chromium-review.googlesource.com/1080189
Commit-Queue: Marina Ciocea <marinaciocea@chromium.org>
Reviewed-by: Max Morin <maxmorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563572}
[modify] https://crrev.com/492c922d15b9a4be21139a31736b538bc2a37f76/content/browser/renderer_host/media/audio_service_listener.cc
[modify] https://crrev.com/492c922d15b9a4be21139a31736b538bc2a37f76/content/browser/renderer_host/media/audio_service_listener.h
[modify] https://crrev.com/492c922d15b9a4be21139a31736b538bc2a37f76/content/browser/renderer_host/media/audio_service_listener_unittest.cc

Status: Fixed (was: Started)
@maxmorin, @olka, should I merge this into M68?
Labels: Merge-Request-68
What OS's is this impacting? Please mark them in the bug so request gets routed appropriately. 
Labels: OS-Linux OS-Mac OS-Windows
Project Member

Comment 7 by sheriffbot@chromium.org, Jun 5 2018

Labels: -Merge-Request-68 Hotlist-Merge-Approved Merge-Approved-68
Your change meets the bar and is auto-approved for M68. Please go ahead and merge the CL to branch 3440 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 5 2018

Labels: -merge-approved-68 merge-merged-3440
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/af747db680d72a467a49516384286fb31b2f681a

commit af747db680d72a467a49516384286fb31b2f681a
Author: Marina Ciocea <marinaciocea@chromium.org>
Date: Tue Jun 05 09:15:08 2018

Fix Media.AudioService.ObservedProcessTerminationStatus UMA stat collection.

Add audio service listener to browser child process listeners and reset |process_id_| when
audio process crashes, is killed or disconnected, instead of when service is stopped.

When running audio service out of process there are 4 possible ways to terminate audio process,
as shown in this[1] state diagram: audio service is stopped, audio process is killed,
audio process crashed or browser is shutdown. Notes:
- BrowserChildProcessHostDisconnected always gets called on BrowserChildProcessHostImpl destructor.
  Resetting |process_id_| on kill/crash ensures kDisconnect is not logged to UMA stat on kill/crash.
- On browser shutdown, if audio service does not call quit closure when it shuts down (i.e. when
  quit timeout is not set), UMA stat is not logged.
- When running audio service in process, |process_id_| is always kNullProcessId, and UMA stat is
  not logged.

[1] https://docs.google.com/drawings/d/1HfT5YIXWh3F4ScP6otnjOCfh1ewQmd7HUfVVvsdKtUc/edit?usp=sharing

Unrelated change: removed connector member from listener, it was only used in constructor.

Bug:  848198 
Change-Id: Iadec84cd2bc72ea05fcc88eeb86333fa0f57f592
Reviewed-on: https://chromium-review.googlesource.com/1080189
Commit-Queue: Marina Ciocea <marinaciocea@chromium.org>
Reviewed-by: Max Morin <maxmorin@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#563572}(cherry picked from commit 492c922d15b9a4be21139a31736b538bc2a37f76)
Reviewed-on: https://chromium-review.googlesource.com/1086907
Cr-Commit-Position: refs/branch-heads/3440@{#183}
Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733}
[modify] https://crrev.com/af747db680d72a467a49516384286fb31b2f681a/content/browser/renderer_host/media/audio_service_listener.cc
[modify] https://crrev.com/af747db680d72a467a49516384286fb31b2f681a/content/browser/renderer_host/media/audio_service_listener.h
[modify] https://crrev.com/af747db680d72a467a49516384286fb31b2f681a/content/browser/renderer_host/media/audio_service_listener_unittest.cc

[bulk-edit: disregard if N/A] Can the owner please set milestone to this bug if applicable?

Sign in to add a comment