842089 - Crash with event listeners on mozCaptureStream()

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

For the last day or so, I've been getting lots of assertions/crashes involving mozCaptureStream() and event listeners.  Here's one of them.

Comment 1

[Scoobidiver (away)]

It might be related to bug 842079.

Comment 2

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

I don't understand this. mozCaptureStream returns nsIDOMMediaStream object, and that isn't
an EventTarget - as far as I see. Yet we try to handle it as an EventTarget?

Comment 3

[Boris Zbarsky [:bzbarsky]]

> mozCaptureStream returns nsIDOMMediaStream object, and that isn't
> an EventTarget - as far as I see.

You may be looking in the wrong place (like mxr for mozilla-central).  It doesn't seem to show the code after bug 837034 happened.

The WebIDL checked in for that bug has:

>+interface MediaStream : EventTarget {

but the C++ class has:

>+class DOMMediaStream : public nsIDOMMediaStream,
>+                       public nsWrapperCache

which is very much not inheriting from EventTarget.  So the EventTarget things end up on the proto chain, reinterpret_cast to EventTarget, call virtual methods, and we get this security bug.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Our MDC page for MediaStream doesn't seem to have any useful spec links, so I can't tell whether it's correct that this is an EventTarget or not.  If it is, then it needs to actually do EventTarget bits...

Comment 5

[Andrew McCreight [:mccr8]]

Using the wrong vtable sounds bad, so marking sec-critical.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We don't implement any EventTarget stuff yet, so I guess we just need to remove EventTarget from MediaStream.webidl.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Or inherit from nsDOMEventTargetHelper get all the EventTarget bits working, yes.  For 21, changing the WebIDL is probably safer.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It seems to me that this kind of mistake should cause a build failure.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 715004 [details] [diff] [review]
fix

r=me

> It seems to me that this kind of mistake should cause a build failure.

Yeah.

I might in fact be able to turn this into a build failure, and some related problems into runtime fatal asserts that will always trigger (we have some asserts along those lines, but they only trigger in certain limited cases which don't apply here).  I'll poke at that on Tuesday.

Comment 11

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

(In reply to Boris Zbarsky (:bz) from comment #3)
> You may be looking in the wrong place (like mxr for mozilla-central).  It
> doesn't seem to show the code after bug 837034 happened.

Uh, indeed. mxr hasn't still been updated...

Comment 12

[Boris Zbarsky [:bzbarsky]]

> I might in fact be able to turn this into a build failure

Filed bug 842561.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/21d3ff83095f because otherwise bug 842561 was going to turn the tree red.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 715004 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 837034
User impact if declined: Probably-exploitable crashes
Testing completed (on m-c, etc.): Passes tests
Risk to taking this patch (and alternatives if risky): Very very low risk
String or UUID changes made by this patch: None

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

bz: did you mean to r- the patch you r+'d and also at the same time propose it for Aurora uplift?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 715004 [details] [diff] [review]
fix

No, didn't mean to change the state of the "r".  ;)

Comment 18

[Jesse Ruderman]

http://hg.mozilla.org/mozilla-central/rev/21d3ff83095f

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/236b2aa5d24f

Comment 20

[u279076]

Flagging for verification in Firefox 21 and 22.

Comment 21

[u279076]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #20)
> Flagging for verification in Firefox 21 and 22.

Kamil, can you please help us with verifying this bug?

Comment 22

[Kamil Jozwiak [:kjozwiak]]

Reproduced the issue using the following build (M-C):
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-02-22-03-11-33-mozilla-central/

Reproduced the issue using the following build (Aurora):
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-02-22-11-12-54-mozilla-aurora/

Once I reproduced the problems, went through the following builds to make sure the issue has been fixed:

Went through the attached testcase using the latest M-C build (Passed):
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-03-12-03-10-46-mozilla-central/

Went through the attached testcase using the latest Aurora build (Passed):
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-03-12-04-20-13-mozilla-aurora/

Comment 23

[u279076]

Thanks Kamil.