Crash on shutdown after mozAudioContext().createBuffer()

RESOLVED FIXED in mozilla19

Status

()

defect
--
critical
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: jruderman, Assigned: Ehsan)

Tracking

(Blocks 1 bug, {crash, testcase})

Trunk
mozilla19
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(3 attachments)

Reporter

Description

7 years ago
Posted file testcase
With:
  user_pref("media.webaudio.enabled", true);

The testcase causes a crash during shutdown.
Reporter

Comment 1

7 years ago
Posted file stack
On Windows: bp-7fa18288-bcdc-4686-be9a-c5fe92121104
Crash Signature: [@ nsXPCOMCycleCollectionParticipant::CheckForRightISupports] → [@ nsXPCOMCycleCollectionParticipant::CheckForRightISupports(nsISupports*)] [@ js::GCThingTraceKind(void*)]
OS: Mac OS X → All
Hardware: x86_64 → All
Oh, cute.  In that testcase, we enter AudioBuffer::InitializeBuffers, call NS_HOLD_JS_OBJECTS, then don't add anything to mChannels.  So our destructor never calls NS_DROP_JS_OBJECTS.

What we should probably do is hold only after we add the first element to the array or something...
Shouldn't we throw in this case?
It makes no sense to create a buffer with 0 channels.
I agree it makes no sense.  Spec says nothing about throwing...

But more importantly, we'd get the same failure mode if we fail to SetCapacity or to insert the first object, no?  It wouldn't matter that we threw, since we would have done the HOLD already...
There are a bunch of edge cases here for the current DROP condition, that maybe can't occur with the current usage of this class. What do you do if somebody initializes an AudioBuffer more than once?  That could eliminate all of the channels, so we'll miss the drop. What happens if inserting the first object succeeds, but a later one fails? If we HOLD at the end of the function, then we'll have a dangling pointer (though with the current usage, that is okay because we don't use the object after failure).
Can we just HOLD unconditionally in the constructor and DROP unconditionally in the destructor? Looking at the code, it seems like in practice what usually happens is that we call the constructor, then initialize it with a non-zero number of buffers, so we'll basically always need to call HOLD.

I wonder if I should change the behavior of DROP to allow failure in XPCJSRuntime in the case where something isn't in the table, then only update the global state in the 
nsContentUtils function. I've seen bloat logs where the HOLD-DROP count has underflowed, which would happen if we DROP too much.
> then only update the global state in the nsContentUtils function
...only update the global state if the XPCJSR drop succeeds, that is.
Assignee

Comment 9

7 years ago
Posted patch Patch (v1)Splinter Review
I'll raise a spec issue about the 0-channel madness.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #678391 - Flags: review?(continuation)
Comment on attachment 678391 [details] [diff] [review]
Patch (v1)

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

woot!
Attachment #678391 - Flags: review?(continuation) → review+
Hmm.  Okay, maybe this isn't quite right. The JS pointers need to be nulled out.
Attachment #678391 - Flags: review+ → review-
Comment on attachment 678391 [details] [diff] [review]
Patch (v1)

The problem that I was concerned about was that we could trigger Unlink without nulling out all of the pointers to JS, so we'd end up keeping JS alive, which maybe could cause us to leak. But the |NS_IMPL_CYCLE_COLLECTION_UNLINK_NSCOMARRAY(mChannels)| deletes all the JS pointers so it should be okay!  My apologies.
Attachment #678391 - Flags: review- → review+
(well, the unlink plus the wrapper cache magic)
https://hg.mozilla.org/mozilla-central/rev/bc191fe62b43
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Assignee

Comment 16

6 years ago
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio

Comment 17

5 years ago
I got this error by just clicking on a link (to reliable site) in a electronic mail. And it is not non-reproducible, totally random:
https://crash-stats.mozilla.com/report/index/600b3122-91be-4fed-8769-5398a2140512
Reporter

Comment 18

5 years ago
zxspectrum3579, sometimes we can trace down crashes based on stacks, but that tends not to be the case for crashes in the JS GC. Sorry.

(In the future, please file new bugs instead of commenting on old FIXED bugs.)
You need to log in before you can comment on or make changes to this bug.