661980 - Add support for scriptable-but-not-script-implementable interfaces

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Currently we have no way of making an interface scriptable while not letting script implementing it.

There are several reasons why we would want to do that:

* It's nice to be able to rely on that all instances of the interface is a
  "real" implementation. This is very much the case for nsIDOMNode for example
  where we constantly have to check that it's one of our nodes.
* The interfaces contains some [notxpcom] methods which the xptstub can't
  implement and so if they are called from C++ we'll end up crashing. This
  will very soon be the case for nsIDOMEventTarget
* Other. nsIAtom contains inline members and so implementing from script is
  guaranteed to crash.

Patch coming up.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

I included marking nsIDOMEventTarget as a test (which will be needed by the changes in bug 658714). And I marked nsIAtom since we really need to.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

How about instead of having [scriptable, scriptcanuseonly] you'd just have [script_usable] ?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I like that idea. It'll be a somewhat bigger patch, but should be ok.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2

Implements script_usable instead.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 537395 [details] [diff] [review]
Patch v2

This looks pretty good.  A couple of nits, and one test suggestion.

>diff --git a/js/src/xpconnect/src/xpcwrappedjsclass.cpp b/js/src/xpconnect/src/xpcwrappedjsclass.cpp
>-            if(NS_SUCCEEDED(info->IsScriptable(&canScript)) && canScript &&
>+            if(NS_SUCCEEDED(info->IsScriptimplementable(&canScript)) && canScript &&

80 lines?

>@@ -291,17 +291,17 @@ nsXPCWrappedJSClass::CallQueryInterfaceO
>-        if(NS_FAILED(info->IsScriptable(&canScript)) || !canScript)
>+        if(NS_FAILED(info->IsScriptimplementable(&canScript)) || !canScript)

80 lines?

>diff --git a/xpcom/reflect/xptinfo/public/nsIInterfaceInfo.idl b/xpcom/reflect/xptinfo/public/nsIInterfaceInfo.idl
> /* this is NOT intended to be scriptable */
>-[uuid(215DBE04-94A7-11d2-BA58-00805F8A5DD7)]
>+[uuid(7de126a2-ef4b-4e3b-a952-78ce4c133e38)]
> interface nsIInterfaceInfo : nsISupports
> {
>     readonly attribute string   name;
>     readonly attribute nsIIDPtr InterfaceIID;
> 
>     PRBool isScriptable();
>+    PRBool isScriptimplementable();

isScriptImplementable, with a capital I.

>diff --git a/xpcom/reflect/xptinfo/src/xptiprivate.h b/xpcom/reflect/xptinfo/src/xptiprivate.h
>@@ -283,16 +287,20 @@ public:
>     const nsID& IID() const { return mIID; }
> 
>     //////////////////////
>     // These non-virtual methods handle the delegated nsIInterfaceInfo methods.
> 
>     nsresult GetName(char * *aName);
>     nsresult GetIID(nsIID * *aIID);
>     nsresult IsScriptable(PRBool *_retval);
>+    nsresult IsScriptimplementable(PRBool *_retval) {
>+        *_retval = GetScriptimplementableFlag();
>+        return NS_OK;
>+    }

Move this one to wherever the rest of these are implemented.

Also, don't we want XPIDL to enforce that an iface with a [notxpcom] method is at most script_usable?  I didn't see that in the tests.

Other than that, looks great.  r=me.  You probably should check with bsmedberg to see if my review is enough here.  I'm not a peer ... but I don't think anyone active is ...

Comment 6

[Benjamin Smedberg]

I'm not sure I like this name, actually. What we really mean by this is not merely that *script* cannot implement the interface, we mean that nobody except the builtin Mozilla platform should implement the interface (at least I think that's what we want). This lets us cast to concrete classes without lots of extra checking.

I think the clearest way to express this would be [scriptable, builtinclass].

And we can then do things like have xptcall itself refuse to reflect builtin classes.

[notxpcom] means that this method isn't xptcall-compatible, and so it's certainly not scriptable in any way or form.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Won't there be situations where we'll want addons to be able to implement interfaces which are exposed to script, without also making it possible for script to implement those interfaces?

Comment 8

[Benjamin Smedberg]

By "addons" do you mean JS addons, or C++ addons? I don't think we should design any of this for the case of C++ addons. If you want JS addons to be able to implement these, then the utility is a little different, and I think we need to be explicit that they are only implementable by chrome but not content (and the name should reflect that).

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I meant C++ addons. JS addons should not be able to implement these interfaces since it can't implement the [notxpcom] methods on them.

I really don't care about the syntax enough to push one way or another. If
[scriptable, builtinclass] is preferred over [script_usable] then I'm happy to go back to the first patch in this bug.

I didn't quite understand the "And we can then do things like have xptcall itself refuse to reflect builtin classes" comment. Do you mean anything beyond what the patch already does?

Comment 10

[Benjamin Smedberg]

Yes, I mean that NS_GetXPTCallStub (http://mxr.mozilla.org/mozilla-central/source/xpcom/reflect/xptcall/public/xptcall.h#198) should refuse to implement "builtinclass" interfaces. This would affect not only JS scripting, but also XPCOM proxies, JavaXPCOM, and PyXPCOM. I think the first version is probably better.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v3

This should work. Even includes a test!

Comment 12

[Benjamin Smedberg]

Comment on attachment 539858 [details] [diff] [review]
Patch v3

I'd love to also check this in xptcall also, but this seems good.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in. Thanks for the quick review!

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

http://hg.mozilla.org/mozilla-central/rev/1ba686855348

Comment 15

[Jim Jeffery not reading bug-mail 1/2/11]

This was backed out in cset: 
http://hg.mozilla.org/mozilla-central/rev/995f2c3e7a78 
shouldn't this be re-opened ?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Indeed.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

After this lands, can we go through and make a whole raft of DOM APIs builtinclass? That would help with issues like bug 664708.

Comment 18

[Johnny Stenback (:jst)]

Yup, that's the (maybe unwritten) plan!

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in! http://hg.mozilla.org/mozilla-central/rev/8e30eba8ff64

We should definitely start marking interfaces as [builtinonly]. Please do file separate bugs on that, and I'm happy to review :)

Comment 20

[Jesse Ruderman]

What semantics did we settle on here?

Can you send a message to dev-platform or planet explaining when [builtinonly] should be used, perhaps encouraging people to file bugs in their modules?

Is it possible to get a list of functions exposed to web content through IDL, so we can find out which classes are most important to consider marking as [builtinonly]?

Did this patch implement the "make casting to concrete classes free" part, or will that be a followup?

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #20)
> What semantics did we settle on here?
> 
> Can you send a message to dev-platform or planet explaining when
> [builtinonly] should be used, perhaps encouraging people to file bugs in
> their modules?

Mail sent to dev-platform. Subject was "[builtinclass] in idl".

> Is it possible to get a list of functions exposed to web content through
> IDL, so we can find out which classes are most important to consider marking
> as [builtinonly]?

The only thing I can think of is to grep our tree for .idl files with [scriptable] but not [builtinclass] interfaces.

> Did this patch implement the "make casting to concrete classes free" part,
> or will that be a followup?

That's not really a separate feature. Once you know that your pointer points to an interface with only a single implementation, you can safely use static_cast<> to cast to that implementation.

Comment 22

[Jesse Ruderman]

> > Can you send a message to dev-platform or planet explaining when
> > [builtinonly] should be used, perhaps encouraging people to file bugs in
> > their modules?
> 
> Mail sent to dev-platform. Subject was "[builtinclass] in idl".

http://groups.google.com/group/mozilla.dev.platform/msg/b9b000850191e659 thanks

> > Did this patch implement the "make casting to concrete classes free" part,
> > or will that be a followup?
> 
> That's not really a separate feature. Once you know that your pointer points
> to an interface with only a single implementation, you can safely use
> static_cast<> to cast to that implementation.

Filed bug 672389 requesting a static analysis ;)

Comment 23

[Eric Shepherd [:sheppy]]

jcranmer already updated the documentation:

https://developer.mozilla.org/en/XPIDL#Interfaces