1186698 - PeerConnection.js: TypeError: Illegal constructor

Status: NEW

(Core :: WebRTC: Networking, defect, P4)

Description

[Jesse Ruderman]

Attachment: testcase

JavaScript error: resource://gre/components/PeerConnection.js, line 566: TypeError: Illegal constructor.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Bz, thoughts on what should happen here?

Probably equally in need of hardening:
http://mxr.mozilla.org/mozilla-central/search?string=__DOM_IMPL__.dispatchEvent

try/catch(){} on the bunch of them?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm, good question.

I guess the difference between the attached testcase and this one:

  conn.onnegotiationneeded = function() { throw "HELLO THERE"; };

is that the exception is really thrown with only chrome script on the stack.

I assume putting a try/catch around the dispatchEvent call doesn't actually help because the binding machinery reports the exception before you ever get your hands on it, right?

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Right, that (http://jsfiddle.net/v1sLudcx ) produces:

    uncaught exception: HELLO THERE                 show:33:1

instead of

    TypeError: Illegal constructor.    PeerConnection.js:567:9

You're right, adding try/catch in PeerConnection.js did nothing.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, ok.

So here's the thing.  If this were C++ code doing the event dispatch, what would happen?  Here's a simple testcase for that:

  <div>Click me</div>
  <script>
    document.querySelector("div").onclick = WebGLRenderingContext;
  </script>

Load that and click the text.  The answer is that an error is reported to the web console (and to the page's onerror handler), just without useful location information.  Also of interest is:

  <div>Click me</div>
  <script>
    document.querySelector("div").onclick = WebGLRenderingContext;
    document.querySelector("div").click();
  </script>

which reports an error at the location of the click() call.

So it seems like the only real bug here is that we're ending up reporting the location as PeerConnection.js (including to web content).  We need to not do that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, so this is a bite of a mess.

If this interface were _not_ js-implemented, then setting the "onfoo" attribute would create an EventHandler for the function that was passed to the setter.  When the event happens, we sould then enter the compartment that the onfoo setter ran in and call into the function, wrapped into that compartment.  Since the onfoo setter is in content, when the attempt to call threw we would be in the content compartment and capture the stack and a filename/lineno in that compartment.

So now the JS-implemented case.  The onfoo setter is called.  This calls into the chrome JS implementation; the argument that's passed in is now wrapped into the chrome compartment (as an Xray).  The chrome JS calls the [ChromeOnly] setEventHandler on the event target ("this").  This ends up creating the EventHandler with the Xray as the backing object and storing it on the node.  Now when the event is fired we end up entering the chrome compartment, calling the Xray, this calls the content-side function, but since it's a DOM constructor it does so in the xray (chrome) compartment, without entering the content compartment.  So when the exception is thrown it gets thrown in the _chrome_ compartment, which means it gets to see the chrome filename/linenumber.  And since it's a JS Error, not a DOMException, it stashes those strings directly instead of having accessors that do principal-filtering at the get site.  So you get an error report with those strings, and window.onerror is passed a chrome-side exception (or more precisely an opaque wrapper for it).

OK, so this sucks.  ;)

I think we have a few possible options here:

1)  Have the DOM always-throwing dummy constructor enter its Xray compartment before it throws.  This would make it behave a bit differently from the non-throwing version which throws when called as a function (and also if it can't get .prototype on the newTarget or if creating the GlobalObject failed, or if any of the argument conversions fails) before it enters the content compartment.

2)  Have setEventHandler rewrap its argument into the event target's compartment.  This would, sadly, mean that setting onfoo from _chrome_ on a JS-implemented thing would not work the same as doing it from content... though arguably setting onfoo from chrome script on a content object is a serious bug anyway.

3)  Change how we expose EventHandler to js-implemented stuff: instead of passing an Xray to the raw object, pass in some sort of representation of the C++ EventHandler, so when we pass it back into setEventHandler we end up with the EventHandler we created on entry to the onfoo setter on the content side instead of creating a new one around the Xray.  This is basically bug 870217, I think.

Of these options, #3 is probably the most correct, but is also the most work and code complexity and codesize.  #1 fixes this specific issue with the throwing constructor, but I bet that just using Event instead of WebGLRenderingContext would lead to the same problem.  So my gut feeling is to do #2 for now and look for time to do #3 sometime here.

Bobby, thoughts?

Comment 6

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

that part of 2) sounds indeed like a rather bad bug. What if chrome onfoo is set, and then content 
tries to access it? Could we make object.setEventHandler throw if object is not from chrome, but handler is?

But anyhow, I'd prefer 3), but 2) is fine too.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> What if chrome onfoo is set, and then content tries to access it? 

It'll get an opaque wrapper.  That's true in the C++-implemented case too.

The difference in (2) between the JS-implemented case and the C++-implemented one is that in the C++-implemented case if you set onfoo from chrome to a chrome function and then the event fires, your function will get called.  But if we do option number (2) then in that situation the event firing will fail to call the function because the event dispatch code will see an opaque wrapper there (I think; Bobby should confirm).  In both cases if content gets onfoo it will get an opaque wrapper object for the chrome function which will throw exceptions if content tries to do anything with it.

Comment 8

[Bobby Holley (:bholley)]

Assuming that there's no __exposedProps__ funny business, chrome [[Object]] objects are opaque to content, and chrome [[Function]] objects are opaque _but callable_.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  So approach #2 would even fire the event handler.  In that case my temptation is to just do that for now....  Thoughts?

Comment 10

[Bobby Holley (:bholley)]

#3 is definitely the most correct solution in terms of the model we're trying to build, but I don't have a handle on exactly how much work it would be.

I'm fine with #2 if it's expedient and doesn't directly move us further from #3 should we find additional reasons to do it down the line.

Comment 11

[Jesse Ruderman]

Attachment: testcase 2

JavaScript error: resource://gre/components/PeerConnection.js, line 611: TypeError: Constructor RTCDataChannelEvent requires 'new'

Comment 12

[Bulk Bug Changes for mreavy's org]

Mass change P3->P4 to align with new Mozilla triage process.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Still reproduces on trunk.

Comment 14

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals