Closed Bug 1426519 Opened 2 years ago Closed 2 years ago

Disable non-standard expression closure everywhere

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(2 files, 1 obsolete file)

So far I am not aware of any breakage caused by disabling this on Nightly. (bug 1319512). We should just disable this unconditionally and worst case we enable this again before release. The high telemetry usage we measured probably just comes from a fingerprinting script.
With this patch we have basically zero test coverage, which is of course quite unfortunate. Adding a runtime option for this in the parser seems like quite a bit of work. I think we used to do this for something, but I can't find the code at the moment.
Attachment #8938838 - Flags: review?(jdemooij)
Comment on attachment 8938838 [details] [diff] [review]
Disable expression closures everywhere

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

What do you think about adding an expressionClosuresEnabled() shell function so we still have test coverage when compiling with JS_HAS_EXPR_CLOSURES?

No fuzz/test coverage makes me a bit uncomfortable, but it's probably ok for now since this is almost certainly dead.
Attachment #8938838 - Flags: review?(jdemooij)
Priority: -- → P3
We will do this in Firefox 60 now.
Attachment #8938838 - Attachment is obsolete: true
Attachment #8945879 - Flags: review?(jdemooij)
Comment on attachment 8945880 [details] [diff] [review]
Disable expression closures everywhere, but allow dynamic enabling them for tests

I removed the macro, because it just makes this code just more complicated to write. I doubt anyone needs it.
Attachment #8945880 - Flags: review?(jdemooij)
Attachment #8945879 - Flags: review?(jdemooij) → review+
Comment on attachment 8945880 [details] [diff] [review]
Disable expression closures everywhere, but allow dynamic enabling them for tests

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

Great, thanks!
Attachment #8945880 - Flags: review?(jdemooij) → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/166a780e234f
Disable expression closures everywhere, but allow dynamic enabling them for tests. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/245e242e19af
Fix tests to explicitly enable expression closures. r=jandem
https://hg.mozilla.org/mozilla-central/rev/166a780e234f
https://hg.mozilla.org/mozilla-central/rev/245e242e19af
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.