Prepare for converting the JSAPI to JSObject handles

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sfink, Assigned: terrence)

Tracking

unspecified
mozilla17
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(6 attachments, 12 obsolete attachments)

2.54 KB, patch
terrence
: review+
Details | Diff | Splinter Review
110.13 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
174.83 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
70.37 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
4.25 KB, text/plain
Details
411.21 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
As a step towards converting the JSAPI to use Handles everywhere necessary, I:

step 1: converted (almost) all JSObject pointers to JSHandleObject or JSMutableHandleObject

step 2: fixed up everything within SpiderMonkey that uses the public (or friend) API

step 3: partially reverted the change to maintain compatibility with Gecko

The end result is an unmodified JSAPI that properly roots everything, but it does the rooting just underneath the public API so that it will be straightforward to backout step 3 (all at once or incrementally) to have a fully handle-based JSAPI.

Note that this is not a complete Handle-ification; it only does JSObject pointers. In particular, jsval/Value is not done. That will be the next step. I haven't decided whether to update the patches or add another similar series on top.
Created attachment 645627 [details] [diff] [review]
Make RawObject and RawConstObject typedefs for documenting parameters that do not require rooting
Created attachment 645628 [details] [diff] [review]
Use JSHandleObject in the JSAPI (all but jsapi-tests)
Created attachment 645629 [details] [diff] [review]
Use JSHandleObject in jsapi-tests (it works out differently than the rest of SM)
Created attachment 645630 [details] [diff] [review]
Automated portion of step 3 (un-handlifying JSAPI)
Created attachment 645631 [details] [diff] [review]
Manual portion of step 3 (un-handlifying the JSAPI)
Created attachment 646376 [details] [diff] [review]
patch1 - Make RawObject typedef for documenting parameters that do not require rooting
Attachment #646376 - Flags: review?(bhackett1024)
Attachment #645627 - Attachment is obsolete: true
Created attachment 646377 [details] [diff] [review]
patch 2 - Use JSHandleObject in the JSAPI (all but jsapi-tests)
Attachment #646377 - Flags: review?(bhackett1024)
Attachment #645628 - Attachment is obsolete: true
Created attachment 646379 [details] [diff] [review]
patch 3 - Use JSHandleObject in jsapi-tests

The changes in jsapi-tests are a little weird, so separating out. This must land together with patch 2, though, or it won't compile.
Attachment #646379 - Flags: review?(bhackett1024)
Attachment #645629 - Attachment is obsolete: true
Created attachment 646381 [details] [diff] [review]
patch 4 - Automated portion of step 3 (un-handlifying JSAPI)
Attachment #646381 - Flags: review?(bhackett1024)
Attachment #645630 - Attachment is obsolete: true
Created attachment 646382 [details] [diff] [review]
patch 5 - Manual portion of step 3 (un-handlifying the JSAPI)
Attachment #646382 - Flags: review?(bhackett1024)
Attachment #645631 - Attachment is obsolete: true
Note that all jstests, jit-tests, and jsapi-tests pass after this patch series, or after just patches 1-3.
Created attachment 646416 [details] [diff] [review]
patch 4 - Automated portion of step 3 (un-handlifying JSAPI)

After reading billm's review of bug 776579, I updated the naming from obj_ => objArg.
Attachment #646416 - Flags: review?(bhackett1024)
Attachment #646381 - Attachment is obsolete: true
Attachment #646381 - Flags: review?(bhackett1024)
Created attachment 646417 [details] [diff] [review]
Manual portion of step 3 (un-handlifying the JSAPI)

...and rebased the manual portion on top.
Attachment #646417 - Flags: review?(bhackett1024)
Attachment #646382 - Attachment is obsolete: true
Attachment #646382 - Flags: review?(bhackett1024)
Ug. Also note that my automatic api reversion script only caught things named JS_ something, so the browser (eg xpconnect) won't compile with this. I'll either have to make the script more flexible or manually revert several dozen things. Doh!
Whiteboard: [js:t]
Created attachment 646679 [details] [diff] [review]
patch 2 - Use JSHandleObject in the JSAPI (all but jsapi-tests)

Updated, mostly to add JSRawObject in more places discovered while getting things to build with the browser.
Attachment #646679 - Flags: review?(bhackett1024)
Attachment #646377 - Attachment is obsolete: true
Attachment #646377 - Flags: review?(bhackett1024)
Created attachment 646681 [details] [diff] [review]
patch 3 - Use JSHandleObject in jsapi-tests (it works out differently than the rest of SM)

this probably didn't change much at all, but updating just in case.
Attachment #646681 - Flags: review?(bhackett1024)
Attachment #646379 - Attachment is obsolete: true
Attachment #646379 - Flags: review?(bhackett1024)
Created attachment 646682 [details] [diff] [review]
patch 4 - Automated portion of step 3 (un-handlifying JSAPI)

Added a bunch of things in for jsfriendapi.h, which doesn't use a leading JS_ so I was missing it before.

Note that this patch is generated automatically, and so shouldn't require much more than a cursory review.
Attachment #646682 - Flags: review?(bhackett1024)
Attachment #646416 - Attachment is obsolete: true
Attachment #646416 - Flags: review?(bhackett1024)
Created attachment 646683 [details] [diff] [review]
patch 5 - Manual portion of step 3 (un-handlifying the JSAPI)

This is mainly what changed in order to get the browser building. I still have not run this through try; will do that next.
Attachment #646683 - Flags: review?(bhackett1024)
Attachment #646417 - Attachment is obsolete: true
Attachment #646417 - Flags: review?(bhackett1024)
Created attachment 646689 [details]
script for automatically un-handlifying jsapi functions

just in case it's helpful to some poor sucker needing to rebase this stuff (hi terrence!), this is the script I use to generate the automated portion. Run it with no arguments from within js/src.
Attachment #646679 - Attachment description: Use JSHandleObject in the JSAPI (all but jsapi-tests) → patch 2 - Use JSHandleObject in the JSAPI (all but jsapi-tests)
Attachment #646681 - Attachment description: Use JSHandleObject in jsapi-tests (it works out differently than the rest of SM) → patch 3 - Use JSHandleObject in jsapi-tests (it works out differently than the rest of SM)
Attachment #646683 - Attachment description: Manual portion of step 3 (un-handlifying the JSAPI) → patch 5 - Manual portion of step 3 (un-handlifying the JSAPI)
(Assignee)

Comment 20

5 years ago
Comment on attachment 646376 [details] [diff] [review]
patch1 - Make RawObject typedef for documenting parameters that do not require rooting

Review of attachment 646376 [details] [diff] [review]:
-----------------------------------------------------------------

I've already done half of this, so I'm going to steal the r+.

::: js/src/gc/Root.h
@@ +224,5 @@
>  /*
> + * Raw pointer used as documentation that a parameter does not need to be
> + * rooted.
> + */
> +typedef JSObject *                  RawObject;

I've already got this and equivalent comment checked into central, farther down in the file.
Attachment #646376 - Flags: review?(bhackett1024) → review+
Created attachment 646765 [details] [diff] [review]
patch 2 - Use JSHandleObject in the JSAPI (all but jsapi-tests)

Sorry for the patch churn, but try server found one stupid thing I messed up in CTypes.cpp. This incorporates it. This is also rebased onto the latest greatest m-i.
Attachment #646765 - Flags: review?(bhackett1024)
Attachment #646679 - Attachment is obsolete: true
Attachment #646679 - Flags: review?(bhackett1024)
Attachment #646681 - Flags: review?(bhackett1024) → review+
Attachment #646765 - Flags: review?(bhackett1024) → review+
Comment on attachment 646682 [details] [diff] [review]
patch 4 - Automated portion of step 3 (un-handlifying JSAPI)

Review of attachment 646682 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jstypedarray.cpp
@@ +3302,2 @@
>  {
> +    RootedObject obj_(cx, obj_Arg);

Pre-existing it seems, but the script kind of made a hash of these obj_ args, which should be renamed I think.
Attachment #646682 - Flags: review?(bhackett1024) → review+
Attachment #646683 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 23

5 years ago
And we have a green try run:
https://tbpl.mozilla.org/?tree=Try&rev=950a3d974aa6

And we are pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a91040f69ea3
(Assignee)

Updated

5 years ago
Assignee: general → terrence
Blocks: 753203
https://hg.mozilla.org/mozilla-central/rev/a91040f69ea3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17

Updated

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