The default bug view has changed. See this FAQ.

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
(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 645627 [details] [diff] [review]
Make RawObject and RawConstObject typedefs for documenting parameters that do not require rooting
(Reporter)

Comment 2

5 years ago
Created attachment 645628 [details] [diff] [review]
Use JSHandleObject in the JSAPI (all but jsapi-tests)
(Reporter)

Comment 3

5 years ago
Created attachment 645629 [details] [diff] [review]
Use JSHandleObject in jsapi-tests (it works out differently than the rest of SM)
(Reporter)

Comment 4

5 years ago
Created attachment 645630 [details] [diff] [review]
Automated portion of step 3 (un-handlifying JSAPI)
(Reporter)

Comment 5

5 years ago
Created attachment 645631 [details] [diff] [review]
Manual portion of step 3 (un-handlifying the JSAPI)
(Reporter)

Comment 6

5 years ago
Created attachment 646376 [details] [diff] [review]
patch1 - Make RawObject typedef for documenting parameters that do not require rooting
Attachment #646376 - Flags: review?(bhackett1024)
(Reporter)

Updated

5 years ago
Attachment #645627 - Attachment is obsolete: true
(Reporter)

Comment 7

5 years ago
Created attachment 646377 [details] [diff] [review]
patch 2 - Use JSHandleObject in the JSAPI (all but jsapi-tests)
Attachment #646377 - Flags: review?(bhackett1024)
(Reporter)

Updated

5 years ago
Attachment #645628 - Attachment is obsolete: true
(Reporter)

Comment 8

5 years ago
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)
(Reporter)

Updated

5 years ago
Attachment #645629 - Attachment is obsolete: true
(Reporter)

Comment 9

5 years ago
Created attachment 646381 [details] [diff] [review]
patch 4 - Automated portion of step 3 (un-handlifying JSAPI)
Attachment #646381 - Flags: review?(bhackett1024)
(Reporter)

Updated

5 years ago
Attachment #645630 - Attachment is obsolete: true
(Reporter)

Comment 10

5 years ago
Created attachment 646382 [details] [diff] [review]
patch 5 - Manual portion of step 3 (un-handlifying the JSAPI)
Attachment #646382 - Flags: review?(bhackett1024)
(Reporter)

Updated

5 years ago
Attachment #645631 - Attachment is obsolete: true
(Reporter)

Comment 11

5 years ago
Note that all jstests, jit-tests, and jsapi-tests pass after this patch series, or after just patches 1-3.
(Reporter)

Comment 12

5 years ago
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)
(Reporter)

Updated

5 years ago
Attachment #646381 - Attachment is obsolete: true
Attachment #646381 - Flags: review?(bhackett1024)
(Reporter)

Comment 13

5 years ago
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)
(Reporter)

Updated

5 years ago
Attachment #646382 - Attachment is obsolete: true
Attachment #646382 - Flags: review?(bhackett1024)
(Reporter)

Comment 14

5 years ago
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]
(Reporter)

Comment 15

5 years ago
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)
(Reporter)

Updated

5 years ago
Attachment #646377 - Attachment is obsolete: true
Attachment #646377 - Flags: review?(bhackett1024)
(Reporter)

Comment 16

5 years ago
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)
(Reporter)

Updated

5 years ago
Attachment #646379 - Attachment is obsolete: true
Attachment #646379 - Flags: review?(bhackett1024)
(Reporter)

Comment 17

5 years ago
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)
(Reporter)

Updated

5 years ago
Attachment #646416 - Attachment is obsolete: true
Attachment #646416 - Flags: review?(bhackett1024)
(Reporter)

Comment 18

5 years ago
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)
(Reporter)

Updated

5 years ago
Attachment #646417 - Attachment is obsolete: true
Attachment #646417 - Flags: review?(bhackett1024)
(Reporter)

Comment 19

5 years ago
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.
(Reporter)

Updated

5 years ago
Attachment #646679 - Attachment description: Use JSHandleObject in the JSAPI (all but jsapi-tests) → patch 2 - Use JSHandleObject in the JSAPI (all but jsapi-tests)
(Reporter)

Updated

5 years ago
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)
(Reporter)

Updated

5 years ago
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+
(Reporter)

Comment 21

5 years ago
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)
(Reporter)

Updated

5 years ago
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.