Closed Bug 910174 Opened 6 years ago Closed 6 years ago

DelayNode output can be cut off early when it is GCed

Categories

(Core :: Web Audio, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox24 --- unaffected
firefox25 --- fixed
firefox26 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

Details

Attachments

(1 file)

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.
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).
Why does AllInputsFinished() not return true?  That seems like a bug that we should fix.
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.
(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.)
Assignee: nobody → karlt
Blocks: 921457
Depends on: 921675
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.
Attachment #811445 - Flags: review?(ehsan)
Attachment #811445 - Flags: review?(ehsan) → review+
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.
Attachment #811445 - Flags: approval-mozilla-beta?
Attachment #811445 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/cb856e6d57c1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
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
Attachment #811445 - Flags: approval-mozilla-beta?
Attachment #811445 - Flags: approval-mozilla-beta+
Attachment #811445 - Flags: approval-mozilla-aurora?
Attachment #811445 - Flags: approval-mozilla-aurora+
(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.
You need to log in before you can comment on or make changes to this bug.