Last Comment Bug 777219 - Prepare for converting the JSAPI to JSObject handles
: Prepare for converting the JSAPI to JSObject handles
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla17
Assigned To: Terrence Cole [:terrence]
:
:
Mentors:
Depends on: 783533
Blocks: ExactRooting
  Show dependency treegraph
 
Reported: 2012-07-24 19:58 PDT by Steve Fink [:sfink] [:s:] (PTO Sep23-28)
Modified: 2012-08-17 05:55 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make RawObject and RawConstObject typedefs for documenting parameters that do not require rooting (2.61 KB, patch)
2012-07-24 20:06 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
no flags Details | Diff | Splinter Review
Use JSHandleObject in the JSAPI (all but jsapi-tests) (385.33 KB, patch)
2012-07-24 20:07 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
no flags Details | Diff | Splinter Review
Use JSHandleObject in jsapi-tests (it works out differently than the rest of SM) (81.89 KB, patch)
2012-07-24 20:08 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
no flags Details | Diff | Splinter Review
Automated portion of step 3 (un-handlifying JSAPI) (165.68 KB, patch)
2012-07-24 20:08 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
no flags Details | Diff | Splinter Review
Manual portion of step 3 (un-handlifying the JSAPI) (60.81 KB, patch)
2012-07-24 20:09 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
no flags Details | Diff | Splinter Review
patch1 - Make RawObject typedef for documenting parameters that do not require rooting (2.54 KB, patch)
2012-07-26 15:51 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
terrence.d.cole: review+
Details | Diff | Splinter Review
patch 2 - Use JSHandleObject in the JSAPI (all but jsapi-tests) (408.12 KB, patch)
2012-07-26 15:51 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
no flags Details | Diff | Splinter Review
patch 3 - Use JSHandleObject in jsapi-tests (110.13 KB, patch)
2012-07-26 15:53 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
no flags Details | Diff | Splinter Review
patch 4 - Automated portion of step 3 (un-handlifying JSAPI) (160.00 KB, patch)
2012-07-26 15:54 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
no flags Details | Diff | Splinter Review
patch 5 - Manual portion of step 3 (un-handlifying the JSAPI) (58.12 KB, patch)
2012-07-26 15:54 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
no flags Details | Diff | Splinter Review
patch 4 - Automated portion of step 3 (un-handlifying JSAPI) (160.68 KB, patch)
2012-07-26 17:02 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
no flags Details | Diff | Splinter Review
Manual portion of step 3 (un-handlifying the JSAPI) (57.97 KB, patch)
2012-07-26 17:03 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
no flags Details | Diff | Splinter Review
patch 2 - Use JSHandleObject in the JSAPI (all but jsapi-tests) (411.21 KB, patch)
2012-07-27 12:59 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
no flags Details | Diff | Splinter Review
patch 3 - Use JSHandleObject in jsapi-tests (it works out differently than the rest of SM) (110.13 KB, patch)
2012-07-27 13:00 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
bhackett1024: review+
Details | Diff | Splinter Review
patch 4 - Automated portion of step 3 (un-handlifying JSAPI) (174.83 KB, patch)
2012-07-27 13:02 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
bhackett1024: review+
Details | Diff | Splinter Review
patch 5 - Manual portion of step 3 (un-handlifying the JSAPI) (70.37 KB, patch)
2012-07-27 13:03 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
bhackett1024: review+
Details | Diff | Splinter Review
script for automatically un-handlifying jsapi functions (4.25 KB, text/plain)
2012-07-27 13:28 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
no flags Details
patch 2 - Use JSHandleObject in the JSAPI (all but jsapi-tests) (411.21 KB, patch)
2012-07-27 17:05 PDT, Steve Fink [:sfink] [:s:] (PTO Sep23-28)
bhackett1024: review+
Details | Diff | Splinter Review

Description Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-24 19:58:25 PDT
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.
Comment 1 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-24 20:06:57 PDT
Created attachment 645627 [details] [diff] [review]
Make RawObject and RawConstObject typedefs for documenting parameters that do not require rooting
Comment 2 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-24 20:07:57 PDT
Created attachment 645628 [details] [diff] [review]
Use JSHandleObject in the JSAPI (all but jsapi-tests)
Comment 3 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-24 20:08:11 PDT
Created attachment 645629 [details] [diff] [review]
Use JSHandleObject in jsapi-tests (it works out differently than the rest of SM)
Comment 4 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-24 20:08:32 PDT
Created attachment 645630 [details] [diff] [review]
Automated portion of step 3 (un-handlifying JSAPI)
Comment 5 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-24 20:09:07 PDT
Created attachment 645631 [details] [diff] [review]
Manual portion of step 3 (un-handlifying the JSAPI)
Comment 6 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-26 15:51:13 PDT
Created attachment 646376 [details] [diff] [review]
patch1 - Make RawObject typedef for documenting parameters that do not require rooting
Comment 7 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-26 15:51:59 PDT
Created attachment 646377 [details] [diff] [review]
patch 2 - Use JSHandleObject in the JSAPI (all but jsapi-tests)
Comment 8 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-26 15:53:28 PDT
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.
Comment 9 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-26 15:54:19 PDT
Created attachment 646381 [details] [diff] [review]
patch 4 - Automated portion of step 3 (un-handlifying JSAPI)
Comment 10 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-26 15:54:56 PDT
Created attachment 646382 [details] [diff] [review]
patch 5 - Manual portion of step 3 (un-handlifying the JSAPI)
Comment 11 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-26 15:56:47 PDT
Note that all jstests, jit-tests, and jsapi-tests pass after this patch series, or after just patches 1-3.
Comment 12 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-26 17:02:33 PDT
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.
Comment 13 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-26 17:03:19 PDT
Created attachment 646417 [details] [diff] [review]
Manual portion of step 3 (un-handlifying the JSAPI)

...and rebased the manual portion on top.
Comment 14 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-26 18:54:42 PDT
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!
Comment 15 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-27 12:59:27 PDT
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.
Comment 16 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-27 13:00:18 PDT
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.
Comment 17 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-27 13:02:01 PDT
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.
Comment 18 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-27 13:03:37 PDT
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.
Comment 19 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-27 13:28:45 PDT
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.
Comment 20 Terrence Cole [:terrence] 2012-07-27 14:21:50 PDT
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.
Comment 21 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-07-27 17:05:17 PDT
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.
Comment 22 Brian Hackett (:bhackett) 2012-07-31 05:03:46 PDT
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.
Comment 23 Terrence Cole [:terrence] 2012-08-01 15:00:59 PDT
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
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-08-01 19:38:25 PDT
https://hg.mozilla.org/mozilla-central/rev/a91040f69ea3

Note You need to log in before you can comment on or make changes to this bug.