822674 - mozRTCPeerConnection isn't a true javascript object as it should be

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[alskiontheweb]

Attachment: firefox_peerconnection_bug.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.97 Safari/537.11

Steps to reproduce:

I am trying to attach data to the mozRTCPeerConnection for use in the many callbacks it wires up.


Actual results:

I get a javascript exception:

NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN: Cannot modify properties of a WrappedNative
file:///C:/Temp/firefox_peerconnection_bug.html
Line 7

This does not happen in Chrome.


Expected results:

The javascript object should have been attached to the mozRTCPeerConnection

Comment 1

[Adam Roach [:abr]]

Is this related to and/or the same issue as Bug 823512? This sounds like it might be a black-box description of that implementation issue...

Comment 2

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

This appears to be because the XPConnect type is XPC_WN_NoMods_NoCall_Proto_JSClass (note the NoMods)

Comment 3

[alskiontheweb]

Sounds like a simple fix. Can I expect a fix soon?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Sounds like a simple fix.

It's not.

So the fundamental issue is that peerconnection is implemented in JS.  And it has a classinfo that's not nsIXPCScriptable (because nsIXPCScriptable is not a scriptable interface, so can't actually be touched from JS), so it ends up with XPC_WN_NoHelper_JSClass as the JSClass.  And this class doesn't allow adding expando properties (see XPC_WN_OnlyIWrite_AddPropertyStub).

I see several options here:

1)  We change something about XPCWrappedNative::Init to use a class that allows expandos
    in some cases when we have no scriptable helper.  This sounds dangerous to me.
2)  We change the classinfo implementation for peerconnection to be a c++ object which
    implements nsIXPCScriptable.  In fact, we can then just create it as normal in
    nsDOMClassInfo, and just expose a scriptable way to get the resulting nsIClassInfo. 
    This seems like the simplest way forward to me, offhand.
3)  Rewrite in C++.  ;)
4)  Rewrite in WebIDL, but that needs a fix for bug 822470 at least.

jst, any obvious pitfalls you see for option 2?  That seems like the best bet for a short-term fix; the long-term fix should be 3 or 4.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and the point is that in C++ NS_GetDOMClassInfoInstance can get the right nsIClassInfo given the classinfo id.  So we'd just need a scriptable API somewhere to do this with the peerconnection id.

Comment 6

[alskiontheweb]

So does that mean it's relegated to not being fixed because it's hard? I hope not. There are a lot of browser based APIs being developed around this stuff actively. The longer it takes for this to get fixed, the less likely people are to try and roll your WebRTC implementation in. That's where my worry is, I'd like to see a browser besides Chrome actually support P2P WebRTC components.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> So does that mean it's relegated to not being fixed because it's hard?

No, it means that fixing it depends on building the infrastructure to make fixing it possible.  In particular, we're going to do item 4 from comment 4, most likely.  It'll just take several months to get there.  And then another 3 months to get it shipping, of course....

If we really need something sooner than that, we should do item 2.  Randell?

Comment 8

[alskiontheweb]

OK, thanks for the information.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Note to self: this is currently exposed via a javascript-constructor category with the contract "@mozilla.org/dom/peerconnection;1".   It doesn't look like there's a this-translator for the callbacks, offhand.

I'm going to try option 2 from comment 4, then if that fails a C++ shim that calls into the JS impl...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Give RTCPeerConnection a sane classinfo.

This passes the tests in dom/media/tests/mochitest.  Not sure what else I should be running....

Comment 11

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

Comment on attachment 712261 [details] [diff] [review]
Give RTCPeerConnection a sane classinfo.

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

OK from my side

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 712261 [details] [diff] [review]
Give RTCPeerConnection a sane classinfo.

r=jst, but peterv, don't let me stop you from looking this over as well.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 712261 [details] [diff] [review]
Give RTCPeerConnection a sane classinfo.

Going to just land this and let Peter tell me later if it's wrong.  ;)

Comment 14

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fc77f0a35d8

Comment 15

[Boris Zbarsky [:bzbarsky]]

And http://hg.mozilla.org/integration/mozilla-inbound/rev/2bfd9a8702ef to disable the test on Android.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0fc77f0a35d8
https://hg.mozilla.org/mozilla-central/rev/2bfd9a8702ef