1491055 - Remove JS_New and use JS::Construct in JS_New consumers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Steve Fink [:sfink] [:s:]]

JS_New() accepts a HandleObject, wraps it in a Value with ObjectValue, then passes that to IsConstructor that first checks isObject(), redundantly.

Given that normally you would be looking up the constructor by name on the global, you will be getting back a Value not a JSObject*.

We should either add an overload that takes a HandleValue, or just change JS_New to take a HandleValue.

(I noticed this when reading through the cookbook example ConstructObjectWithNew.)

Comment 1

[Philip Chimento [:ptomato]]

I think having both overloads would be best, since you can also use JS_GetClassObject() to get the constructor.

Comment 2

[Jan de Mooij [:jandem]]

I think we should remove JS_New and use JS::Construct instead. And yes it already takes the function as HandleValue :)

Comment 3

[Tooru Fujisawa [:arai]]

makes sense

Comment 4

[anisha.rohra]

Hello. I'd like to work on this as my first bug. Is this still a bug that needs to be addressed? If so, I'd appreciate any pointers you can give me about where to start!

Comment 5

[Tooru Fujisawa [:arai]]

yes, this is still valid

https://searchfox.org/mozilla-central/search?q=symbol:_Z6JS_NewP9JSContextN2JS6HandleIP8JSObjectEERKNS1_16HandleValueArrayE&redirect=false

1. to work on this, you first need to be able to build Firefox [1].
2. once you've built, check the above link that shows definition/delaration/consumers of JS_New.
3. replace consumers of JS_New to use JS::Construct [2]
4. remove definition/declaration of JS_New
5. rebuild Firefox and see if it compiles
6. after that, submit your patch with moz-phab [3] (the document provides 2 options, moz-phab and arcanist, but I suggest using moz-phab)

if you have other questions, feel free to ask here, or in #jsapi channel in irc.mozilla.org [4]

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
[2] https://searchfox.org/mozilla-central/rev/3e0f1d95fcf8832413457e3bec802113bdd1f8e8/js/src/jsapi.h#1878-1905
[3] https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html
[4] https://wiki.mozilla.org/index.php?title=IRC

Comment 6

[anisha.rohra]

Working on it now, think I've found a fix and my build successfully compiles. However, I wonder if there is a method for me to test my implementation? And speaking of tests, should I remove the test cases/file testNewObject.cpp in my patch?

Comment 7

[Tooru Fujisawa [:arai]]

testNewObject should be rewritten to use JS::Construct.
you can run it by running jsapi-test in object directory, jsapi-test binary, with ./jsapi-test testNewObject command

Comment 8

[Rizwan Syed]

Hey, Is this bug open to being worked on? I'd like to get involved if so. I see it is unassigned. cheers :)

Comment 9

[Rizwan Syed]

(Please ignore my last comment- I didn't realise this was in C++, I thought it was in JS :) )

Comment 10

[anisha.rohra]

Attachment: fix Bug 1491055: Replace all instances of JS_New with JS::Construct

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 12

[achmurali]

Hi, can i work on this bug if it's still open.

Thanks,
Murali

Comment 13

[Tooru Fujisawa [:arai]]

Yes, it's still open.

For the details, see comment #5.
If you got any question, feel free to ask here, or in https://chat.mozilla.org/#/room/#spidermonkey:mozilla.org (irc.mozilla.org is discontinued)

Comment 14

[achmurali]

Could you please explain the reason to switch from JS::Construct to JS_New, wasn't able to figure it out from (https://bugzilla.mozilla.org/show_bug.cgi?id=1491055#c0).

Comment 15

[Tooru Fujisawa [:arai]]

this bug is about switching from JS_New to JS::Construct.

Reasons are the following:

• JS_New accepts HandleObject, but what it want is HandleValue
  • HandleObject is an alias of JS::Handle<JSObject*>
  • HandleValue is an alias of JS::Handle<JS::Value>
  • RootedObject is an alias of JS::Rooted<JSObject*>
  • RootedValue is an alias of JS::Rooted<JS::Value>
  • JSObject is a class that represents object in JS
  • JS::Value is a class that represents any value in JS
  • JS::Rooted is a class to register the value to GC's root list
  • JS::Handle is a reference to JS::Rooted and implicitly convertible from JS::Rooted
  • JSObject can be wrapped into JS::Value by ObjectValue
  • Whether JS::Value contains JSObject or not can be queried by JS::Value::isObject
  • JSObject inside JS::Value can be unwrapped by JS::Value::toObject
  • JS_New wraps the received HandleObject ctor into RootedValue ctorVal, and use it throughout the function (the RootedValue is implicitly converted to HandleValue when passing to other function)
  • Usually the constructor (ctor) is retrieved as JS::Value, so it's better just receive it as HandleValue, instead of letting the API consumer to convert it to HandleObject
• So, originally this bug was about to add a variant of JS_New that accepts HandleValue
  • actually, this doesn't apply to the remaining consumer, there it holds JSObject in nsJSObjWrapper::mJSObj
  • but in general, receiving HandleValue is better
• There's JS::Construct that does almost same thing as JS_New (for historical reason)
  • it matches the requirement (accepts HandleValue)
  • it has more callsites than JS_New
• So, this bug's purpose is changed to replace remaining JS_New consumer to use JS::Construct, and remove JS_New

Comment 16

[Sneha k]

Attachment: Bug 1491055 - Replaced consumers of JS_NEW to use JS::Construct and COMMENTED definitions and declaration of JS_NEW.

Comment 17

[Sneha k]

please review my changes

Comment 18

[achmurali]

(In reply to Tooru Fujisawa [:arai] from comment #15)

this bug is about switching from JS_New to JS::Construct.

Reasons are the following:

• JS_New accepts HandleObject, but what it want is HandleValue
  • HandleObject is an alias of JS::Handle<JSObject*>
  • HandleValue is an alias of JS::Handle<JS::Value>
  • RootedObject is an alias of JS::Rooted<JSObject*>
  • RootedValue is an alias of JS::Rooted<JS::Value>
  • JSObject is a class that represents object in JS
  • JS::Value is a class that represents any value in JS
  • JS::Rooted is a class to register the value to GC's root list
  • JS::Handle is a reference to JS::Rooted and implicitly convertible from JS::Rooted
  • JSObject can be wrapped into JS::Value by ObjectValue
  • Whether JS::Value contains JSObject or not can be queried by JS::Value::isObject
  • JSObject inside JS::Value can be unwrapped by JS::Value::toObject
  • JS_New wraps the received HandleObject ctor into RootedValue ctorVal, and use it throughout the function (the RootedValue is implicitly converted to HandleValue when passing to other function)
  • Usually the constructor (ctor) is retrieved as JS::Value, so it's better just receive it as HandleValue, instead of letting the API consumer to convert it to HandleObject
• So, originally this bug was about to add a variant of JS_New that accepts HandleValue
  • actually, this doesn't apply to the remaining consumer, there it holds JSObject in nsJSObjWrapper::mJSObj
  • but in general, receiving HandleValue is better
• There's JS::Construct that does almost same thing as JS_New (for historical reason)
  • it matches the requirement (accepts HandleValue)
  • it has more callsites than JS_New
• So, this bug's purpose is changed to replace remaining JS_New consumer to use JS::Construct, and remove JS_New

Thanks for the elaborate explanation :arai, maybe i need to look for another bug.

Comment 19

[Sneha k]

Attachment: Bug 1491055

Depends on D105875

Comment 20

[Sneha k]

Attachment: Bug 1491055 - Remove JS_New and use JS::Construct in JS_New consumers.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5860f15cc60
Remove JS_New and use JS::Construct in JS_New consumers. r=arai

Comment 22

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b5860f15cc60