Closed Bug 560643 Opened 10 years ago Closed 9 years ago

Add a special jsval type to XPIDL

Categories

(Core :: XPCOM, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(4 files, 7 obsolete files)

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
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: general → nobody
Component: JavaScript Engine → XPCOM
QA Contact: general → xpcom
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: nobody → jorendorff
Attached patch part 3, support for [jsval] (obsolete) — Splinter Review
The qsgen.py support in here is, as far as I know, completely untested.
Attached patch part 4, some examples (obsolete) — Splinter Review
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.
(Part 1 and part 2 are unchanged from WIP v1, but some line numbers changed in the meantime.)
Attachment #443492 - Attachment is obsolete: true
Attached patch part 3, support for [jsval] - v2 (obsolete) — Splinter Review
This fixes some significant bugs in v1 and adds traceable quick stub support.
Attached patch part 4, some examples - v2 (obsolete) — Splinter Review
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
Attachment #444773 - Flags: superreview?(jst)
Attachment #444773 - Flags: review?(jst)
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)
Attachment #444773 - Flags: superreview?(jst)
Attachment #444773 - Flags: superreview+
Attachment #444773 - Flags: review?(jst)
Attachment #444773 - Flags: review+
Attachment #444774 - Flags: superreview?(jst)
Attachment #444774 - Flags: superreview+
Attachment #444774 - Flags: review?(jst)
Attachment #444774 - Flags: review+
Whoops - For security reasons, we need to tighten up one of the wrapper classes before part 3 goes in.
Depends on: 565735
http://hg.mozilla.org/mozilla-central/rev/bff36a551ab7
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Wait... this isn't really fixed yet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attached patch part 3, support for [jsval] - v3 (obsolete) — Splinter Review
Attachment #444775 - Attachment is obsolete: true
Attachment #449382 - Flags: review?(jst)
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+
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+
Attachment #452149 - Flags: superreview?(jst) → superreview+
Blocks: 568863
http://hg.mozilla.org/mozilla-central/rev/500e65f68068
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.