Closed Bug 917260 Opened 6 years ago Closed 6 years ago

Nullcheck Destination() in AudioContext::DestinationStream

Categories

(Core :: Web Audio, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox25 --- fixed
firefox26 --- fixed
firefox27 --- fixed

People

(Reporter: padenot, Assigned: padenot)

References

Details

(Whiteboard: [qa-])

Attachments

(3 files, 1 obsolete file)

No description provided.
Summary: Nullcheck Destination() in AudioContext::DestinationStream. r= → Nullcheck Destination() in AudioContext::DestinationStream
Attached file 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 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.
Attachment #805942 - Flags: review+
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.
Attachment #805942 - Flags: approval-mozilla-beta?
Attachment #805942 - Flags: approval-mozilla-aurora?
(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.
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.
Attachment #806328 - Flags: review?(ehsan)
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?
If we take the null check approach, then we'll also need

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

later in AudioContext::Shutdown().
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.
Attachment #806328 - Attachment is obsolete: true
Attachment #806328 - Flags: review?(ehsan)
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!
Attachment #806536 - Flags: review?(ehsan) → review+
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
Attachment #806536 - Flags: approval-mozilla-beta?
Attachment #806536 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/23a83ccc8c93
https://hg.mozilla.org/mozilla-central/rev/6d158763f7b9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
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.
Attachment #805942 - Flags: approval-mozilla-beta?
Attachment #805942 - Flags: approval-mozilla-beta+
Attachment #805942 - Flags: approval-mozilla-aurora?
Attachment #805942 - Flags: approval-mozilla-aurora+
Attachment #806536 - Flags: approval-mozilla-beta?
Attachment #806536 - Flags: approval-mozilla-beta+
Attachment #806536 - Flags: approval-mozilla-aurora?
Attachment #806536 - Flags: approval-mozilla-aurora+
That's pretty unfortunate, because Destination() follows the code style for "never returns null"
Whiteboard: [qa-]
(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.
You need to log in before you can comment on or make changes to this bug.