Add a special jsval type to XPIDL

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Other Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 7 obsolete attachments)

25.88 KB, patch
jst
: review+
Details | Diff | Splinter Review
2.36 KB, patch
jst
: review+
Details | Diff | Splinter Review
79.06 KB, patch
jst
: review+
Details | Diff | Splinter Review
27.59 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
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.
(Assignee)

Updated

9 years ago
Assignee: general → nobody
Component: JavaScript Engine → XPCOM
QA Contact: general → xpcom

Comment 1

9 years ago
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.
(Assignee)

Comment 2

9 years ago
Created attachment 443491 [details] [diff] [review]
part 1, rename JSVal -> jsval in existing IDL - WIP v1
Assignee: nobody → jorendorff
(Assignee)

Comment 3

9 years ago
Created attachment 443492 [details] [diff] [review]
part 2, manual IDL cleanup - WIP v1
(Assignee)

Comment 4

9 years ago
Created attachment 443493 [details] [diff] [review]
part 3, support for [jsval]

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

Comment 5

9 years ago
Created attachment 443494 [details] [diff] [review]
part 4, some examples
(Assignee)

Comment 6

9 years ago
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.
(Assignee)

Comment 7

9 years ago
Created attachment 444773 [details] [diff] [review]
part 1, rename JSVal -> jsval in existing IDL - v2
Attachment #443491 - Attachment is obsolete: true
(Assignee)

Comment 8

9 years ago
Created attachment 444774 [details] [diff] [review]
part 2, manual IDL cleanup - v2

(Part 1 and part 2 are unchanged from WIP v1, but some line numbers changed in the meantime.)
Attachment #443492 - Attachment is obsolete: true
(Assignee)

Comment 9

9 years ago
Created attachment 444775 [details] [diff] [review]
part 3, support for [jsval] - v2

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

Comment 10

9 years ago
Created attachment 444778 [details] [diff] [review]
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
Attachment #443493 - Attachment is obsolete: true
Attachment #443494 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #444773 - Flags: superreview?(jst)
Attachment #444773 - Flags: review?(jst)
(Assignee)

Comment 11

9 years ago
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.)
Attachment #444774 - Flags: superreview?(jst)
Attachment #444774 - Flags: review?(jst)

Updated

9 years ago
Attachment #444773 - Flags: superreview?(jst)
Attachment #444773 - Flags: superreview+
Attachment #444773 - Flags: review?(jst)
Attachment #444773 - Flags: review+

Updated

9 years ago
Attachment #444774 - Flags: superreview?(jst)
Attachment #444774 - Flags: superreview+
Attachment #444774 - Flags: review?(jst)
Attachment #444774 - Flags: review+
(Assignee)

Comment 12

9 years ago
Whoops - For security reasons, we need to tighten up one of the wrapper classes before part 3 goes in.
Depends on: 565735

Comment 14

9 years ago
http://hg.mozilla.org/mozilla-central/rev/bff36a551ab7
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Wait... this isn't really fixed yet.
(Assignee)

Updated

9 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 16

9 years ago
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.
No longer depends on: 565735
(Assignee)

Comment 17

9 years ago
Created attachment 449382 [details] [diff] [review]
part 3, support for [jsval] - v3
Attachment #444775 - Attachment is obsolete: true
Attachment #449382 - Flags: review?(jst)
(Assignee)

Comment 18

9 years ago
Created attachment 449383 [details] [diff] [review]
part 4, some examples - v3
Attachment #444778 - Attachment is obsolete: true
Attachment #449383 - Flags: review?(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.
Attachment #449382 - Flags: review?(jst) → review+
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.
Attachment #449383 - Flags: review?(jst) → review+
(Assignee)

Comment 21

9 years ago
Created attachment 452149 [details] [diff] [review]
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.)
Attachment #449382 - Attachment is obsolete: true
Attachment #452149 - Flags: superreview?(jst)
Attachment #452149 - Flags: review+

Updated

9 years ago
Attachment #452149 - Flags: superreview?(jst) → superreview+

Updated

8 years ago
Blocks: 568863

Comment 24

8 years ago
http://hg.mozilla.org/mozilla-central/rev/500e65f68068
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.