560643 - Add a special jsval type to XPIDL

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jason Orendorff [:jorendorff]]

WebIDL has the special "any" type meaning "any JavaScript value". We could use the proposed jsval type for that. It would also be handy in places where we're currently using XPCCallContext::GetArgv() or even nsIVariant.

jsvals would be subject to some rules regarding GC:

  - When a jsval is passed to an in parameter or setter,
    the caller must ensure that the value passed is GC-reachable (rooted)
    for the duration of the call.

  - When calling a method that returns a jsval, a getter that
    returns a jsval, or a method with a jsval out parameter,
    the caller must provide a pointer to a GC-root.

    As always with jsval roots, the caller must initialize it
    to a non-garbage value, and the callee must not put arbitrary
    garbage into that variable, even temporarily, because the GC
    requires a valid jsval there.

One missing piece I need in order to start implementing this is an existing interface, preferably a DOM interface, that could use it. That way I can test as I go.

Comment 1

[Robert Sayre]

as discussed on IRC, we have a bunch of code that actually returns jsvals using

http://mxr.mozilla.org/mozilla-central/ident?i=SetReturnValueWasSet

so that would be a good place to start.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: part 1, rename JSVal -> jsval in existing IDL - WIP v1

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: part 2, manual IDL cleanup - WIP v1

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: part 3, support for [jsval]

The qsgen.py support in here is, as far as I know, completely untested.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: part 4, some examples

Comment 6

[Jason Orendorff [:jorendorff]]

This is how far I got before having to switch to other stuff.

Parts 1 and 2 just set the stage a little bit. Part 1 just renames JSVal to jsval. It's strictly mindless string substitution, courtesy of perl. Part 2 contains the few manual fixups to put the jsval type in nsrootidl.idl along with astring and the rest.

Part 3 adds support for a special jsval type to XPT, XPConnect, xpidl, and qsgen. This part is not sufficiently tested. jsval return values are somewhat tested. The changes to support jsval arguments are not.

The changes to qsgen are also untested. Are there any methods that we would like to quickstub, but currently can't because they use SetReturnValueWasSet or GetArgv or nSIVariant?

Part 4 changes some interfaces to use jsval instead of SetReturnValueWasSet. My hope is that we can get rid of SetReturnValueWasSet.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: part 1, rename JSVal -> jsval in existing IDL - v2

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: part 2, manual IDL cleanup - v2

(Part 1 and part 2 are unchanged from WIP v1, but some line numbers changed in the meantime.)

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: part 3, support for [jsval] - v2

This fixes some significant bugs in v1 and adds traceable quick stub support.

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: part 4, some examples - v2

This adds some more examples that weren't in v1.

I ran these through the try server, but unfortunately the tracemonkey tree is a mess right now, so the results were kind of inconclusive. :-P

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 444774 [details] [diff] [review]
part 2, manual IDL cleanup - v2

These are the easy parts. As soon as I get a clean try server run, I'll ask for review on parts 3 and 4.

(If I should break part 4 down for reviews from individual module owners, please let me know.)

Comment 12

[Jason Orendorff [:jorendorff]]

Whoops - For security reasons, we need to tighten up one of the wrapper classes before part 3 goes in.

Comment 13

[Jason Orendorff [:jorendorff]]

Part 1: http://hg.mozilla.org/tracemonkey/rev/bff36a551ab7
Part 2: http://hg.mozilla.org/tracemonkey/rev/ccf4b3b2317c

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/bff36a551ab7

Comment 15

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

Wait... this isn't really fixed yet.

Comment 16

[Jason Orendorff [:jorendorff]]

Actually -- I think bug 565735 is completely independent of this because the new jsval type is no better or worse than using SetReturnValueWasSet or GetArgv or nsIVariant.

Comment 17

[Jason Orendorff [:jorendorff]]

Attachment: part 3, support for [jsval] - v3

Comment 18

[Jason Orendorff [:jorendorff]]

Attachment: part 4, some examples - v3

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 449382 [details] [diff] [review]
part 3, support for [jsval] - v3

Looks good, r=jst. Couple of whitespace nits noted below...

- In  CallMethodHelper::~CallMethodHelper()

             else if(dp->IsValCString())
                 delete (nsCString*) p;
-        }
+            else if(dp->IsValJSRoot())
+                JS_RemoveRoot(mCallContext, dp->ptr);
+        }   
     }
+

Remove the extra whitespace after the replaced '}' here.

- In xpcom/base/nsrootidl.idl:

 [ref, astring] native AString(ignored);
 [ref, astring] native AStringRef(ignored);
 [ptr, astring] native AStringPtr(ignored);
 
-native jsval(jsval);
+[jsval]       native jsval(jsval);

Add one more space so the 'native' keywords line up.

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 449383 [details] [diff] [review]
part 4, some examples - v3

Looks good! But this will conflict with dwitte's landing of bug 568059, but you should simply be able to remove the navigator.preference() related changes in this patch before landing.

Comment 21

[Jason Orendorff [:jorendorff]]

Attachment: part 3, support for [jsval] - v4

Carrying forward jst's r+, requesting sr.

This is the same as v3 but rebased to tip. (The JS_AddRoot API was changed to JS_AddValueRoot.)

Comment 22

[Jason Orendorff [:jorendorff]]

Part 3 landed Friday: http://hg.mozilla.org/tracemonkey/rev/500e65f68068

Comment 23

[Ben Newman (:bnewman) (:benjamn)]

Pushed to mozilla-central as well:
http://hg.mozilla.org/mozilla-central/rev/699c59338e5c

Comment 24

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/500e65f68068