808374 - Crash on shutdown after mozAudioContext().createBuffer()

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Jesse Ruderman]

Attachment: testcase

With:
  user_pref("media.webaudio.enabled", true);

The testcase causes a crash during shutdown.

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Scoobidiver (away)]

On Windows: bp-7fa18288-bcdc-4686-be9a-c5fe92121104

Comment 3

[Boris Zbarsky [:bzbarsky]]

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...

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Shouldn't we throw in this case?
It makes no sense to create a buffer with 0 channels.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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...

Comment 6

[Andrew McCreight [:mccr8]]

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).

Comment 7

[Andrew McCreight [:mccr8]]

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.

Comment 8

[Andrew McCreight [:mccr8]]

> then only update the global state in the nsContentUtils function
...only update the global state if the XPCJSR drop succeeds, that is.

Comment 9

[(no longer active)]

Attachment: Patch (v1)

I'll raise a spec issue about the 0-channel madness.

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 678391 [details] [diff] [review]
Patch (v1)

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

woot!

Comment 11

[Andrew McCreight [:mccr8]]

Hmm.  Okay, maybe this isn't quite right. The JS pointers need to be nulled out.

Comment 12

[Andrew McCreight [:mccr8]]

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.

Comment 13

[Andrew McCreight [:mccr8]]

(well, the unlink plus the wrapper cache magic)

Comment 14

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc191fe62b43

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bc191fe62b43

Comment 16

[(no longer active)]

Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.

Comment 17

[User Dderss]

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

Comment 18

[Jesse Ruderman]

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.)