Unnecessary cast between JSObject* and JSFunction* in CreateFunctionPrototype

RESOLVED FIXED in Firefox 62

Status

()

--
trivial
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: arai, Assigned: maharsh312, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla62
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 months ago
CreateFunctionPrototype calls NewFunctionWithProto and receives the return value as `JSObject* functionProto_`.

https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/js/src/vm/JSFunction.cpp#838-841
> static JSObject*
> CreateFunctionPrototype(JSContext* cx, JSProtoKey key)
> {
> ...
>     JSObject* functionProto_ =
>         NewFunctionWithProto(cx, nullptr, 0, JSFunction::INTERPRETED,
>                              enclosingEnv, nullptr, objectProto, AllocKind::FUNCTION,
>                              SingletonObject);

NewFunctionWithProto returns JSFunction*

https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/js/src/vm/JSFunction.h#813-817
> extern JSFunction*
> NewFunctionWithProto(JSContext* cx, JSNative native, unsigned nargs,
>                      JSFunction::Flags flags, HandleObject enclosingEnv, HandleAtom atom,
>                      HandleObject proto, gc::AllocKind allocKind = gc::AllocKind::FUNCTION,
>                      NewObjectKind newKind = GenericObject);

where JSFunction is the derived class of JSObject.

https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/js/src/vm/JSFunction.h#41
> class JSFunction : public js::NativeObject

https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/js/src/vm/NativeObject.h#465
> class NativeObject : public ShapedObject

https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/js/src/vm/ShapedObject.h#21
> class ShapedObject : public JSObject

So, the assignment above performs upcast.

and then it downcasts functionProto_ to JSFunction* with `&functionProto_->as<JSFunction>()` call.

https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/js/src/vm/JSFunction.cpp#845
> static JSObject*
> CreateFunctionPrototype(JSContext* cx, JSProtoKey key)
> {
> ...
>     RootedFunction functionProto(cx, &functionProto_->as<JSFunction>());

So, the upcast and downcast are completely unnecessary, and we can just receive the return value of NewFunctionWithProto into RootedFunction.



You'll need basic knowledge of C++, and be able to build and test Firefox [1] or SpiderMonkey [2].

A skilled first-time contributor should be able to finish this in a month.  Leave comments / questions here, or ask me (:arai) in IRC #introduction and #jsapi channels.

[1] https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
(Assignee)

Comment 1

10 months ago
Hi,

This is my first experience with open source community and I would like to work on this bug. Please guide me on how should I begin with and get the code base.
(Reporter)

Comment 2

10 months ago
Hi, thank you for your comment :D

please follow this document to setup the development environment (until the step just before running "./mach build"):
  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

and then follow this document to build SpiderMonkey shell:
  https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
(Assignee)

Comment 3

10 months ago
I have found a remove upcast and downcast JSFunction.cpp. Now, which specific test file should I run to check my changes?
(Reporter)

Comment 4

10 months ago
Great :)

First, please confirm that you can build with your change applied.

then, you need to run test which executes the code you've touched,
and actually CreateFunctionPrototype is executed while starting the shell, so any test is fine (in jstests, jit-test),
or just run JS shell and confirm that it doesn't crash.
(Assignee)

Comment 5

10 months ago
As I changed JSObject to JSFunction in "JSFunction.cpp", I was able to remove the upcast. But, I am getting an error during the build if I remove "->as<JSFunction>()" which performs downcast.

Can you help me with this?

Do tell me if something is wrong with my understanding of bug.
(Reporter)

Comment 7

10 months ago
Comment on attachment 8980536 [details] [diff] [review]
Bug 1463645 - Removed unnecessary upcast and downcast of JSFunction

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

Thank you for your patch :D

Please address the following comments, and then attach the updated patch, and set "review" flag to "?" and choose me as requestee next time!

::: js/src/vm/JSFunction.cpp
@@ +835,4 @@
>       * give it the guts to be one.
>       */
>      RootedObject enclosingEnv(cx, &self->lexicalEnvironment());
> +        

please remove spaces here,
and also please remove newline.

@@ +836,5 @@
>       */
>      RootedObject enclosingEnv(cx, &self->lexicalEnvironment());
> +        
> +    RootedFunction functionProto(cx, NewFunctionWithProto(cx, nullptr, 0, JSFunction::INTERPRETED,
> +                             							  enclosingEnv, nullptr, objectProto, AllocKind::FUNCTION,

please use spaces instead of tabs,
and align "e" of "enclosing" with "c" of "cx" in the above line.

@@ -838,5 @@
> -    JSObject* functionProto_ =
> -        NewFunctionWithProto(cx, nullptr, 0, JSFunction::INTERPRETED,
> -                             enclosingEnv, nullptr, objectProto, AllocKind::FUNCTION,
> -                             SingletonObject);
> -    if (!functionProto_)

please keep this null-check.
this case can happen if Out Of Memory (OOM) happens inside NewFunctionWithProto,
and this is to properly handle such case.
Attachment #8980536 - Flags: feedback+
(Reporter)

Updated

10 months ago
Assignee: nobody → maharsh312
Status: NEW → ASSIGNED
(Assignee)

Comment 8

10 months ago
Attachment #8980649 - Flags: review?(arai.unmht)
(Reporter)

Comment 9

10 months ago
Comment on attachment 8980649 [details] [diff] [review]
Bug 1463645- Added a nullcheck and improved the indentation

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

Great
Almost there!

Please fold this and the previous patches into single one.

::: js/src/vm/JSFunction.cpp
@@ +838,4 @@
>      RootedFunction functionProto(cx, NewFunctionWithProto(cx, nullptr, 0, JSFunction::INTERPRETED,
> +                                                          enclosingEnv, nullptr, objectProto,
> +                                                          AllocKind::FUNCTION,
> +                                                          SingletonObject));

this can be moved to previous line.
we're using 99-chars per line for code and 80-chars per line for comment.
Attachment #8980649 - Flags: review?(arai.unmht) → feedback+
(Reporter)

Comment 11

10 months ago
Comment on attachment 8980731 [details] [diff] [review]
Bug 1463645 - Removed unnecessary upcast and downcast of JSFunction (updated)

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

Excellent!
Thank you for fixing this :D

here's try run for automated testing:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd6605bd336b3094bb2609f26766da9239fc3b2e
once it completes without failure, you can add "checkin-needed" keyword to Tracking-Keywords field above,
(you can set multiple keywords separated by ",")
so that the patch is landed to mozilla-inbound which will be merged to mozilla-central shortly.

also, you can mark previous patches "obsolete" in Details link in attachments,
click "Details" next to attachment, click "edit details", check "obsolete", and "Submit".
so that it's clear which patch is the latest one.

for next time, when you submitting new attachment, you can mark them obsolete in the form, list under "Obsoletes"
Attachment #8980731 - Flags: review?(arai.unmht) → review+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed
(Assignee)

Updated

10 months ago
Attachment #8980536 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8980649 - Attachment is obsolete: true

Comment 12

10 months ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab890e8f2582
Removed unnecessary upcast and downcast of JSFunction r=arai
Keywords: checkin-needed

Comment 13

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ab890e8f2582
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.