Testcase needs location.reload() to trigger the assertion. This assertion gets called repeatedly and messes with the output of upcoming testcases made by the fuzzer -- this is not caused by the recursion through location.reload() in the testcase. Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/fbcd6734691f

Josh, can you please take a look?

Interestingly it is not crashing with my MacOS build but on Linux it crashes very fast. The stack is different with each crash.

Comment on attachment 763189 [details] testcase Setting as text plain so I don't crash my browser when opening it :-)

So, this is cause by the fact that we use an OfflineAudioContext at 44100Hz. If I modify the test case to use 48000Hz, it works just fine. In fact, I'm sure sure if it is actually allowed for now, OfflineAudioContext should throw if the rate is not 48000Hz. Maybe this is just a matter of throwing the assertion for now.

Oh, right. So we talked about this at one of the Audio WG teleconfs. We should make the IDL return value for createMediaStreamDestination() return nullable, and return null if called on an OAC with a "non-standard" sampling rate. Can you please post on public-audio to remind crogers to spec this? Thanks!

Maybe I can needinfo myself as a reminder?

If this is a spec error does that mean it's not a vulnerability? Or do we need to change our code to protect against this illegal value?(In reply to Paul Adenot (:padenot) from comment #4) > Setting as text plain so I don't crash my browser when opening it :-) Get the bugzilla Tweaks addon which adds a "source" link next to attachments. That way people can use the attachment as a testcase in place, but you can also easily open it for inspection. (also the Details link will open HTML safely but you don't get as much real estate to view it.)

I need to investigate more, there are fishy things here, like use-after-free of messages. Seems to be related to some kind of race because when I put printfs to log, the use-after-free went away. What we could do is to throw for now when the user specifies a sample rate that we don't support, so it gives us time to investigate. This only buys us time.

What does the stack look like here? This might indeed be specific to OAC. In that case, we should just throw, and we won't need to worry about this bug beyond that.

There are a couple different stacks for this testcase, maybe three. I'll post them tomorrow.

needinfoing for the stacks

Marking critical per comment 9.

For some reason, I can't repro the use-after-free and other ASAN errors, but I get this nice assert, which is kind of expected. I think we should throw instead of all this mess.

:padenot, based on comment 4 I assumed that you can reproduce the crash. Please see also comment 3.

(In reply to Paul Adenot (:padenot) from comment #14) > Created attachment 772059 [details] > stack > > For some reason, I can't repro the use-after-free and other ASAN errors, but > I get this nice assert, which is kind of expected. I think we should throw > instead of all this mess. Agreed.

Christoph, I used to be able to repro with at least 3 different stacks, and iirc I have not updated this tree, and I can't repro anymore. Not sure why.

Comment on attachment 772084 [details] [diff] [review] Throw when creating an MediaStreamDestination node on an OfflineAudioContext Review of attachment 772084 [details] [diff] [review]: ----------------------------------------------------------------- Can you please post to public-audio to make sure that we spec this? Thanks!

I was thinking about that again the other day, and I was wondering if using a MediaStreamDestination node on an OfflineAudioContext would make sense with the new MediaRecorder API? In which case we don't want this patch.

I think we should take this patch for now and later figure out our story around MediaStreams and OfflineAudioContext. It's not trivial because we'll need to define some kind of offline MediaStream and that will require spec work.

How did this land on Aurora before it was checked into trunk? Bugs need explicit aurora approval from release management before landing there and they need to be checked into trunk first. Also, this bug affects more than one branch so it should have gotten sec-approval before going in. https://wiki.mozilla.org/Security/Bug_Approval_Process

(In reply to Al Billings [:abillings] from comment #24) > How did this land on Aurora before it was checked into trunk? Bugs need > explicit aurora approval from release management before landing there and > they need to be checked into trunk first. Web Audio is being developed pretty much in parallel on trunk and Aurora. This has been discussed on release-drivers, sorry there are no public archives to link to. > Also, this bug affects more than one branch so it should have gotten > sec-approval before going in. > > https://wiki.mozilla.org/Security/Bug_Approval_Process This fix wasn't really a security fix, we just disabled code which used to hit this problem...

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #25) > Web Audio is being developed pretty much in parallel on trunk and Aurora. > This has been discussed on release-drivers, sorry there are no public > archives to link to. I understand this but, still, fixes that aren't branch only don't normally go into branches before they go into trunk and trunk is green. Aurora fixes don't go in without approval from Release Management. From what you say above, I can't tell if this had Aurora approval from them. > This fix wasn't really a security fix, we just disabled code which used to > hit this problem... Well, the bug is sec-critical here and gets tracked all over as such. Does that mean we should open this bug to the public now?

(In reply to Al Billings [:abillings] from comment #26) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #25) > > > Web Audio is being developed pretty much in parallel on trunk and Aurora. > > This has been discussed on release-drivers, sorry there are no public > > archives to link to. > > I understand this but, still, fixes that aren't branch only don't normally > go into branches before they go into trunk and trunk is green. There are sometimes good reasons to wait (such as when you're not sure if the patch is the right fix, or where you're afraid of regressions, etc.) In other cases that is just unnecessary formality which slows people down. > Aurora fixes > don't go in without approval from Release Management. From what you say > above, I can't tell if this had Aurora approval from them. Sorry for the confusion, these patches are pre-approved. > > This fix wasn't really a security fix, we just disabled code which used to > > hit this problem... > > Well, the bug is sec-critical here and gets tracked all over as such. Does > that mean we should open this bug to the public now? Yes, I don't see why we can't do that.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #25) > This fix wasn't really a security fix, we just disabled code which used to > hit this problem... ... the problem which was almost certainly exploitable (e.g. bug 883938)? Sounds like a "security fix" to me. But sure, since the fix is now available on every branch that has the feature we don't need to keep the bug hidden.

This bug had its patch landed on every needed branch, closing.