This is problematic for a number of reasons. |any| and |object| pretty much roll back any of the security and sanity guarantees we get from the binding layer. If an arbitrary object really needs to be passed, I think we should just do a structured clone instead.

Apparently WebIDL, or at least our parser, can't handle an attribute with type |Sequence<MediaStream>|. So hopefully we can just get rid of these deprecated versions, which aren't used in our test suite, and aren't in the spec.

I've written thorough test coverage for this, which is now attached. I spent a few hours on the train yesterday fighting with the CodeGen to make this work. I think it probably makes the most sense to insert this at the layer of CGJSImplMember. Before invoking the method, we should iterate over the arguments, and for any |object| or |any|, we should do the following: (1) Declare a Rooted<T> clonedArgX(cx); to store the result of the clone (2) Do a scoped entry of the target compartment (3) JS_StructuredClone(cx, argX, clonedArgX.address(), nullptr, nullptr); (4) Set argX (a handle) to clonedArgX. For return values, we should simply do: JS_StructuredClone(cx, rv, rv.address(), nullptr, nullptr) before returning, since at that point we should be in the caller's compartment. However, I ran into a number of problems: * How do I force the caller to pass me a JSContext? * How do I determine the compartment at this level, before the CallSetup is created? I have |aCompartment|, but that's not enough to pass to JSAutoCompartment. * How do I deal with all the different return value types? It looks like getters sometimes have a dipper convention in which they return void (such as MozInterAppConnection::GetKeyword) Are these straightforward hurdles to overcome? If so, I'm happy to write the patch. But if it involves reshuffling various things in the CodeGen, it would be much more efficient for someone more familiar with all of its intricacies (like bz, or maybe mccr8?) to do it. Boris, what do you think?

(In reply to Bobby Holley (:bholley) from comment #2) > Created attachment 814342 [details] [diff] [review] > Remove deprecated attribute versions of {local,global}Streams from > PeerConnection. v1 > > Apparently WebIDL, or at least our parser, can't handle an attribute with > type > |Sequence<MediaStream>|. So hopefully we can just get rid of these deprecated > versions, which aren't used in our test suite, and aren't in the spec. Right, that's not allowed by webidl, because an attribute where |foo.localStreams === foo.localStreams| is false is silly.

Comment on attachment 814342 [details] [diff] [review] Remove deprecated attribute versions of {local,global}Streams from PeerConnection. v1 Review of attachment 814342 [details] [diff] [review]: ----------------------------------------------------------------- This LGTM but I am rerouting to JIB, who is handling this piece.

> How do I force the caller to pass me a JSContext? Maybe modifying needCx in Codegen.py is what you want? At least, you can look at the places it is called.

Comment on attachment 814342 [details] [diff] [review] Remove deprecated attribute versions of {local,global}Streams from PeerConnection. v1 (In reply to Bobby Holley (:bholley) from comment #0) > This is problematic for a number of reasons. |any| and |object| pretty much > roll back any of the security and sanity guarantees we get from the binding > layer. If an arbitrary object really needs to be passed, I think we should > just do a structured clone instead. I'm confused. The description of this bug says "Stop passing direct JS object references to JS-implemented WebIDL", but this patch removes a readonly attribute which is returning a JS object *from* JS-implemented WebIDL, not passing it *to* JS-implemented WebIDL. - I suppose we're returning direct references to objects coming from JS-implemented webidl, if that's what you mean, but is there a safety issue here? > So hopefully we can just get rid of these deprecated versions, > which aren't used in our test suite, and aren't in the spec. These were deprecated with warnings in FF24 to not immediately break webrtc devs who made stuff on FF23 and earlier. Time may have come to remove them. CCing Maire for comment. If so, there are a few others in the same file we should remove at the same time. Lastly, please note that RTCPeerConnection and GetUserMedia have a few APIs where they *must* use raw JS objects, specifically: mandatory constraint inputs. The spec requires us to fail on unknown keys, so we cannot use webidl dictionaries in the API because they silently filter out unknowns. For this reason, our APIs were carefully written to process the raw objects by passing them through dictionaries internally after some careful inspection looking for unknowns. Code was reviewed by bz.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #7) > I'm confused. The description of this bug says "Stop passing direct JS > object references to JS-implemented WebIDL", but this patch removes a > readonly attribute which is returning a JS object *from* JS-implemented > WebIDL, not passing it *to* JS-implemented WebIDL. - I suppose we're > returning direct references to objects coming from JS-implemented webidl, if > that's what you mean, but is there a safety issue here? It's really an issue of to/from. The WebRTC stuff isn't currently a safety issue AFAICT, but it gets in the way of our desired semantics for object/any, because we want to clone and throw for everything that can't be cloned, and we don't want to implementing cloning for MediaStreams. > These were deprecated with warnings in FF24 to not immediately break webrtc > devs who made stuff on FF23 and earlier. Time may have come to remove them. > CCing Maire for comment. If so, there are a few others in the same file we > should remove at the same time. Ok. > Lastly, please note that RTCPeerConnection and GetUserMedia have a few APIs > where they *must* use raw JS objects, specifically: mandatory constraint > inputs. The spec requires us to fail on unknown keys, so we cannot use > webidl dictionaries in the API because they silently filter out unknowns. That's fine, as long as they don't mind getting clones.

> I think it probably makes the most sense to insert this at the layer of CGJSImplMember. I don't think that's all that viable; see below. > * How do I determine the compartment at this level By duplicating all the CallSetup work. > I have |aCompartment| That's the caller compartment, not the callee compartment. I think the right way to handle this is in the codegen for the callback interface that implements actual calls to the chrome code. This would incidentally make direct calls to the JS-implemented interface from C++ do the right thing too. So basically, modify CallbackMember.getArgConversion and CallbackMember.getResultConversion to do something special in the js-impl case. Note that the latter already does; see the isJSImplementedDescriptor bit and the "JSImpl" value isCallbackReturnValue being passed to getJSToNativeConversionInfo. We could add something similar to getWrapTemplateForType; right now it just does a JS_WrapValue/Object, but it could do something else in the "JSImpl" case. At least this would work for arguments. For return values, it's not clear to me that the _caller_ compartment is available here. And for C++ callers it's generally not, of course... So maybe we need to handle return values somewhere higher up the callstack in the call-from-JS case and accept that they'll be direct chrome object references in the C++ case.

Boris, would you be willing to write a patch doing the above?

Sure, after I get back from vacation. So in about 3 weeks.

bz: bholley: It seems to me like the right place to do cloning of args is in the callback codegen, as I described [8:08pm] bholley: bz: yeah. rv is harder though [8:08pm] bz: bholley: for return values, it makes more sense to do it in the binding code itsel [8:08pm] bz: er, itself [8:08pm] bholley: bz: yeah [8:08pm] jwatt joined the chat room. [8:08pm] bholley: bz: how do we make it give us a JSContext? [8:09pm] bholley: bz: we can always AutoJSContext [8:09pm] bz: we have a JSContext in the _binding_ code [8:09pm] bz: not to be confused with the impl code [8:09pm] bholley: bz: always? [8:09pm] bholley: bz: wait, do you mean in the lowercase version? [8:09pm] bz: Let's take a concrete example [8:09pm] bholley: we have 3 functions we go through [8:09pm] bholley: first something_with_underscores [8:09pm] bholley: then InterfaceName::Foopy [8:09pm] bholley: then InterfaceNameImpl::Foopy [8:10pm] bz: Right [8:10pm] bz: I'm talking about something_with_underscores [8:10pm] bz: which has a "C++ object" [8:10pm] bz: in this case JSObject* or JS::Value [8:10pm] bz: and the cx that actually called into it [8:10pm] jet left the chat room. (Quit: jet) [8:10pm] bz: and does a JS_WrapValue/Object [8:10pm] bz: and it would just do something a bit more complicated instead. [8:10pm] florian is now known as electra. [8:11pm] electra is now known as famoused. [8:11pm] bholley: bz: it does a MaybeWrapValue or something [8:11pm] bz: sure [8:11pm] bz: the point remains [8:11pm] bholley: bz: it seems cleanest to pass the cx to the second level [8:11pm] bz: (in our case there would be no "maybe" about it) [8:11pm] bholley: bz: because the codegen for that part is unique to JSImpl [8:11pm] bholley: bz: whereas the other codegen presumably is not [8:11pm] bz: hmm [8:11pm] bz: Ok [8:12pm] bz: So there is, say, mozRTCPeerConnection::CreateOffer [8:12pm] bz: and mozRTCPeerConnectionJSImpl::CreateOffer [8:12pm] mayhemer joined the chat room. [8:12pm] bholley: right. I'm suggesting mozRTCPeerConnection::CreateOffer [8:12pm] bz: The latter shares codegen with other callback stuff [8:12pm] bholley: bz: mostly because it has the least junk in it already [8:12pm] bz: right [8:12pm] bz: We could do that. [8:13pm] bz: So pass a cx to that. [8:13pm] bz: And then for the object/any/typedarray case do magic. [8:13pm] bholley: bz: so how do we pass the cx? [8:14pm] bz: the same way we pass the compartment right now [8:14pm] bholley: bz: and do we really want to treat typedarrays like this? [8:14pm] Ms2ger: needsCx? [8:14pm] bz: Hmm [8:14pm] bholley: bz: I think we probably want to hold out for Xrays on those [8:14pm] bz: We might be able to just needscx, true [8:14pm] bz: bholley: ah, fair [8:14pm] bz: ok [8:16pm] bz: So yeah, the simplest thing is to force needsCx to true here [8:16pm] bz: Like we force it to false right now [8:17pm] bholley: bz: ok [8:18pm] bholley: bz: can I leave a NEEDINFO for you on the bug? [8:18pm] bz: You mean for me to fix this in 3 weeks? I guess so, yeah. [8:19pm] bz: Copy/paste the above conversation's relevant bits in too? [8:19pm] bholley: ok

Andrew said he might take a look at this next week.

Comment on attachment 814342 [details] [diff] [review] Remove deprecated attribute versions of {local,global}Streams from PeerConnection. v1 Review of attachment 814342 [details] [diff] [review]: ----------------------------------------------------------------- r- because patch breaks mochitests: > 0:17.95 72 ERROR TEST-UNEXPECTED-FAIL | > /tests/dom/media/tests/mochitest/test_dataChannel_basicAudio.html | > uncaught exception - TypeError: this._pc.localStreams is undefined at > http://mochi.test:8888/tests/dom/media/tests/mochitest/pc.js:1316 Please change http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/pc.js#1316 to use getLocalStreams() and getRemoteStreams().

(note to self - this is stored in my local branch |webidlany|).

(In reply to Jan-Ivar Bruaroey [:jib] from comment #7) > > So hopefully we can just get rid of these deprecated versions, > > which aren't used in our test suite, and aren't in the spec. > > These were deprecated with warnings in FF24 to not immediately break webrtc > devs who made stuff on FF23 and earlier. Time may have come to remove them. > CCing Maire for comment. If so, there are a few others in the same file we > should remove at the same time. It probably makes sense to remove them. Do we have any idea much impact will this have on devs? We can start warning people now that the deprecated versions will be removed in Fx 27. If we think a non-trivial number of devs will be impacted, I could blog about it too -- even put something on Hacks.

Good ideas. As to impact, we know the changes they need to make are trivial, mostly renaming. If they don't make the changes then stuff may stop working of course, but it sounds like this is what the trains are for. As to how many devs are impacted, I have no idea. We know this minimizes the difference between browsers, so it should be welcome overall,I think.

I've started looking at this. I'll look at the argument passing stuff first, as that sounds a bit more straightforward. I think it makes sense to deal with the WebRTC deprecation in a separate bug.

I filed bug 929530 for the deprecation stuff.

So we need to be careful here to not cause broken JS+WebIDL implementations. Say one wants to implement CustomEvent in JS, var d = {}; (new CustomEvent("foo", {detail: d})).detail === d; // This should still be true.

(In reply to Olli Pettay [:smaug] from comment #20) > So we need to be careful here to not cause broken JS+WebIDL implementations. > Say one wants to implement CustomEvent in JS, > var d = {}; > (new CustomEvent("foo", {detail: d})).detail === d; // This should still be > true. Hopefully disallowing attributes (bug 929554) should mostly solve this, since that's the most likely case where identity would matter.

I haven't tried to run anything, but this at least compiles.

Note that we may also have issues here with callbacks and callback interfaces, since those look like structs on the C++ side but unwrap right back to a JS value when going back into JS.

You mean like in http://mxr.mozilla.org/mozilla-central/source/dom/webidl/RTCStatsReport.webidl : > callback RTCStatsReportCallback = void (object value, DOMString key, RTCStatsReport obj); The callback gets an object passed because the stats are dictionaries of varying types. Will that be a problem?

> The callback gets an object passed That's covered by this bug. The issue comment 23 is about is whether it's safe to pass a function from untrusted JS to chrome JS and then have chrome JS call it.

(In reply to On vacation Oct 12 - Oct 27 from comment #23) > Note that we may also have issues here with callbacks and callback > interfaces, since those look like structs on the C++ side but unwrap right > back to a JS value when going back into JS. IIRC, I complained about this. Do you remember where this discussion happened? What was the outcome?

No, and I don't recall any outcome. Right now we just pass them through.

Here's a fun case that has come up in the argument cloning. PeerConnectionObserver is ChromeOnly, and has Constructor(object domPC). In the __init for PeerConnection, we create a PeerConnectionObserver, and pass it |this|, the inner JS implementation of PeerConnection (I think): this._observer = new this._win.PeerConnectionObserver(this); So, if we clone it, we get some weird neutered object, which I think later causes "Permission denied to access object" and failure. It looks to me like we need to use the standard wrapping path in some situation. I'm not sure what the weakest thing we can get away with is. In this case, the caller and callee compartments are not the same, and somehow the caller is chrome but the callee isn't (?!?), so I'm not sure what condition we could use that is weaker than "the interface is ChromeOnly" to decide when to do wrapping instead of cloning. (Side note: when we fail in this way, which ends up with JS_CallFunctionValue failing in mozRTCPeerConnectionJSImpl::__Init, we end up later crashing with a null deref in ~PeerConnectionImpl. I wonder if that's similar to that weird intermittent crash we sometimes see...)

This version fixes a silly error (throw on failure, not success!), and uses the existing behavior for Chrome-only interfaces to work around the PeerConnectionObserver issue, which seems to be the only interface affected. It passes the tests in dom/media/tests/mochitest/, but those tests also don't hit any of the clone cases I added, so that doesn't mean anything. Here's a try run, with NS_ASSERTIONs when we hit the clone case to see if this code is ever actually being run: https://tbpl.mozilla.org/?tree=Try&rev=d7921e46238a

Bobby, do you have any thoughts on comment 28? Thanks.

(In reply to Andrew McCreight [:mccr8] from comment #28) > Here's a fun case that has come up in the argument cloning. > PeerConnectionObserver is ChromeOnly, and has Constructor(object domPC). I passed using type 'object' out of convenience here. I'm open to any convention that is easiest for you guys. PeerConnectionObserver and RTCPeerConnection are "friends" in the same .js file and they have references to each-other because they talk. Ideally, there seems to be no need to go through webidl, if that is an option. > (Side note: when we fail in this way, which ends up with > JS_CallFunctionValue failing in mozRTCPeerConnectionJSImpl::__Init, we end > up later crashing with a null deref in ~PeerConnectionImpl. I wonder if > that's similar to that weird intermittent crash we sometimes see...) Which crash is that?

(In reply to Jan-Ivar Bruaroey [:jib] from comment #31) > PeerConnectionObserver and RTCPeerConnection are "friends" in the same .js > file and they have references to each-other because they talk. Ideally, > there seems to be no need to go through webidl, if that is an option. Yeah, what you are doing seems perfectly reasonable. I'll try to think of another way to do it without punching a hole through WebIDL. > Which crash is that? Bug 877515. I guess it isn't the same, but it feels similar somehow...

(In reply to Jan-Ivar Bruaroey [:jib] from comment #31) > (In reply to Andrew McCreight [:mccr8] from comment #28) > > Here's a fun case that has come up in the argument cloning. > > PeerConnectionObserver is ChromeOnly, and has Constructor(object domPC). > > I passed using type 'object' out of convenience here. I'm open to any > convention that is easiest for you guys. > > PeerConnectionObserver and RTCPeerConnection are "friends" in the same .js > file and they have references to each-other because they talk. Ideally, > there seems to be no need to go through webidl, if that is an option. So, I think the constructor should take an RTCPeerConnection instead of an |object|. And the issue here is that the the PeerConnectionObserver implementation wants to access the JS object behind RTCPeerConnection. Did I get that right? If so, there are a few solutions I can think of: (1) Expose a ChromeOnly |id| on RTCPeerConnection, and maintain a WeakMap in the JS file mapping (id => underlying JS object). (2) Implement chrome-only .wrappedJSObject property for all JS-implemented WebIDL objects. This would let any chrome caller unwrap JS-implemented WebIDL to the underlying JS.

(2) would be a good mirror for the chromeonly __DOM_IMPL__ thing we already have, for what it's worth.

Thought as much as .wrappedJSObject is a good analog of the old XPCOM component behavior, I think we should probably give it a different name if we do that.

In bug 934035, I work around the object argument to PeerConnectionObserver using the trick somebody suggested to pass the implementation object through using an Xray expando, so no additional support is needed. This version, which removes the ChromeOnly exception and rebases over the handlification of JS_StructuredCloneObject is mostly green: https://tbpl.mozilla.org/?tree=Try&rev=ea0ccf69901e The one failing test is dom/settings/tests/test_settings_blobs.html, on this line: let req = mozSettings.createLock().set({"test1": storedBlob}); where storedBlob is defined by let storedBlob = new Blob([""], {"type": "text/xml"}); Presumably the problem is that the blob can't be cloned, though I wasn't able to figure out the inner workings of structured clone. "JSON with WebIDL objects as data" seems like a reasonable use case, but I'm not sure how to support it via JS_StructuredClone.

There's some way to add custom hooks to JS_StructuredClone, so maybe that will work. I'll ask sfink about it.

So, we already have code to serialize Blobs and FileList, but it's specific to PostMessage - see SCTAG_DOM_BLOB in nsGlobalWindow.h. The reason for the specificity is that we need to hold a strong reference to any serialized XPCOM objects, which happens on the event itself (see PostMessageEvent::StoreISupports). We already have a mechanism for default main-thread structured clone callbacks, but it's currently limited to ImageData (see NS_DOM{Read,Write}StructuredClone). I think we should generalize the Blob/FileList stuff, and hoist it into NS_DOM{Read,Write}StructuredClone so that we don't have to keep re-inventing the wheel. I think we should create a refcounted Gecko wrapper around a structured clone buffer, which could take over the StoreISupports duties. We could then stash this directly in PostMessageEvent, and take advantage of this machinery for our uses as well.

We'll land these before actually landing bug 923904, because we want to build the tests for bug 968335 on top of the same infrastructure.

Comment on attachment 8375203 [details] [diff] [review] Tests. v2 Review of attachment 8375203 [details] [diff] [review]: ----------------------------------------------------------------- Covers just the build bits (you probably didn't need my review on this).

(In reply to Gregory Szorc [:gps] from comment #42) > Covers just the build bits (you probably didn't need my review on this). Aren't we supposed to get review from buildconfig peers for anything other than adding/removing files these days?

Comment on attachment 8375203 [details] [diff] [review] Tests. v2 Switching review of this to Andrew, since he's going to write the patch here anyway. Andrew, if you could give this a quick lookover, we can land bug 968335.

Comment on attachment 8375203 [details] [diff] [review] Tests. v2 Review of attachment 8375203 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for writing these tests! bz, does the general setup here of adding a DEBUG-only webidl and implementation etc. seem okay to you? ::: dom/bindings/Bindings.conf @@ +1612,5 @@ > #################################### > # Test Interfaces of various sorts # > #################################### > > +'TestInterfaceJS' : {}, You don't need an empty entry in Bindings.conf, do you? If not, please remove this. ::: dom/bindings/test/test_bug923904.html @@ +9,5 @@ > + <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> > + <script type="application/javascript"> > + > + /** Test for cloning of |any| and |object| for js-implemented WebIDL. **/ nit: "js-" should be "JS-". @@ +14,5 @@ > + SimpleTest.waitForExplicitFinish(); > + SpecialPowers.pushPrefEnv({set: [['dom.expose_test_interfaces', true]]}, go); > + > + function go() { > + var any = { a: 11 }; "any" and "obj" are a little confusing as variable names. Maybe "someAny" and "someObj" or something? @@ +18,5 @@ > + var any = { a: 11 }; > + var obj = { b: 22, c: "str" }; > + var t = new TestInterfaceJS(any, obj); > + is(Object.getPrototypeOf(t), TestInterfaceJS.prototype, "Prototype setup works correctly"); > + is(t.anyArg.toSource(), any.toSource(), "anyArg looks correct"); "looks correct" is not great. Maybe "anyArg has the same source as its input" or something? Same with the next one. @@ +44,5 @@ > + is(iface[propname].__proto__, Object.prototype, "vanilla object"); > + isnot(iface[propname], obj, "Should not be the original object"); > + isnot(iface[propname], iface[propname], "Should get cloned each time"); > + try { > + iface[propname] = { stringProp: "hi", reflectorProp: document }; It would be nice if we had a test in here to check that blob works, though I can just put that on my to-do list. Not having that causes a failure in some random place in the test suite.

(In reply to Andrew McCreight [:mccr8] from comment #45) > ::: dom/bindings/Bindings.conf > @@ +1612,5 @@ > > #################################### > > # Test Interfaces of various sorts # > > #################################### > > > > +'TestInterfaceJS' : {}, > > You don't need an empty entry in Bindings.conf, do you? If not, please > remove this. It wasn't necessary back when I first wrote these tests in October, but seems to have become necessary in the mean time. I'd be curious to know if there's something else I should do here. > It would be nice if we had a test in here to check that blob works, though I > can just put that on my to-do list. Not having that causes a failure in > some random place in the test suite. Yeah, if you could do that I'd appreciate it - I don't want to add too much to this test given that I'm not actually fixing the bug. Thanks for the review!

A green try push is here: https://tbpl.mozilla.org/?tree=Try&rev=9480abb18974 I realize in retrospect that I should have probably done an all-platform push, since the package manifest munging here might do something funny on android or b2g. It's too late now though. If it breaks something, I apologize in advance to the sheriffs.

> but seems to have become necessary in the mean time Sounds fishy. What happens if you leave it out?

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1e8c2e85575a for b2g emulator bustage a la https://tbpl.mozilla.org/php/getParsedLog.php?id=34717947&tree=Mozilla-Inbound

Comment on attachment 8375203 [details] [diff] [review] Tests. v2 Apart from the bindings.conf thing, looks reasonable.

(In reply to Boris Zbarsky [:bz] from comment #49) > Sounds fishy. What happens if you leave it out? ...nothing, apparently. Adding it definitely make things compile when they hadn't before, but presumably I've done something else in the mean time to rectify that. Anyway, sorry for the churn. https://tbpl.mozilla.org/?tree=Try&rev=d607ce9c51e6

https://hg.mozilla.org/mozilla-central/rev/e7d697004921 https://hg.mozilla.org/mozilla-central/rev/f665d5a2292c

I think we should do bug 1036214 and WONTFIX this bug.