1179662 - crash in MediaStream::AddConsumer with SetVelocity()/Doppler after source node finishes

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Karl Tomlinson (:karlt)]

#0  0x00007fe011e1e186 in nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::Length (this=0x128) at ../../dist/include/nsTArray.h:361
#1  0x00007fe0146023b3 in nsTArray_Impl<mozilla::MediaInputPort*, nsTArrayInfallibleAllocator>::AppendElement<mozilla::MediaInputPort*&, nsTArrayInfallibleAllocator> (this=0x128, aItem=@0x7fdf568fe8c0: 0x7fdfaa9c2a30)
    at ../../dist/include/nsTArray.h:1515
#2  0x00007fe0145ff734 in mozilla::MediaStream::AddConsumer (this=0x0, 
    aPort=0x7fdfaa9c2a30)
    at /mnt/ssd1/karl/moz/dev/dom/media/MediaStreamGraph.h:503
#3  0x00007fe0145ef209 in mozilla::MediaInputPort::Init (this=0x7fdfaa9c2a30)
    at /mnt/ssd1/karl/moz/dev/dom/media/MediaStreamGraph.cpp:2769
#4  0x00007fe0145ef648 in mozilla::ProcessedMediaStream::Message::Run (
    this=0x7fdf51daee80)
    at /mnt/ssd1/karl/moz/dev/dom/media/MediaStreamGraph.cpp:2868
#5  0x00007fe0145ea9f2 in mozilla::MediaStreamGraphImpl::UpdateGraph (
    this=0x7fdfaabb15c0, aEndBlockingDecision=39424)
    at /mnt/ssd1/karl/moz/dev/dom/media/MediaStreamGraph.cpp:1359
#6  0x00007fe0145eb18e in mozilla::MediaStreamGraphImpl::OneIteration (
    this=0x7fdfaabb15c0, aFrom=38426, aTo=38917, aStateFrom=39040, 
    aStateEnd=39424)
    at /mnt/ssd1/karl/moz/dev/dom/media/MediaStreamGraph.cpp:1507
#7  0x00007fe014519c8d in mozilla::AudioCallbackDriver::DataCallback (
    this=0x7fdfed4e0000, aBuffer=0x7fdf49eae040, aFrames=448)
    at /mnt/ssd1/karl/moz/dev/dom/media/GraphDriver.cpp:902
#8  0x00007fe014519821 in mozilla::AudioCallbackDriver::DataCallback_s (
    aStream=0x7fdf51c97510, aUser=0x7fdfed4e0000, aBuffer=0x7fdf49eae040, 
    aFrames=448) at /mnt/ssd1/karl/moz/dev/dom/media/GraphDriver.cpp:743
#9  0x00007fe015d1bc23 in stream_request_callback (s=0x7fdf51ca7000, 
    nbytes=3584, u=0x7fdf51c97510)
    at /mnt/ssd1/karl/moz/dev/media/libcubeb/src/cubeb_pulse.c:198

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: crashtest

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: uninline DestroyMediaStream overrides so that mStream type need not be complete

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: specify AudioNode::mStream as AudioNodeStream

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: rename AudioNode::Stream() to GetStream() as it may return null

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: handle null AudioNodeStream stream

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: call UnregisterAudioBufferSourceNode only once

Because these nodes are only registered on PannerNodes if they have a
stream, they need only be unregistered once after the stream is destroyed.

Previously unregistration happened on finish, unlink, and destructor.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: call DisconnectFromGraph once only during unlink

Comment 8

[Paul Adenot (:padenot)]

Comment on attachment 8628661 [details] [diff] [review]
crashtest

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

You're landing this, right?

Comment 9

[Paul Adenot (:padenot)]

(try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=08b2ae4af477)

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/950835c3c86f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6672a6212e3
https://hg.mozilla.org/integration/mozilla-inbound/rev/23b96ee6b007
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b56fb5bde8c
https://hg.mozilla.org/integration/mozilla-inbound/rev/925c04c394e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f969a016248
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bbcd0482756

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to Paul Adenot (:padenot) from comment #8)
> You're landing this, right?

Yes.  https://hg.mozilla.org/integration/mozilla-inbound/rev/925c04c394e7

Comment 12

[Karl Tomlinson (:karlt)]

[Tracking Requested - why for this release]:
Recently introduced crash.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/950835c3c86f
https://hg.mozilla.org/mozilla-central/rev/b6672a6212e3
https://hg.mozilla.org/mozilla-central/rev/23b96ee6b007
https://hg.mozilla.org/mozilla-central/rev/9b56fb5bde8c
https://hg.mozilla.org/mozilla-central/rev/925c04c394e7
https://hg.mozilla.org/mozilla-central/rev/0f969a016248
https://hg.mozilla.org/mozilla-central/rev/9bbcd0482756

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 8628666 [details] [diff] [review]
handle null AudioNodeStream stream

Approval Request Comment

Please consider this an approval request for all patches on the bug.
The patches apply on top of changes in bug 1175510.

[Feature/regressing bug #]: bug 974089
[User impact if declined]:
Potential crash.  The is probably not likely in typical Web Audio usage, but some slightly out of ordinary usage could trip this.
[Describe test coverage new/current, TreeHerder]: new crashtest.
[Risks and why]: 
There is perhaps a small risk of ownership issues but the changes are not complex, so I don't think we've missed anything.  There are many existing tests.
[String/UUID change made/needed]: none.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8628666 [details] [diff] [review]
handle null AudioNodeStream stream

Approving uplift as there is a crash test.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a30fae55009e
https://hg.mozilla.org/releases/mozilla-aurora/rev/acc469211333
https://hg.mozilla.org/releases/mozilla-aurora/rev/eb419b4b1910
https://hg.mozilla.org/releases/mozilla-aurora/rev/856dd8d4b6f6
https://hg.mozilla.org/releases/mozilla-aurora/rev/54c223db6470
https://hg.mozilla.org/releases/mozilla-aurora/rev/6e0a8c777927
https://hg.mozilla.org/releases/mozilla-aurora/rev/d7eea16d75d9