285047 - XPConnect will wrap JS objects as interfaces which contain [noscript] methods, leading to discomfort and rashes in C++ callers

Status: RESOLVED INVALID

(Core :: XPConnect, defect)

Description

[Mike Shaver (:shaver emeritus)]

I hit this when trying to implement nsIRDFResource, and it made me sad. 
XPConnect creates a wrapper for be JS object, and all is well until the template
builder tries to call GetValueConst on it.  I guess we could just be returning
NS_ERROR_NOT_IMPLEMENTED there, but I'm more comfortable with not allowing the
wrappers to be created in the first place, because it's pretty rare that NS_NYI
is handled well by callers, and I think it would be pretty frustrating to get
deep into component development only to find that a critical caller doesn't play
nicely.  (I should say: it _is_ pretty frustrating to etc.)

I'll attach a patch to that effect.

Comment 1

[Mike Shaver (:shaver emeritus)]

Attachment: Don't create wrappedjs goodness for interfaces which have noscript methods.

Comment 2

[David Bradley]

I'm surprised this patch wouldn't break various things in the browser. I have a
vague memory that the streams interfaces use this, and this would remove their
use from JS.

If this patch goes forward then marking methods as not scriptable should be
deprecated. Either an interface is scriptable or not, and marking a method as
not-scriptable means the interface is not-scriptable the same as marking the
interface itself as not-scriptable.

Comment 3

[Darin Fisher]

Yeah, won't this break chatzilla?  It uses nsIOutputStream IIRC.

Comment 4

[Darin Fisher]

Or does this only apply to interfaces (w/ noscript methods) that may be
implemented by a JS component?  (In which case, sorry for the SPAM.)

Comment 5

[James Ross]

Is it important that nsISupports fails to be 'scriptable' with these conditions
(if I read the patch right)? Would that not make /everything/ non-scriptable?

Comment 6

[David Bradley]

I don't think it's limitted to just XPCOM components implemented in JS, this
would affect any JS object trying to satisfy an XPCOM interface. I'm also
wondering how this would affect the double wrapping that currently occurs when
converting from Native to JS. JS to Native isn't double wrapped, though.

Comment 7

[Mike Shaver (:shaver emeritus)]

To be clear, it's my _intent_ (and I think the effect of the patch) that this
only prevent JS objects from _implementing_ interfaces with [noscript] methods,
not from _manipulating_ interfaces with [noscript] methods.

I believe nsISupports to be special-cased for wrapper creation, but that belief
is not something I would bet body parts on.  I'll look deeper.

But I'm hearing a lot of WONTFIX (or INVALID?) support here.  Is there general
disagreement with my assertion that having JS code be able to implement only
_some_ parts of a scriptable interface is a recipe for pain and despair?  My
actual crash here is a result of poor error checking in the RDF template
builder, I think, so I would not feel completely ashamed if you all waved me
off.  I was planning to investigate more before filing, but didn't want to leave
all the state on my machine in Toronto while I'm travelling this week.

Comment 8

[John Bandhauer]

FWIW, I'd call this INVALID. XPConnect specifically does not test that a JS
implementation implements all methods of an interface. That was intentional.
[noscript] on a method precludes script participating in that given method (as
either caller or implementor). But, it does not preclude participating in the
interface as a whole. So, partially fulfilling the contract of the interface is
considered 'good enough' when JS is the caller. Why would this not be so when JS
is the implementor?

If an interface is factored incorectly then fix the interface. Don't exclude JS
from implemeting it as much as it can.

You might instead think in terms of informative warnings :)

Comment 9

[Mike Shaver (:shaver emeritus)]

I think the difference is that while a JS consumer of an interface can tell what
part of the interface's contract they need to invoke, a JS interface
_implementor_ can't predict what parts of the contract are safe for all callers.

But I'll take INVALID here!  I have learned my painful lessons about arguing
XPConnect semantic vision with jband!

Comment 10

[Darin Fisher]

(In reply to comment #9)
> I think the difference is that while a JS consumer of an interface can tell what
> part of the interface's contract they need to invoke, a JS interface
> _implementor_ can't predict what parts of the contract are safe for all callers.

Interesting example is nsIWebProgressListener.  When you hook up your JS
webprogresslistener to a webprogress instance, you can selectively tell it which
callbacks you are interested in (via the flags parameter to
addProgressListener).  This allows you to leave the other methods undefined,
which is very convenient.