94650 - Should not allow a scriptable interface to inherit from a nonscriptable one

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P4)

Description

[Bradley Baetz (:bbaetz)]

Darin and I were trying to write a JS componant today which implemented 
nsIHttpNotify. We were having problems, and getting xpconnect errors whenever 
we tried to pass 'this' into a method taking an nsINetNotify.

It turned out that nsIHttpNotify was scriptable, but that interface inherits 
from nsINetNotify, which is not scriptable, due to a mistake when this 
interface was checked in in 1999. (I commited a fix this afternoon)

Is there a use for this? xpidl should at least warn when this situation occurs.

Comment 1

[David Bradley]

I think the short term thing to do is disallow or strongly warn against it.
Long term, I'm trying to come up with a reason to support it. Is there a reason
why anyone would want a scriptable interface to inherit from a non-scriptable
interface. To me it violates consistency of the interface across languages, but
on the other hand we have scriptable interfaces that expose methods not visible
to scripting language. So we already have the ability to create interfaces that
appear different from user to user.

Welcome input from others.

Comment 2

[Bradley Baetz (:bbaetz)]

I can't think of a use. js objects can't implement any interface which contains
a [noscript] element, because the c++ code would have problems. I don't know how
to warn about that, though, at compile time or at runtime.

(You could arrange to write c++ code which checked a seprate attribute to see if
the implementation was written in js, but thats just bad design, and we
shouldn't support that.)

Why not turn the warning on, and see if any of our code triggers the warning
which are not simple oversights (like this example was)? If there are no false
positives, and noone on npm.xpcom can think of a reason, then make it an error.

Comment 3

[John Bandhauer]

> I can't think of a use. js objects can't implement any interface which 
> contains a [noscript] element, because the c++ code would have problems.

This is not true. JSObjects can implement any [scriptable] interface. Calls to
[noscript] methods will simply fail. Sometimes this may make the JS object
incapable of *sufficiently* implementing a given interface. But this is not
categorical - lots of interfaces can be usefully utilized by calling only a
subset of the available methods.

A use for this functionality is when you want to inherit from an interface which
you can not change (may be it is frozen or already in the field) and you need
your inheriting interface to be scriptable.

An xpidl compiler warning may be in order. But, I don't think this is an error
condition.

dbradley might consider adding DEBUG only (and maybe switched on with a pref)
printfs or JSConsole messages to give greater detail for cases where data
conversions fail in xpcconvert (or in this case deeper in
nsXPCWrappedJS::GetNewOrUsed).

Comment 4

[Bradley Baetz (:bbaetz)]

jband: shaver has convinced me of the use for a js class to implement an
interface containing [noscript] methods.

What I meant to say was that theres no use for allowing a [scriptable] interface
to inherit off a nonscriptable one, since js can't do anything with that object,
even if it could get one.

JS certainly can't implement it, and pass itsself to a c++ function taking the
parent interface, which is where this problem started. I'd have to test, but I
suspect that it also can't be passed into js from c++ via a method which takes
the a paramater with the base class type. And if you can't do either of those,
then making the child class scriptable is useless.

As an aside, making a frozen interface scriptable won't change the c++ ABI, will
it? And if it wasn't scriptable to start with, it can't break any precompiled
scripts. Why can't you make a nonscriptable frozen interface scriptable, then
create a new interface inheriting from that for all the new attributes and methods?

Comment 5

[John Bandhauer]

> ... And if you can't do either of those, then making the child class 
> scriptable is useless.

But, it is not useless. It simply can not be applied to those uses. You *can* 
inherit from an existing interface and use that inherited interface. This has 
potential uses. There may come to be interfaces with methods that expect 
that inherited interface. The fact that there exists an ancestor interface 
that is not scriptable might be immaterial to an number of uses. Note that the 
inheritence chain may be arbitrarily deep and perhaps only *one* of the links in 
the chain is not scriptable. In the particular case you had the JSObject *could* 
have been passed as an nsISupports. In other cases there might be more 
interesting scriptable interfaces in the inheritence chain.

> Why can't you make a nonscriptable frozen interface scriptable...

One can argue that frozeness is not just about the ABI - it can be argued that 
frozenness is also about details like scriptablity.

Also, part of what might be deployed in the field is the xpt file in which the 
bit about scriptability is stored. You can not undo that with a cvs checkin.

Comment 6

[Mike Shaver (:shaver emeritus)]

I agree pretty much completely with jband here.  I once believed that an xpidl
error was appropriate, but jband's convinced me otherwise.

I'm not even sure that you shouldn't be able to pass a JSObject as a pointer to
a non-scriptable interface, and have XPConnect throw an error if
non-scriptable-interface methods are called, but all the examples I can think of
right now are pretty pathological.

Comment 7

[Bradley Baetz (:bbaetz)]

Hmm. OK, I think I've been convinced :)

Can we generate a warning, though? I would still guess that if this is happening
in the tree, its probably an error.

Comment 8

[Mike Shaver (:shaver emeritus)]

Warnings generally indicate that a construct is legal, but likely to be counter
to the programmer's intent (e.g. |if (a = b)|).  I don't like warnings for which
there is no mechanism by which the programmer can indicate that the construct is
intentional (|if ((a = b))| in the preceding example), personally, so I'd be
happiest with a warning that could be avoided by something like

interface ScriptableThing : [noscript] NonScriptableThing
{
  /* ... */
};

What do people think of that?
(Now I just have to go and make |if ((foo.bar))| not generate a strict warning
in JS...)

Comment 9

[David Bradley]

I'm fine with that construct as long as it falls within the confines of the
existing grammar. If not, then I guess there just won't be a way to silence the
warning. I'll take a look when I make the patch.

Comment 10

[David Bradley]

The following two interfaces nsIScriptSecurityManager and imgIDecoderObserver
inherit from non-scriptable interfaces. Since this is just a warning for now,
this won't break the build or anything. I also built ns as well.

Patch next.

Comment 11

[David Bradley]

Attachment: Adds warning about scriptable interfaces inheriting from non-scriptable interfaces

Comment 12

[Bradley Baetz (:bbaetz)]

imgIDecoderObserver looks like an oversight, except that nothing scriptable
takes that interface or its parent (which only has one method, which is
[noscript]) as a paramater, so it probably doesn't matter. Pav?

nsIScriptSecurityManager also doesn't need to be passed in from js to anything
taking an nsIXPCSecurityManager, and js code can't use nxIXPCSecurityManager anyway.

Comment 13

[Stuart Parmenter]

It seems as if it should still work, ignoring methods on non-scriptable 
interfaces, but still flattening scriptable interfaces together...  either way, 
imgIContainerObserver could be changed to be scriptable to fix this problem, 
however, i don't expect it to be used from script anytime soon.

Comment 14

[David Bradley]

Comment on attachment 55003 [details] [diff] [review]
Adds warning about scriptable interfaces inheriting from non-scriptable interfaces

Something odd happened, not sure how this ever compiled, new patch coming soon

Comment 15

[David Bradley]

Attachment: This one actually compiles

Comment 16

[David Bradley]

Can I get a r/sr for the patch.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 55049 [details] [diff] [review]
This one actually compiles

r=jst

Comment 18

[John Bandhauer]

Comment on attachment 55049 [details] [diff] [review]
This one actually compiles

sr=jband

Comment 19

[David Bradley]

patch checked in

Comment 20

[Phil Schwartau]

Marking Verified -