Compile subscript loader scripts with noScriptRval by default

RESOLVED FIXED in Firefox 55

Status

()

Core
XPConnect
RESOLVED FIXED
a month ago
15 days ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks: 3 bugs, {addon-compat})

unspecified
mozilla55
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 wontfix, firefox55 fixed)

Details

(Whiteboard: [qf])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Compiling scripts to return a value disables optimizations and prevents JITting the top-level scope. Compiling with noScriptRval on my machine saves 10ms (or ~20%) on just loading the devtools startup code, and 20ms (16%) on overall startup.
Comment hidden (mozreview-request)
Blocks: 1356825
(In reply to Kris Maglione [:kmag] from comment #0)
> Compiling scripts to return a value disables optimizations and prevents
> JITting the top-level scope. Compiling with noScriptRval on my machine saves
> 10ms (or ~20%) on just loading the devtools startup code, and 20ms (16%) on
> overall startup.

Er, rather, and 20ms on add-on manager startup. 30ms total from the two combined.
I have some code cleanups to this file in progress in bug 1356666 and bug 1356799 you may have to rebase around.
(In reply to Andrew McCreight [:mccr8] from comment #3)
> I have some code cleanups to this file in progress in bug 1356666 and bug
> 1356799 you may have to rebase around.

No complaints from me. That file has been progressively turning into a nightmare since b2g started using it...

Comment 5

a month ago
mozreview-review
Comment on attachment 8858531 [details]
Bug 1356810: Use noScriptRval option by default for loadSubScript.

https://reviewboard.mozilla.org/r/130500/#review134000

::: js/xpconnect/idl/mozIJSSubScriptLoader.idl:45
(Diff revision 1)
>       *                      - target:  an object to evaluate onto (default: global object of the caller)
>       *                      - ignoreCache: if set to true, will bypass the cache for reading the file.
>       *                      - async: if set to true, the script will be loaded
>       *                        asynchronously, and a Promise is returned which
>       *                        resolves to its result when execution is complete.
> +     *                      - wantReturnValue: If true, the script will return

Would be nice to say that this defaults to true.
Attachment #8858531 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 6

a month ago
mozreview-review-reply
Comment on attachment 8858531 [details]
Bug 1356810: Use noScriptRval option by default for loadSubScript.

https://reviewboard.mozilla.org/r/130500/#review134000

> Would be nice to say that this defaults to true.

Well, it defaults to false :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c706be68a0742078cb76c03f58a8e9d01de2dec
Bug 1356810: Use noScriptRval option by default for loadSubScript. r=billm
(In reply to Kris Maglione [:kmag] from comment #6)
> Comment on attachment 8858531 [details]
> Bug 1356810: Use noScriptRval option by default for loadSubScript.
> 
> https://reviewboard.mozilla.org/r/130500/#review134000
> 
> > Would be nice to say that this defaults to true.
> 
> Well, it defaults to false :)

Oops, I misread the code for ParseBoolean. Aren't we at risk of breaking some add-ons this way? Should we at least flag this as addon-compat?
(In reply to Bill McCloskey (:billm) from comment #8)
> Oops, I misread the code for ParseBoolean. Aren't we at risk of breaking
> some add-ons this way? Should we at least flag this as addon-compat?

I searched DXR, and I couldn't find any add-ons that actually appeared to use the return value. The closest I could find was a bit of the SDK sandbox code that returns the value of loadSubScript when it's using that rather than evalInSandbox, but I don't think that's being used anywhere either.

I suppose there's an outside chance that someone is using it, though, so it can't hurt to add the flag.
Keywords: addon-compat
https://hg.mozilla.org/integration/mozilla-inbound/rev/93adb36bc1d2b22fb5357e6e5714626db3770e42
Bug 1356810: Follow-up: Fix xpcshell test for async subscript return value.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf8468e259dce509bfe0c4cc51874a0797932e36
Bug 1356810: Follow-up: Fix SDK worker tests.

Comment 12

a month ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/20dff607fb88
Follow-up: Fix SDK worker tests. a=merge
https://hg.mozilla.org/mozilla-central/rev/5c706be68a07
https://hg.mozilla.org/mozilla-central/rev/93adb36bc1d2
https://hg.mozilla.org/mozilla-central/rev/20dff607fb88
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 14

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf8468e259dc

Comment 15

a month ago
Just wondering if this patch has broken the add-on LastPass?  It's in the regression range I determined.  LP no longer fills in forms automatically.  LP has already been broken by another patch https://bugzilla.mozilla.org/show_bug.cgi?id=1356883 although auto fill was still working until this newer patch.

Comment 16

a month ago
I appears that whatever was causing the LP auto fill problem is no longer present on inbound.
Comment on attachment 8858531 [details]
Bug 1356810: Use noScriptRval option by default for loadSubScript.

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: This change significantly improves startup performance, and should help mitigate the startup performance regressions from enabling Screenshots on beta
[Is this code covered by automated tests?]: To some extent. Its consumers are covered by more tests than the interface itself.
[Has the fix been verified in Nightly?]: N/A
[Needs manual test from QE? If yes, steps to reproduce]: N/A
[List of other uplifts needed for the feature/fix]: None, but it will need some rebasing due to recent refactoring in the subscript loader. Uplifting bug 1356666 and bug 1356799 would help.
[Is the change risky?]: Low-risk
[Why is the change risky/not risky?]: This simply prevents loadSubScript from returning the result value of script evaluation. That feature was never widely-known or widely-used. The only real risk is that this may affect some add-ons, but it hasn't caused add-on issues so far, and dxr suggests that it's not likely to.
[String changes made/needed]: None
Attachment #8858531 - Flags: approval-mozilla-beta?
Blocks: 1356243

Updated

15 days ago
Blocks: 1363905
Screenshots will go live in Fx55 so we don't have to uplift these patches in 54. Beta54-. Mark 54 won't fix.
status-firefox54: --- → wontfix

Updated

15 days ago
Attachment #8858531 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.