911521 - Make it easier to create JS objects from JS that implement WebIDL Interfaces

Status: NEW

(Core :: DOM: Bindings (WebIDL), defect)

Description

[Eric Rescorla (:ekr)]

Consider the following code in PeerConnection.js:

  this.foundIceCandidate(new this._dompc._win.m  foundIceCandidate: function(c) {
    this.dispatchEvent(new this._dompc._win.RTCPeerConnectionIceEvent("icecandidate",
                                                                      { candidate: c }));
  },


If you call foundIceCandidate with:

  new RTCIceCandidate();

This does not work. However, if you call it with:

  new this._dompc._win.mozRTCIceCandidate();

It does, even though RTCIceCandidate implements the expected mozRTCIceCandidate() contract. According to bz, this is because:

[6:52pm] bz: the contract is only used when we are asked to create a mozRTCIceCandidate object
...
[6:52pm] bz: at which point we use that contract to create a chrome-side JS object
...
[6:52pm] bz: and have the content-side C++ object point to it.
[6:52pm] bz: So what you need to do in this case is create a content-side C++ object
[6:52pm] bz: since that's what your callee expects to see

It would be nice if either (a) this worked out of the box or (b) the documentation and errors were clearer about what was expected.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Peter and I just talked about this.  We have a proposal that will somewhat affect what we put in place in bug 865951, but I think it should still be OK.

The proposal is as follows:

If:

1) A WebIDL method is called while in a chrome compartment.
2) We're trying to unwrap a JS object to a JS-implemented interface.
3) The passed-in object is not a cross-compartment wrapper.
4) The passed-in object has a "contractID" property that matches the contractID
   registered for the interface.

Then we go ahead and auto-wrap the JS object in the right WebIDL object.  The change from bug 865951 is item #4 above; I would start doing that in the situation we were in for that bug as well.

Jan-Ivar, does that seem reasonable?  In particular, do existing things using the bug 865951 setup already have the right contractID properties?

Comment 2

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

> Jan-Ivar, does that seem reasonable?

I don't think so because the constructor (the init() and __unit() methods) will not have been called in this case. I think the problem here is that RTCIceCandidate in PeerConnection.js should have been called RTCIceCandidateImpl - it is an implementation prototype, and instantiating from it directly seems like pilot error. In c++, you wouldn't expect to be able to create a new PeerConnectionImpl.

Comment 3

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

s/__unit/__init/

Comment 4

[Eric Rescorla (:ekr)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> > Jan-Ivar, does that seem reasonable?
> 
> I don't think so because the constructor (the init() and __unit() methods)
> will not have been called in this case. I think the problem here is that
> RTCIceCandidate in PeerConnection.js should have been called
> RTCIceCandidateImpl

I'm not following this. I've seen lots of cases in code
where some class implements interface X where that class
isn't called XImpl.


> - it is an implementation prototype, and instantiating
> from it directly seems like pilot error.

Well, pilot error or not, it's permitted by JS and then
doesn't behave as expected, causing confusion for the
programmer.


> In c++, you wouldn't expect to be
> able to create a new PeerConnectionImpl.

Really?

http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsNameSpaceManager.cpp#l254
(nsCOMPtr<NameSpaceManagerImpl> manager = new NameSpaceManagerImpl();)

http://dxr.mozilla.org/mozilla-central/source/content/xbl/src/nsXBLProtoImpl.cpp#l492
(nsXBLProtoImpl* impl = new nsXBLProtoImpl();)

Comment 5

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

RTCIceCandidate(Impl) has init() and __init() methods that are called implicitly by the mozRTCIceCandidate constructor, therefore, constructing it another way seems like a bad idea.

> Well, pilot error or not, it's permitted by JS and then doesn't behave as expected,
> causing confusion for the programmer.

I don't mean to be glib, but most things are permitted in JS and then don't behave as expected. ;-) I'm all for avoiding confusion though.

> Really?

OK fine I'll concede that point (nevermind that this is still an incorrect way to create another peerconnection, even though it is permitted), but IPeerConnection is XPIdl and doesn't have construction semantics. Some Webidl interfaces and RTCIceCandidate do.

In any case, I don't think what bz proposed is reasonable, because the constructor is not called.

Comment 6

[Boris Zbarsky [:bzbarsky]]

My proposal does assume that the JS side is being reasonable and doing whatever initialization it needs to.  Note that we already have a codepath like that: the one added in bug 865951.  And another one that we added in bug 895495...

I do agree that the proposal lets chrome JS shoot itself in the foot, though.  I'm not sure I see a good way around that if we want to add a simpler way to do the construction here.  :(

Comment 7

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

OK so I'm already behind when chasing a construction guarantee here then.

I suppose you could invoke the same benefits as in bug 865951 (private construction abilities) and bug 895495 (performance), but I think "simpler" is a stretch when it comes with said footgun. I wouldn't do it for that reason alone. That said, I see the pattern now.

It's a shame there's no way to have the webidl constructor args forwarded to the implementation constructor.

Comment 8

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

Sorry that was ambiguous. I meant I'm ok if the other benefits are desired here.

Comment 9

[Eric Rescorla (:ekr)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #5)
> RTCIceCandidate(Impl) has init() and __init() methods that are called
> implicitly by the mozRTCIceCandidate constructor, therefore, constructing it
> another way seems like a bad idea.

Fine, but the problem here is that it looks like you can do it and then
you can't, causing confusion. I spent about an hour messing around with
this, assuming it was something simple like a typo before I gave up and
asked bz. that seems inefficient.

I would be perfectly satisfied if attempts to directly call the
constructor in the JS caused an error which pointed you in the
right direction.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> It's a shame there's no way to have the webidl constructor args forwarded to the
> implementation constructor.

I should just try to figure out a way to to this...

> I would be perfectly satisfied if attempts to directly call the
> constructor in the JS caused an error

That's actually pretty hard, since bindings code can't affect the running of random JS on random pure-JS objects... ;)

Comment 11

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

(In reply to Boris Zbarsky [:bz] from comment #10)
> > I would be perfectly satisfied if attempts to directly call the
> > constructor in the JS caused an error
> 
> That's actually pretty hard, since bindings code can't affect the running of
> random JS on random pure-JS objects... ;)

RTCIceCandidate could of course verify its constructor was called, but at runtime cost.

I still think renaming it RTCIceCandidateImpl would help, because right now it sounds like the end-all to call, while the one to call sounds like a cat walked on my keyboard. Otherwise, even with a solution from bz, users might spend hours trying to figure out why their constructor args don't work, and we're back her.

Comment 12

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven't been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Given our desire to not support JS-implemented webidl, I suspect we should just wontfix this.