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

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want P3])

Attachments

(1 attachment, 2 obsolete attachments)

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.
Created attachment 537276 [details] [diff] [review]
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.
Assignee: nobody → jonas
Attachment #537276 - Flags: review?(khuey)
How about instead of having [scriptable, scriptcanuseonly] you'd just have [script_usable] ?
I like that idea. It'll be a somewhat bigger patch, but should be ok.
Created attachment 537395 [details] [diff] [review]
Patch v2

Implements script_usable instead.
Attachment #537276 - Attachment is obsolete: true
Attachment #537276 - Flags: review?(khuey)
Attachment #537395 - Flags: review?(khuey)
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 ...
Attachment #537395 - Flags: review?(khuey) → review+
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.
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?
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).
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?
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.
Created attachment 539858 [details] [diff] [review]
Patch v3

This should work. Even includes a test!
Attachment #537395 - Attachment is obsolete: true
Attachment #539858 - Flags: review?(benjamin)
Comment on attachment 539858 [details] [diff] [review]
Patch v3

I'd love to also check this in xptcall also, but this seems good.
Attachment #539858 - Flags: review?(benjamin) → review+
Checked in. Thanks for the quick review!
http://hg.mozilla.org/mozilla-central/rev/1ba686855348
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
This was backed out in cset: 
http://hg.mozilla.org/mozilla-central/rev/995f2c3e7a78 
shouldn't this be re-opened ?
Indeed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
After this lands, can we go through and make a whole raft of DOM APIs builtinclass? That would help with issues like bug 664708.
Yup, that's the (maybe unwritten) plan!
Depends on: 664932
Keywords: dev-doc-needed

Updated

6 years ago
Whiteboard: [sg:want P3]
Blocks: 658714
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 :)
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Comment 20

6 years ago
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?
(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.

Updated

6 years ago
Blocks: 672389

Comment 22

6 years ago
> > 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 ;)
jcranmer already updated the documentation:

https://developer.mozilla.org/en/XPIDL#Interfaces
Keywords: dev-doc-needed → dev-doc-complete

Updated

5 years ago
Depends on: 761621
You need to log in before you can comment on or make changes to this bug.