910174 - DelayNode output can be cut off early when it is GCed

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Karl Tomlinson (:karlt)]

1. Load the testcase in attachment 771655 [details].
2. Click to play
3. Wait

Expected results:
PlayingRefChange code in DelayNodeEngine sends ADDREF to the node.

Actual results:
The DelayNodeEngine is destroyed without any ADDREF sent.

IIUC this is because aStream->AllInputsFinished() doesn't return true.

If this did ever return true, I wonder whether this could be after the
DelayNode is destroyed anyway, which would be too late for the
PlayingRefChange code to take a reference.

Also, as mentioned in bug 898291 comment 2, the Node may be destroyed even
before the first ProduceAudioBlock() call.

Comment 1

[Karl Tomlinson (:karlt)]

I wonder whether the Engine could take a (self-ish) reference to its owning
Stream.  We don't need to keep the Node alive, only the Engine, and its output
streams.  But I guess that would require MSG redesign to keep the downstream
Engines alive and deal with cycles.  And we still need a way to ensure that the self reference is dropped (bug 910171).

Comment 2

[(no longer active)]

Why does AllInputsFinished() not return true?  That seems like a bug that we should fix.

Comment 3

[Karl Tomlinson (:karlt)]

AudioBufferSourceNode can set aFinished in ProduceAudioBlock when it is done.  I don't think many other nodes can do that because they may be reconnected to another source.

Comment 4

[(no longer active)]

(In reply to comment #3)
> AudioBufferSourceNode can set aFinished in ProduceAudioBlock when it is done. 
> I don't think many other nodes can do that because they may be reconnected to
> another source.

Yeah, that's true.  Not all nodes actually know when they're finished, that is the information that the AudioContext has (for the offline case, that is.)

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: add DelayNode's tail-time reference as soon as the engine receives sound

This makes the DelayNode logic similar to ConvolverNode.

It removes the dependence on AllInputsFinished() which didn't return true
for many input types.

The DelayProcessor is no longer continuously reset (bug 921457) and the
reference is now correctly added again when all inputs are finished and then
new inputs are connected.

This depends on the patch in bug 921675 because the DelayProcessor hasn't calculated the current delay time when sound is first received.

Comment 6

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb856e6d57c1

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 811445 [details] [diff] [review]
add DelayNode's tail-time reference as soon as the engine receives sound

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new feature: Web Audio DelayNode
User impact if declined: 
Bug 921457 is the most prominent bug fixed here.  That does not depend on GC running, but means that in some circumstances a DelayNode will not play sounds after its first sound is finished.  The precise circumstances leading to the bug would be difficult for an author to diagnose.

Testing completed (on m-c, etc.): on m-i, in testsuite
Risk to taking this patch (and alternatives if risky):
The patch only modifies behavior of Web Audio's DelayNode so risk is confined to that.

String or IDL/UUID changes made by this patch: none

This patch needs the patch in bug 921675.

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cb856e6d57c1

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 811445 [details] [diff] [review]
add DelayNode's tail-time reference as soon as the engine receives sound

low risk cleanup of a new web feature

Comment 10

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7dfe4a775531
https://hg.mozilla.org/releases/mozilla-beta/rev/72473eb035c2

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #0)
> Also, as mentioned in bug 898291 comment 2, the Node may be destroyed even
> before the first ProduceAudioBlock() call.

Bug 923301 covers that.