Closed Bug 894150 Opened 6 years ago Closed 6 years ago

Crash with AudioListener.setOrientation after GC

Categories

(Core :: Web Audio, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jruderman, Assigned: padenot)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, crash, testcase)

Crash Data

Attachments

(4 files)

Attached file stack
Assertion failure: ns (How come we don't have a stream here?), at content/media/webaudio/AudioNode.cpp:240
On Windows: bp-a5c83b8a-b594-4a28-85f8-d1f642130716.
Crash Signature: [@ mozilla::AudioNodeStream::SetThreeDPointParameter(unsigned int, mozilla::dom::ThreeDPoint const&) ]
OS: Mac OS X → All
Hardware: x86_64 → All
What happens here is that we don't keep a reference to the PannerNode, so it is
collected, but we forgot to unregister it from the AudioListener, so we ended up
trying to send data to a non-existent stream.

This simply unregisters the PannerNode to the context that forwards it to the
listener when the stream is destroyed, so the listener won't try to send the
data to the soon-to-be-destroyed PannerNode.
Attachment #776358 - Flags: review?(ehsan)
Assignee: nobody → paul
Comment on attachment 776358 [details] [diff] [review]
Unregister the PannerNode in the AudioListener when destroying the stream. r=

Review of attachment 776358 [details] [diff] [review]:
-----------------------------------------------------------------

Also, please include the test case here as a mochitest when landing the patch.  You can use SpecialPowers.forceGC() to trigger gc (see test_bug866737.html for example.)

::: content/media/webaudio/AudioContext.cpp
@@ +399,5 @@
>  void
>  AudioContext::UnregisterPannerNode(PannerNode* aNode)
>  {
>    mPannerNodes.RemoveEntry(aNode);
> +  mListener->UnregisterPannerNode(aNode);

Please null-check mListener here, as we lazily create it.
Attachment #776358 - Flags: review?(ehsan) → review+
Attached patch Test.Splinter Review
(converted tests)
Looks like you missed the null pointer check I asked for.  :-)
Relanded with the qref :-) (tested locally, does not crash anymore).

https://hg.mozilla.org/integration/mozilla-inbound/rev/31b881b4b960
https://hg.mozilla.org/mozilla-central/rev/31b881b4b960
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Duplicate of this bug: 895196
You need to log in before you can comment on or make changes to this bug.