917260 - Nullcheck Destination() in AudioContext::DestinationStream

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Comment 1

[Paul Adenot (:padenot)]

Attachment: Nullcheck Destination() in AudioContext::DestinationStream. r=

Comment 2

[Paul Adenot (:padenot)]

Attachment: stacktrace

(gdb) down
#2  Stream (this=0x0) at ../../../dist/include/mozilla/dom/AudioNode.h:209
209	  MediaStream* Stream() { return mStream; }
(gdb) p *this
Cannot access memory at address 0x0

Also, the AudioContext is always offline, and the AudioContext seem to have just been initialized (nothing in the hashtables, no destination, no listener, no decode jobs/decoder).

Comment 3

[(no longer active)]

Comment on attachment 805942 [details] [diff] [review]
Nullcheck Destination() in AudioContext::DestinationStream. r=

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

Seems like the AudioContext object has been unlinked.

Comment 4

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4323508b3412

Comment 5

[Paul Adenot (:padenot)]

Comment on attachment 805942 [details] [diff] [review]
Nullcheck Destination() in AudioContext::DestinationStream. r=

[Approval Request Comment]
Bug caused by (feature/regressing bug #): not clear, this seem to depend on the cc timing
User impact if declined: crash
Testing completed (on m-c, etc.): locally
Risk to taking this patch (and alternatives if risky): this is a nullcheck that should have been done anyways. If not taken, it will crash.
String or IDL/UUID changes made by this patch: none.

Comment 6

[Ed Morley [:emorley]]

The push this bug was in + followups have been backed out for crashes of form:
https://tbpl.mozilla.org/php/getParsedLog.php?id=27977619&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=27976043&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/58dbd84ae828
https://hg.mozilla.org/integration/mozilla-inbound/rev/9524b1df278e
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb89f2090779
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe576415129e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e5d5e42f6c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/b221626331b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aa38a1a2b06
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d2aac8dc618
https://hg.mozilla.org/integration/mozilla-inbound/rev/ade49a801461
https://hg.mozilla.org/integration/mozilla-inbound/rev/4323508b3412

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #3)
> Seems like the AudioContext object has been unlinked.

I think you are right.  This is probably from bug 914030, which we want on Beta for bug 910171.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: remove AudioContext weak ref from window in Unlink()

Should objects remove weak references when they are unlinked, like so?

This means that Shutdown won't be called after the context is unlinked.
The context and the destination form a cycle, so the destination won't be
destroyed before the context is unlinked.

I'm happy to go with the null check instead, if you prefer that solution.

It's unfortunate to remove the context from the window in 2 places.  I expect
Unlink() is always called before the destructor for this class because
AudioContext always forms a cycle with the destination, but that's a bit of a
distant link.  I'd prefer to leave the Remove call in the destructor also in
case ownership changes one day.

Comment 9

[Karl Tomlinson (:karlt)]

I would have expected the AudioContext to be destroyed during CC if Unlink is called an that would be before nsGlobalWindow::SetNewDocument() could be called, but perhaps the destroy doesn't always happen in the same CC iteration as the Unlink?

Comment 10

[Karl Tomlinson (:karlt)]

If we take the null check approach, then we'll also need

-  if (mIsOffline) {
+  if (mIsOffline && mDestination) {
     mDestination->OfflineShutdown();
   }

later in AudioContext::Shutdown().

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 806328 [details] [diff] [review]
remove AudioContext weak ref from window in Unlink()

I'm preferring the null-check solution.
The role of unlink is to remove strong references, and clearing week references can be done in a single place in the destructor.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: null check mDestination in AudioContext::Shutdown()

Comment 13

[(no longer active)]

Comment on attachment 806536 [details] [diff] [review]
null check mDestination in AudioContext::Shutdown()

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

Yeah, I like the null check approach better too.  Thanks!

Comment 14

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23a83ccc8c93
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d158763f7b9

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 806536 [details] [diff] [review]
null check mDestination in AudioContext::Shutdown()

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 914030
User impact if declined: Can't land bug 914030 on 25.  null deref crash on 26.
Testing completed (on m-c, etc.): on m-i
Risk to taking this patch (and alternatives if risky): none - null check
String or IDL/UUID changes made by this patch: none

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/23a83ccc8c93
https://hg.mozilla.org/mozilla-central/rev/6d158763f7b9

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 805942 [details] [diff] [review]
Nullcheck Destination() in AudioContext::DestinationStream. r=

I'm going to assume these are both necessary. If not, please ping me asap, as they'll be uplifted shortly.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a242430fb69a
https://hg.mozilla.org/releases/mozilla-aurora/rev/e5c91b47542d

https://hg.mozilla.org/releases/mozilla-beta/rev/902feb79b477
https://hg.mozilla.org/releases/mozilla-beta/rev/859717cd6229

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

That's pretty unfortunate, because Destination() follows the code style for "never returns null"

Comment 20

[(no longer active)]

(In reply to comment #19)
> That's pretty unfortunate, because Destination() follows the code style for
> "never returns null"

That's only going to be the case if it's called through Web IDL.