Comment on attachment 771042 [details] [diff] [review] Part 2: Keep the ConvolverNode alive while its reverb buffers are being exhausted Review of attachment 771042 [details] [diff] [review]: ----------------------------------------------------------------- OK, but I have one question: Can the following happen? -- A ConvolverNode is created and some inputs are provided, and things run normally for a while. -- All inputs removed from ConvolverNode -- On audio thread, buffers drain and a PlayingRefChanged(RELEASE) is queued -- On main thread, a new input node is added. -- On audio thread, new input is received and a PlayingRefChanged(ADDREF) is queued -- On main thread, input is removed. -- On main thread, PlayingRefChanged(RELEASE) runs and mPlayingRef is dropped. -- On main thread, cycle collection happens and the ConvolverNode goes away prematurely Basically, it seems to me PlayingRefChanged events can race with main-thread stuff in a way that means the ADDREF comes too late to keep the node alive.

I think the solution here probably requires using MediaStreamGraph::GetCurrentGraphUpdateIndex and related state. We need to keep track of the last graph update that added an input node to the ConvolverNode. Then in the PlayingRefChanged event, we can store the index of the last graph update that was processed. If that PlayingRefChanged event was fired before the adding of the input node was processed by the MSG, then it should be ignored.

Yeah, I guess you're right about the race condition. Paul, can you please look into implementing comment 4?

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed650741dc5 https://hg.mozilla.org/integration/mozilla-inbound/rev/ff011e5b3655 These two patches need to land on Aurora, a=webaudio.

This broke the build, because gcc with -fpermissive issues a warning which we treat as an error :( https://hg.mozilla.org/integration/mozilla-inbound/rev/4937e561e40d https://tbpl.mozilla.org/php/getParsedLog.php?id=24914612&tree=Mozilla-Inbound#error0

https://hg.mozilla.org/integration/mozilla-inbound/rev/7151ab8ff70c https://hg.mozilla.org/integration/mozilla-inbound/rev/5e815a5ca50d

Backed out again since apparently where it builds, it will leak in mochitest-1. https://hg.mozilla.org/integration/mozilla-inbound/rev/4afa33be1fce I'm afraid I won't have enough time to work on this more in the near future. Can somebody please pick it up?

I'll take this since I need it fixed to land my neutering patch...

The previous version of part 2 had some problems. In particular, if aInput.IsNull(), we never decremented mLeftOverData, which means we'd never fire the RELEASE message.

Comment on attachment 771291 [details] [diff] [review] part 2 v2 Review of attachment 771291 [details] [diff] [review]: ----------------------------------------------------------------- (Please rebase on top of bug 890023 which just landed on inbound)

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae3cbb0dd69f https://hg.mozilla.org/integration/mozilla-inbound/rev/987fd0f14e76

https://hg.mozilla.org/releases/mozilla-aurora/rev/f12e9f364c3a https://hg.mozilla.org/releases/mozilla-aurora/rev/f6e678faa38c

https://hg.mozilla.org/mozilla-central/rev/ae3cbb0dd69f https://hg.mozilla.org/mozilla-central/rev/987fd0f14e76

Assuming no verification needed here. Please add the verifyme keyword and remove the [qa-] whiteboard tag to request verification.