Use ::mozilla::Maybe to de-duplicate the loop of ScriptLoader::GiveUpBytecodeEncoding

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
2 years ago
8 days ago

People

(Reporter: nbp, Assigned: waterftw, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla55
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
In the dom/script/ScriptLoader.cpp, the function named ScriptLoader::GiveUpBytecodeEncoding has 2 loops which are mostly doing the same things, apart from the initialization of the AutoEntryScript structure.

Using ::mozilla::Maybe defined in mbft/Maybe.h, wrap the AutoEntryScript structure such that it can be initialize in a branch, and used after, in a single loop which contain a branch around FinishIncrementalEncoding, which test if the AutoEntryScript got initialized.
(Assignee)

Comment 1

2 years ago
Hi Nicolas, can you assign me this bug?
(Reporter)

Comment 2

2 years ago
(In reply to Stanley [:waterftw] from comment #1)
> Hi Nicolas, can you assign me this bug?

Done.

If you need any extra guidance, you can ask on irc.mozilla.org, on the #jsapi channel, either Blake (mrbkap) or me (nbp).
Assignee: nobody → stanleyye1996
(Reporter)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

2 years ago
Also not too sure about the review flags..
Attachment #8874267 - Flags: review?(nicolas.b.pierron)
Attachment #8874267 - Flags: feedback?(nicolas.b.pierron)
(Reporter)

Comment 4

2 years ago
Comment on attachment 8874267 [details] [diff] [review]
Wrapped the RootedScript and AutoEntryScript classes inside a Maybe container

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

::: dom/script/ScriptLoader.cpp
@@ +2190,5 @@
>    // removal of all request from the current list and these large buffers would
>    // be removed at the same time as the source object.
>    nsCOMPtr<nsIScriptGlobalObject> globalObject = GetScriptGlobalObject();
> +  Maybe<AutoEntryScript> aes;
> +  Maybe<JS::RootedScript> script;

nit: JS::RootedScript are creating a link list of Rooted Script which are on the same stack.  Having this class under a Maybe would be dangerous if we were to have another Rooted in the same frame, and if we were to not respect the RAII order.

Having a single JS::RootedScript should be fine, but for future-proof safety, I suggest to move the JS::RootedScript inside the if branch instead of having a Maybe<> surrounding it.
Attachment #8874267 - Flags: review?(nicolas.b.pierron)
Attachment #8874267 - Flags: review+
Attachment #8874267 - Flags: feedback?(nicolas.b.pierron)
(Assignee)

Comment 5

2 years ago
Attachment #8874715 - Flags: review?(nicolas.b.pierron)
(Reporter)

Comment 6

2 years ago
Comment on attachment 8874715 [details] [diff] [review]
Revision 1:  Removed the Maybe container for RootedScript

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

This diff looks good. Make a proper patch, such that your patch can be imported and landed for you, while keeping the authorship.

Here is the documentation for making a patch that someone else can commit on your behalf:
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: dom/script/ScriptLoader.cpp
@@ +2205,5 @@
>      TRACE_FOR_TEST_NONE(request->mElement, "scriptloader_bytecode_failed");
> +
> +    if (aes.isSome()) {
> +      JS::RootedScript script(aes->cx());
> +      script.set(request->mScript);

nit: JS::RootedScript script(aes->cx(), request->mScript);
Attachment #8874715 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 7

2 years ago
Attachment #8874267 - Attachment is obsolete: true
Attachment #8874715 - Attachment is obsolete: true
Attachment #8875182 - Flags: checkin?(nicolas.b.pierron)

Comment 8

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3666d91a4263
Use ::mozilla::Maybe to de-duplicate the loop of ScriptLoader::GiveUpBytecodeEncoding r=nbp
(Reporter)

Comment 9

2 years ago
Comment on attachment 8875182 [details] [diff] [review]
Bug1367515.patch

(based on comment 8)
Attachment #8875182 - Flags: review+
Attachment #8875182 - Flags: checkin?(nicolas.b.pierron)
Attachment #8875182 - Flags: checkin+
(Reporter)

Comment 10

2 years ago
Thanks you for your contribution!
If you are interested in contributing again, you can find more mentored issues at https://www.joshmatthews.net/bugsahoy/?dom=1
https://hg.mozilla.org/mozilla-central/rev/3666d91a4263
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.