New issue
Advanced search Search tips

Issue 858834 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

Potential layout test leak in AudioContext uninitializing phase

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

Issue description

From CL: https://chromium-review.googlesource.com/c/chromium/src/+/1115516

Without DidClose() method in AudioContext::Uninitialize(), the dom-exceptions.html
layout test leaks. Investigate why.
 
Owner: acomminos@fb.com
Status: Started (was: Available)
The first call to DeferredTaskHandler::AddRenderingOrphanHandler in AudioNode::Dispose (https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/webaudio/audio_node.cc?l=626&rcl=ca8eefb7e1bd33d8a49617fe45d3b7ef0ecd9e4e) appears to be the source of leakage here.

I'm going to continue investigating why the context isn't cleaning these up in BaseAudioContext::Uninitialize- I suspect that they're being added too late.
Thanks for looking into this!

> the dom-exceptions.html layout test leaks

How did you know about the leakage? Was it found via Blink leak detector?
> How did you know about the leakage? Was it found via Blink leak detector?

The --enable-leak-detection flag for run_web_tests.py provided the context needed.

The actual source of the leak was found by grepping for the AudioContextState enum. Since the leak was only reproducing when context_state_ was not kClosed, there were only a few places that actually checked that raw value that could possibly leak nodes.

I've posted a CL to check for the uninitialized state instead (https://chromium-review.googlesource.com/c/chromium/src/+/1284697). Thanks!
Thanks! I made some comments on the CL. We can continue to discuss there.
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 1

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

commit 56d152ff60b364132d7caf2f456c0d06bf2cfe6f
Author: Andrew Comminos <acomminos@fb.com>
Date: Thu Nov 01 17:39:33 2018

Avoid adding orphaned nodes to a task handler on a closed audio context.

If a BaseAudioContext is uninitialized, its render thread will no longer
clear associated orphaned render nodes (as it is stopped). Instead of
checking whether or not the context has reported itself as being closed
(which is independent of the initialization state), simply check the
initialization state itself.

R=hongchan@chromium.org

Bug:  858834 
Change-Id: I5b088d17122b0c86270ff8ca75b13ab1487a2131
Reviewed-on: https://chromium-review.googlesource.com/c/1284697
Commit-Queue: Andrew Comminos <acomminos@fb.com>
Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604630}
[modify] https://crrev.com/56d152ff60b364132d7caf2f456c0d06bf2cfe6f/third_party/blink/renderer/modules/webaudio/audio_node.cc

Status: Fixed (was: Started)
Closing, audio handler orphaning logic now corrected.

Sign in to add a comment