865951 - Need to figure out how to let JS-implemented WebIDL return JS-implemented objects from methods/getters

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Consider this setup:

  [JSImplementation="@mozilla.org/foo"]
  interface Foo {};

  [Constructor, JSImplementation="@mozilla.org/bar"]
  interface Bar {
    Foo getFoo();
  };

Note that Foo itself does NOT have a constructor, because content is not supposed to be able to construct an instance of Foo directly.

Now how should getFoo be implemented?

A few options:

1)  We make the interface object for Foo actually produce a new Foo when called in chrome.  Then getFoo() calls |new contentWindow.Foo()|.  This requires Foo to know the right window (which is not too bad; it probably wants that anyway).  It also means is can't do any random initialization of Foo that's not allowed by the constructor, and in particular if Foo already has a constructor it can't do anything that constructor doesn't allow.  It also seems very hoop-jumpy.

2)  We make the JS-to-C++ conversion coming out of the call into the callback try to unwrap as an actual Foo and if that fails fall back to taking the given JSObject and wrapping it up in a Foo.  That way getFoo() can just |new FooImpl()| in the chrome and return it and the generated code will see that it's not already a WebIDL Foo object and just wrap it up into one.

I personally like option 2 a lot more, honestly...  Thoughts?  Other ideas?

Comment 1

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

Just making sure option 2 can handle this use-case (of different webidl method accepting returned object as "regular" webidl objects are accepted):

  let pc1 = new mozRTCPeerConnection();

  // offer returned must be type RTCSessionDescription(.webidl)
  pc1.createOffer(function (offer) {
    // passed back into different method which takes RTCSessionDescription
    pc1.setLocalDescription(offer, step2, failed);
  }, failed);

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yes, that would work fine in option 2.  The only caveat, but it holds for all this stuff is that setLocalDescription would see an Xray around the content-side RTCSessionDescription object, not the chrome-side object backing it.  Though we could add a chrome-only getter for getting out the chrome-side object if needed.

Comment 3

[Andrew McCreight [:mccr8]]

Option 2 sounds okay to me.  I guess returning an incorrect JS object isn't a big deal, as long as it doesn't happen to have a property that should be kept private that is public in the accidental interface.

Comment 4

[Reuben Morais [:reuben]]

(In reply to Boris Zbarsky (:bz) from comment #0)
> Consider this setup:
> 
>   [JSImplementation="@mozilla.org/foo"]
>   interface Foo {};
> 
>   [Constructor, JSImplementation="@mozilla.org/bar"]
>   interface Bar {
>     Foo getFoo();
>   };
> 
> Note that Foo itself does NOT have a constructor, because content is not
> supposed to be able to construct an instance of Foo directly.

Does that note mean that if Foo *had* a constructor, implementing getFoo() would be trivial? ContactManager returns Contact via DOMRequest for most of its functions, and if I just do "new ContactImpl();", I get "TypeError: Value does not implement interface mozContact" when trying to pass that object back to the API.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Does that note mean that if Foo *had* a constructor, implementing getFoo() would be
> trivial?

Well, possibly.  If Foo had a constructor getFoo could do:

  var newFoo = new relevantContentWindow.Foo();

and create an object.  Whether it could then _configure_ that object properly is a separate question....

Comment 6

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

> If Foo had a constructor getFoo could do:
>
>  var newFoo = new relevantContentWindow.Foo();
>
> and create an object.

That is what I'm doing for bug 823512 at the moment, and it works.

> Whether it could then _configure_ that object properly is a separate question....

All constructors look to be public in the webrtc spec FWIW, so this workaround works. Bug 851178 is actually slightly more pressing, since I have ugly init2(constructor_arg) functions to get around that one. Example:

  onCreateOfferSuccess: function(offer) {
    let desc = new this._dompc._win.mozRTCSessionDescription();
    desc.init2({ type: "offer", sdp: offer });
    this.callCB(this._dompc._onCreateOfferSuccess, desc);
    this._dompc._executeNext();
  },

But even that's hidden in the implementation, and the content JS remains clean (Comment 1).

Comment 7

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

> But even that's hidden in the implementation

Except it pollutes the webidl with the init2 function, so not totally hidden.

Comment 8

[Reuben Morais [:reuben]]

This is the last thing blocking bug 850430 for now.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: If the return value of a JS-implemented method or attribute is a JS-implemented interface and the returned object is not a DOM object, automatically wrap it up in an instance of that interface. does not invoke __init on the chrome object, since there ar

We could try doing the init() thing, but we'd need to get an nsISupports for the JSObject....

Comment 10

[Andrew McCreight [:mccr8]]

> does not invoke __init on the chrome object, since there ar
What is this supposed to be?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> What is this supposed to be?

Read the actual checkin comment in the diff, not the part of it that bzexport chose to use?  ;)

One other note.  We could add a ref from the C++ jsimpl object back to the C++ dom impl object and use that plus GetWrapper() to find the right content-side JSObject instead of getting the __DOM_IMPL__ property.  I'm not sure it's any cleaner, though, and would use more memory.

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 751428 [details] [diff] [review]
If the return value of a JS-implemented method or attribute is a JS-implemented interface and the returned object is not a DOM object, automatically wrap it up in an instance of that interface.   does not invoke __init on the chrome object, since there ar

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

::: dom/bindings/BindingUtils.cpp
@@ +1773,5 @@
> +bool
> +GetWindowForJSImplementedObject(JSContext* cx, JS::Handle<JSObject*> obj,
> +                                nsPIDOMWindow** window)
> +{
> +  // Be very careful to not get tricked here.

Maybe add an assert that cx and obj are in the same compartment?

@@ +1775,5 @@
> +                                nsPIDOMWindow** window)
> +{
> +  // Be very careful to not get tricked here.
> +  MOZ_ASSERT(NS_IsMainThread());
> +  NS_ABORT_IF_FALSE(xpc::AccessCheck::isChrome(js::GetObjectCompartment(obj)),

Should this be a release-mode failure?

@@ +1780,5 @@
> +                    "Should have a chrome object here");
> +
> +  // Look up the content-side object.
> +  JS::Rooted<JS::Value> domImplVal(cx);
> +  if (!JS_GetProperty(cx, obj, "__DOM_IMPL__", domImplVal.address())) {

Does a failure in JS_GetProperty cause there to be an exception?

::: dom/bindings/Codegen.py
@@ +2291,5 @@
>  
>      codeOnFailure is the code to run if unwrapping fails.
> +
> +    If isCallbackReturnValue is "JSImpl" and our descriptor is also
> +    JS-implemented, will fall back to just creating the right object if what we

drop the "will" or change it to "we will" or something.

@@ +2355,5 @@
>  class FailureFatalCastableObjectUnwrapper(CastableObjectUnwrapper):
>      """
>      As CastableObjectUnwrapper, but defaulting to throwing if unwrapping fails
>      """
> +    def __init__(self, descriptor, source, target, exceptionCode,

Kind of weird that the order of exceptionCode and isCallbackReturnValue differs between child and parent class.  Maybe make them consistent?

@@ +2556,5 @@
> +    If isCallbackReturnValue is "JSImpl" or "Callback", then the declType may be
> +    adjusted to make it easier to return from a callback.  Since that type is
> +    never directly observable by any consumers of the callback code, this is OK.
> +    Furthermore, if isCallbackReturnValue is "JSImpl", that might affect the
> +    behavior of some conversions.

That last sentence is quite vague.  Can you describe it a little more, or refer to another place in the code where the conversion happens or the behavior is described? (CastableObjectUnwrapper?)

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Maybe add an assert that cx and obj are in the same compartment?

JS_GetProperty will assert that.  I can add another one if you want, I guess.

> Should this be a release-mode failure?

Yes!  I thought NS_ABORT_IF_FALSE did that, but you're right: it does not.  I will switch to NS_RUNTIMEABORT.

> drop the "will" or change it to "we will" or something.

Hmm.  OK.  I thought the "will" had an implied subject of "this method", but I guess it's not that clear.  Will fix.

> Kind of weird that the order of exceptionCode and isCallbackReturnValue differs between
> child and parent class.  Maybe make them consistent?

OK.

> or refer to another place in the code where the conversion happens or the behavior is
> described? 

Will do.

Comment 14

[Andrew McCreight [:mccr8]]

> JS_GetProperty will assert that.  I can add another one if you want, I guess.
Ah, right.  That's okay then.  The JS engine one is going to be better anyways, because it will runtime abort in Nightly/Aurora.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Oh, and:

> Does a failure in JS_GetProperty cause there to be an exception?

Yes.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e82140fbb63

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e82140fbb63