1426519 - Disable non-standard expression closure everywhere

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Tom S [:evilpie]]

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.

Comment 1

[Tom S [:evilpie]]

Attachment: Disable expression closures everywhere

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.

Comment 2

[Jan de Mooij [:jandem]]

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.

Comment 3

[Tom S [:evilpie]]

We will do this in Firefox 60 now.

Comment 4

[Tom S [:evilpie]]

Attachment: Fix tests to explicitly enable expression closures

Comment 5

[Tom S [:evilpie]]

Attachment: Disable expression closures everywhere, but allow dynamic enabling them for tests

Comment 6

[Tom S [:evilpie]]

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.

Comment 7

[Jan de Mooij [:jandem]]

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!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/166a780e234f
https://hg.mozilla.org/mozilla-central/rev/245e242e19af

Comment 10

[Kohei Yoshino]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/expression-closure-support-has-been-removed/

Comment 11

[Florian Scholz (Open Web Docs)]

https://github.com/mdn/browser-compat-data/pull/1138
https://developer.mozilla.org/en-US/Firefox/Releases/60#Removals_from_the_web_platform
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Expression_closures