Last Comment Bug 661980 - Add support for scriptable-but-not-script-implementable interfaces
: Add support for scriptable-but-not-script-implementable interfaces
Status: RESOLVED FIXED
[sg:want P3]
: dev-doc-complete
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
:
Mentors:
Depends on: 664932 761621
Blocks: 672389 658714
  Show dependency treegraph
 
Reported: 2011-06-03 17:01 PDT by Jonas Sicking (:sicking) PTO Until July 5th
Modified: 2012-06-05 08:13 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to fix (20.93 KB, patch)
2011-06-03 17:10 PDT, Jonas Sicking (:sicking) PTO Until July 5th
no flags Details | Diff | Review
Patch v2 (23.66 KB, patch)
2011-06-04 17:26 PDT, Jonas Sicking (:sicking) PTO Until July 5th
khuey: review+
Details | Diff | Review
Patch v3 (24.47 KB, patch)
2011-06-16 11:55 PDT, Jonas Sicking (:sicking) PTO Until July 5th
benjamin: review+
Details | Diff | Review

Description Jonas Sicking (:sicking) PTO Until July 5th 2011-06-03 17:01:58 PDT
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) PTO Until July 5th 2011-06-03 17:10:45 PDT
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.
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-06-03 17:30:14 PDT
How about instead of having [scriptable, scriptcanuseonly] you'd just have [script_usable] ?
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-03 17:31:15 PDT
I like that idea. It'll be a somewhat bigger patch, but should be ok.
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-04 17:26:51 PDT
Created attachment 537395 [details] [diff] [review]
Patch v2

Implements script_usable instead.
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-09 16:55:19 PDT
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 [:bsmedberg] 2011-06-10 09:02:32 PDT
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) PTO Until July 5th 2011-06-10 13:05:19 PDT
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 [:bsmedberg] 2011-06-10 13:07:29 PDT
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) PTO Until July 5th 2011-06-10 14:06:24 PDT
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 [:bsmedberg] 2011-06-10 20:35:57 PDT
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) PTO Until July 5th 2011-06-16 11:55:42 PDT
Created attachment 539858 [details] [diff] [review]
Patch v3

This should work. Even includes a test!
Comment 12 Benjamin Smedberg [:bsmedberg] 2011-06-16 11:58:51 PDT
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) PTO Until July 5th 2011-06-16 14:13:03 PDT
Checked in. Thanks for the quick review!
Comment 14 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-16 14:14:01 PDT
http://hg.mozilla.org/mozilla-central/rev/1ba686855348
Comment 15 Jim Jeffery not reading bug-mail 1/2/11 2011-06-16 18:11:50 PDT
This was backed out in cset: 
http://hg.mozilla.org/mozilla-central/rev/995f2c3e7a78 
shouldn't this be re-opened ?
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-06-16 19:32:13 PDT
Indeed.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-16 20:04:36 PDT
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, jst@mozilla.com) 2011-06-16 20:11:23 PDT
Yup, that's the (maybe unwritten) plan!
Comment 19 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-22 18:25:35 PDT
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 2011-06-22 23:43:45 PDT
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) PTO Until July 5th 2011-07-18 15:20:14 PDT
(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 2011-07-18 16:02:34 PDT
> > 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] 2011-08-17 08:07:20 PDT
jcranmer already updated the documentation:

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

Note You need to log in before you can comment on or make changes to this bug.