New issue
Advanced search Search tips

Issue 856690 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 856531



Sign in to add a comment

Fix incomplete inheritance of BaseAudioContext and AudioContext

Project Member Reported by hongchan@chromium.org, Jun 26 2018

Issue description

The current call chain when BAC is collected:

BaseAudioContext::ContextDestroyed()
  -> BaseAudioContext::Uninitialize()

However, AC needs .close() call to invoke the uinitialization:

AudioContext::CloseContext()
  -> AudioContext::Uninitialize()

So without calling .close() method, AC's resources will not be collected properly.

The solution is:

AudioContext::ContextDestroyed()
  -> AudioContext::Uninitialize()
    -> BaseAudioContext::Uninitialize()

and also CloseContext() does not have to collect resource aggressively.

AudioContext::CloseContext()
  -> StopRendering()
  -> DidClose()
 
Defining a clear life cycle of BaseAudioContext and its derivatives requires a in-depth discussion, so first I would like to focus on the shutting down process.

1. User closes the context.
  - The explicit closure of context means that the resource can be collected.
  - However, the destination node must be valid.

2. User drops the reference to the context.
  - The context will leak unless it is closed.
  - When GC happens, BaseAudioContext::HasPendingActivity() gets called.

3. User closes the tab.
  - AudioContext::ContextDestroyed() gets called.
  - Eventually it will be chained to BaseAudioContext::ContextDestroyed().

Currently there is nothing we can do about 2., but 1. and 3. can be resolved.

Solution 1.
 - Stop the destination node rendering.
 - Do not delete the destination node. It will eventually be collected when
   ContextDestroyed() is called.

Solution 3.
 - Implement AudioContext::ContextDestroyed().
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 29 2018

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

commit fbeb7bf25a794a2d482e56faab4577b09a36c9d5
Author: Hongchan Choi <hongchan@chromium.org>
Date: Fri Jun 29 01:08:08 2018

Add AudioContext::ContextDestroyed

Currently AudioContext has no way of uninitializing the instance without
an explicit context closure. This extends BaseAudioContext's
ContextDestroyed to AudioContext so we can hook up its own uninitialize
routine, and then eventually calls the parent's uninitializer.

This also fixes other issue of UKM metrics being reported in less cases
because they were only reported when context.close() was called.

Bug:  856690 ,  688052 ,  670065 
Change-Id: I721dc2c27201caab1b0a95baf8f6906f44b76979
Reviewed-on: https://chromium-review.googlesource.com/1115516
Commit-Queue: Hongchan Choi <hongchan@chromium.org>
Reviewed-by: Raymond Toy <rtoy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571349}
[modify] https://crrev.com/fbeb7bf25a794a2d482e56faab4577b09a36c9d5/third_party/WebKit/LayoutTests/webaudio/internals/audiocontext-close.html
[modify] https://crrev.com/fbeb7bf25a794a2d482e56faab4577b09a36c9d5/third_party/blink/renderer/modules/webaudio/audio_context.cc
[modify] https://crrev.com/fbeb7bf25a794a2d482e56faab4577b09a36c9d5/third_party/blink/renderer/modules/webaudio/audio_context.h
[modify] https://crrev.com/fbeb7bf25a794a2d482e56faab4577b09a36c9d5/third_party/blink/renderer/modules/webaudio/base_audio_context.cc
[modify] https://crrev.com/fbeb7bf25a794a2d482e56faab4577b09a36c9d5/third_party/blink/renderer/modules/webaudio/base_audio_context.h

Status: Fixed (was: Started)

Sign in to add a comment