1175510 - Crash when running the DT2 demo

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

STR:
- http://beta.unity3d.com/jonas/DT2/
- click play (no matter which one)

Observed:
Crash!

Some crash reports of mine:
bp-1b86e5e5-c646-45dc-afdb-1ca7e2150617
bp-08aacd1c-6220-445e-85fd-d5d1c2150617

Someone tested that and confirmed the crash:
bp-6be4613f-2741-4278-bfd8-7f1542150617

Comment 1

[Alice0775 White]

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c0ef8de6093e&tochange=7d4c3379ecb3

Suspect: Bug 974089

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]:

Comment 3

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

Adding a tracking flag for FF41. This does not repro on FF40.0a2 (2015-06-03).

Added links in crash signature field.

Comment 5

[Paul Adenot (:padenot)]

Yeah we should null-check or something. I'll get to it on the plane tomorrow.

Comment 6

[Paul Adenot (:padenot)]

Attachment: Update the AudioBufferSourceNode <=> PannerNode mapping when destroying AudioNodeStream. r=

Comment 7

[Paul Adenot (:padenot)]

Attachment: Update the AudioBufferSourceNode <=> PannerNode mapping when destroying AudioNodeStream. r=

Oops we need to null-check here, this can be called from the CC.

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8625165 [details] [diff] [review]
Update the AudioBufferSourceNode <=> PannerNode mapping when destroying AudioNodeStream. r=

This approach looks good thanks, but should be adjusted to avoid adding inefficiency.

>+  // Maybe we are destroying an AudioBufferSourceNode's stream. Update the
>+  // panner <-> source mapping so we don't try to send the doppler to a
>+  // destroyed stream.
>+  if (Context()) {
>+    Context()->UpdatePannerSource();
>+  }

UpdatePannerSource() is not O(1) so this could be expensive if done for every
node.  It should be moved to AudioBufferSourceNode::DestroyMediaStream()
override so that it is not called more often than currently.

I would call UnregisterAudioBufferSourceNode() because UpdatePannerSource() is
an implementation detail.

The calls to UnregisterAudioBufferSourceNode() from ~AudioBufferSourceNode()
and the AudioBufferSourceNode unlink code can now be removed, because
DisconnectFromGraph() calls DestroyMediaStream(), and DisconnectFromGraph() is
called from AudioBufferSourceNode unlink code and from AudioNode::Release().

Comment 9

[Paul Adenot (:padenot)]

Attachment: Update the AudioBufferSourceNode <=> PannerNode mapping when destroying AudioNodeStream. r=

Is this what you had in mind ?

Comment 10

[Paul Adenot (:padenot)]

Attachment: Update the AudioBufferSourceNode <=> PannerNode mapping when destroying AudioNodeStream. r=

The previous fix was wrong and not complete. First I was updating the
panner/node mapping right before destroying the stream, so the changes would not
have been picked up.

Then, we are computing the panner/source mapping using node connections, and not
stream connections, because we're on the main thread, so we need to check if the
node has a stream when deciding if a node is to be registered (nodes that don't
have streams are finished, so it's okay to not adjust their doppler shift).

With this, I've verified that the crash in comment 0 is fixed.

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 8628608 [details] [diff] [review]
Update the AudioBufferSourceNode <=> PannerNode mapping when destroying AudioNodeStream. r=

>+    // Check if this node is an AudioBufferSourceNode that still have a stream,
>+    // which means it is still playing.

Can you reword this please so as not to imply that it has necessarily started playing?  Perhaps just "it has not finished playing".

Comment 12

[Karl Tomlinson (:karlt)]

crashtest in bug 1179662

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b10b297cec0

Comment 14

[Karl Tomlinson (:karlt)]

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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b10b297cec0

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 8628608 [details] [diff] [review]
Update the AudioBufferSourceNode <=> PannerNode mapping when destroying AudioNodeStream. r=

Approval Request Comment
[Feature/regressing bug #]: bug 974089
[User impact if declined]: crash running game
[Describe test coverage new/current, TreeHerder]: added new crashtest
[Risks and why]:
There is perhaps a small risk of ownership but the code is not so complex, so I don't think we've missed anything.
[String/UUID change made/needed]: none.

Comment 17

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

Comment on attachment 8628608 [details] [diff] [review]
Update the AudioBufferSourceNode <=> PannerNode mapping when destroying AudioNodeStream. r=

Approving for uplift to Aurora. A crash test was added to verify the fix.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c41b3fd3ff67