Closed
Bug 1083458
Opened 10 years ago
Closed 7 years ago
Remove SpiderMonkey support for expression closures (shorthand function syntax)
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: cpeterson, Assigned: evilpies)
References
(Blocks 1 open bug)
Details
(Whiteboard: [DocArea=JS])
Attachments
(5 files, 3 obsolete files)
7.21 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
12.45 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
3.36 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
3.91 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
7.85 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
Updated•10 years ago
|
Component: General → JavaScript Engine
Comment 1•10 years ago
|
||
MDN content has been moved to a separate page to mark this clearly as non-standard/deprecated.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Expression_closures
Updated•10 years ago
|
Keywords: site-compat
Comment 2•9 years ago
|
||
Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/expression-closure-support-will-be-removed/
Comment 3•8 years ago
|
||
current telemetry result
CONTENT
| Nightly | Aurora | Beta | Release
---+---------+---------+---------+----------
45 | 55.72k | 150.11k | 1.23M | 1.88M
46 | 47.26k | 110.75k | 1.79M | 1.64M
47 | 54.42k | 136.06k | 2.13M | 2.82M
48 | 61.91k | 148.77k | 2.16M | 6.25M
49 | 45.91k | 107.39k | 4.63M | 20.49M
50 | 45.41k | 215.39k | 13.41M | -
51 | 52.06k | 667.19k | - | -
52 | 450.70k | - | - | -
ADDON
| Nightly | Aurora | Beta | Release
---+---------+---------+---------+---------
45 | 93.77k | 195.58k | 170.83k | 97.52k
46 | 56.12k | 208.61k | 94.02k | 63.28k
47 | 55.44k | 200.31k | 78.3k | 231.89k
48 | 77.46k | 158.59k | 175.29k | 280.96k
49 | 60.46k | 209.24k | 126.68k | 3.83M
50 | 59.29k | 162.04k | 1.24M | -
51 | 19.68k | 252.67k | - | -
52 | 88.94k | - | - | -
not sure what happens tho, it hits spike on last cycle (that is not specific version)...
anyway, we don't observe any reduction after showing warning (bug 995610) :/
Reporter | ||
Comment 4•8 years ago
|
||
I have actually seen this expression closure warning on a real website! The following BBC News page very intermittently logs an expression closure warning, depending on which ads (from Rubicon Project ad network) are loaded.
http://www.bbc.com/news/world-us-canada-38656814
JavaScript warning: https://cdn.adsafeprotected.com/sca.17.1.7.js, line 1: expression closures are deprecated
The script is using eval() to detect support for expression closures, and other browser features, to fingerprint users. Search for "expressionClosure" in the attached sca.17.1.7.js file. This probably explains why expression closures are so common in JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT telemetry: https://mzl.la/2jaLfZL
JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_ADDONS telemetry reports plenty of add-ons still using expression closures, but I think it's worth testing whether we can at least hide expression closures from regular web content. Expression closures are a deprecated language feature we'd like to remove and are actively being used to fingerprint users. I realize that detecting expression closure support does not contribute any meaningful entropy for fingerprinting (because all Firefox users support expression closures and no non-Firefox users do).
Comment 5•8 years ago
|
||
I'd like to remove these from my addon. Here is how it's used:
getHelperForLanguage: function(l) null
Is this just a function returning null? Can I replace that with:
getHelperForLanguage: function() {return null;}
Reporter | ||
Comment 6•8 years ago
|
||
(In reply to Eric H. Jung from comment #5)
> I'd like to remove these from my addon. Here is how it's used:
>
> getHelperForLanguage: function(l) null
>
> Is this just a function returning null? Can I replace that with:
>
> getHelperForLanguage: function() {return null;}
Yes and your change is correct.
In your particular example defining a function as part of an object literal, you could also use an ES6's shorter method definition syntax like:
getHelperForLanguage() {return null;},
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions
Comment 7•8 years ago
|
||
Or `getHelperForLanguage: () => null`
Comment 8•8 years ago
|
||
Shorthand method definitions introduced in Firefox 33, still not available in IE or whatever it is called now.
The important bit related to this bug is placing the curly brackets around whatever expression is to be returned and using the return keyword (with or without a semi-colon).
Comment 9•8 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #6)
> Yes and your change is correct.
>
> In your particular example defining a function as part of an object literal,
> you could also use an ES6's shorter method definition syntax like:
>
> getHelperForLanguage() {return null;},
>
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/
> Method_definitions
Thanks. This will be removed from FoxyProxy Standard in its next release (4.6) and FoxyProxy Basic in its next release (3.6)
Comment 10•8 years ago
|
||
telemetry table generated by the script in bug 1083470 comment #6
CONTENT
| nightly | aurora | beta | release
----+------------+------------+------------+------------
45 | 59745 | 343200 | 1391066 | 2034283
46 | 48009 | 112849 | 2025797 | 1729146
47 | 55504 | 138089 | 2421614 | 3319298
48 | 64231 | 158405 | 2641369 | 7590326
49 | 47739 | 111023 | 4958256 | 21556859
50 | 47139 | 221618 | 13979525 | 23663556
51 | 54845 | 719252 | 13876764 | 6371716
52 | 462630 | 620659 | 3829983 | 22
53 | 425658 | 190734 | - | 1
54 | 109505 | - | - | -
ADDONS
| nightly | aurora | beta | release
----+------------+------------+------------+------------
45 | 95761 | 197887 | 219781 | 417992
46 | 57778 | 209156 | 199249 | 191959
47 | 55785 | 202172 | 288088 | 1809065
48 | 88436 | 168291 | 462228 | 1958165
49 | 74099 | 221353 | 287324 | 5265871
50 | 82582 | 186774 | 1634315 | 92150325
51 | 42056 | 325700 | 15330684 | 25679298
52 | 144776 | 1684192 | 5038724 | -
53 | 1231626 | 558765 | - | -
54 | 358656 | - | - | -
(note: latest versions don't have enough data to compare)
Comment 11•7 years ago
|
||
CONTENT
| nightly | aurora | beta | release
----+------------+------------+------------+------------
45 | 63387 | 472713 | 1501298 | 2138490
46 | 48548 | 114212 | 2170640 | 1797932
47 | 55810 | 141089 | 2613280 | 3702689
48 | 67800 | 171864 | 3495916 | 9282126
49 | 49071 | 113332 | 5231487 | 22042456
50 | 48127 | 225680 | 14234711 | 24530855
51 | 56322 | 728323 | 14458774 | 15946811
52 | 468205 | 652239 | 8399492 | 14079882
53 | 440372 | 526532 | 8879646 | 17741299
54 | 222092 | 1018247 | 11810352 | 1045938
55 | 576031 | 415 | 203703 | -
56 | 52932 | - | - | -
ADDONS
| nightly | aurora | beta | release
----+------------+------------+------------+------------
45 | 96289 | 198353 | 250751 | 674045
46 | 57781 | 209270 | 249379 | 320672
47 | 56830 | 202477 | 578530 | 3153536
48 | 91775 | 178833 | 779363 | 3881537
49 | 90025 | 229458 | 442268 | 5774554
50 | 85606 | 200705 | 1863232 | 94209611
51 | 51140 | 345110 | 15767022 | 54204632
52 | 154409 | 1785597 | 10208301 | 51481376
53 | 1296902 | 1222045 | 9225468 | 68124841
54 | 673167 | 1844268 | 12196295 | 4912235
55 | 1398464 | 23213 | 518631 | -
56 | 116199 | - | - | -
(note: latest versions don't have enough data to compare)
Comment 12•7 years ago
|
||
CONTENT
| nightly | aurora | beta | release
----+------------+------------+------------+------------
45 | 65825 | 488304 | 1562563 | 2190818
46 | 48853 | 115012 | 2249984 | 1835235
47 | 56154 | 143167 | 2722575 | 3894112
48 | 69613 | 180443 | 4066094 | 10351494
49 | 49644 | 114778 | 5406076 | 22271369
50 | 48827 | 227295 | 14366430 | 25074950
51 | 57240 | 733307 | 14700508 | 16143939
52 | 470268 | 663610 | 8512646 | 14516458
53 | 446556 | 573104 | 9052273 | 18449700
54 | 224067 | 1043255 | 12846193 | 19416714
55 | 595912 | 7975 | 11642883 | 12437826
56 | 325609 | 14292 | 8394049 | -
57 | 550462 | - | - | -
ADDONS
| nightly | aurora | beta | release
----+------------+------------+------------+------------
45 | 96546 | 199130 | 282454 | 771377
46 | 57783 | 209471 | 284954 | 384743
47 | 56980 | 202494 | 687073 | 3871930
48 | 92822 | 180327 | 948996 | 4832679
49 | 96946 | 230072 | 515861 | 5939663
50 | 92087 | 207486 | 1946932 | 94619533
51 | 56986 | 347886 | 15944034 | 54965267
52 | 155165 | 1810108 | 10262028 | 52136309
53 | 1369975 | 1252034 | 9321603 | 70237351
54 | 673297 | 1940356 | 12883840 | 67360509
55 | 1441130 | 62231 | 10422325 | 39430176
56 | 611217 | 25221 | 7184891 | -
57 | 121557 | - | - | -
Comment 13•7 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #4)
> The script is using eval() to detect support for expression closures, and
> other browser features, to fingerprint users. Search for "expressionClosure"
> in the attached sca.17.1.7.js file. This probably explains why expression
> closures are so common in JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT
> telemetry: https://mzl.la/2jaLfZL
That makes sense. I think we should try removing support for expression closures on Nightly after the merge next week, then we can see if it breaks anything.
Comment 14•7 years ago
|
||
CONTENT
| nightly | aurora | beta | release
----+------------+------------+------------+------------
52 | 470609 | 667523 | 8541451 | 14592467
53 | 448157 | 581558 | 9093863 | 18514282
54 | 224927 | 1047737 | 12939087 | 19548228
55 | 599048 | 8023 | 11787706 | 13714351
56 | 330849 | 14917 | 8908219 | 10469463
57 | 560252 | 28214 | 9478448 | 62
58 | 625411 | 65270 | 88 | 17
ADDONS
| nightly | aurora | beta | release
----+------------+------------+------------+------------
52 | 155313 | 1820362 | 10292035 | 52296698
53 | 1411538 | 1256300 | 9347465 | 70430425
54 | 673350 | 1946785 | 13051554 | 67768977
55 | 1448950 | 62231 | 10535246 | 43112554
56 | 635965 | 26771 | 7735926 | 31036552
57 | 142467 | 4137 | 1109 | 1
58 | 35348 | 13 | - | -
Assignee | ||
Comment 16•7 years ago
|
||
We already disabled expression closures in bug 1426519. We can completely remove the disabled code after this reaches stable (Firefox 60) and everything looks okay.
Assignee: nobody → evilpies
Updated•7 years ago
|
Keywords: dev-doc-needed,
site-compat
Assignee | ||
Comment 17•7 years ago
|
||
This mostly just removes the code that was disabled in bug 1426519, but there is more stuff to delete.
Especially this:
- // If we remove expression closure, we can remove isExprBody flag from
- // LazyScript and JSScript.
Attachment #8961064 -
Flags: review?(jdemooij)
Assignee | ||
Comment 18•7 years ago
|
||
Attachment #8961065 -
Flags: review?(jdemooij)
Comment 19•7 years ago
|
||
Comment on attachment 8961064 [details] [diff] [review]
Remove basic expression closure code
Review of attachment 8961064 [details] [diff] [review]:
-----------------------------------------------------------------
\o/
Will you land this in Firefox 61 or 62? Comment 16 suggests 62 but IMO we could land this now.
Attachment #8961064 -
Flags: review?(jdemooij) → review+
Updated•7 years ago
|
Attachment #8961065 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 20•7 years ago
|
||
Thanks for the review. I actually forgot about that comment, we are probably clear to land this now.
Assignee | ||
Comment 21•7 years ago
|
||
This patch isn't finished, because Reflect.parse actually needs isExprBody, but I am not sure what to replace it with. I probably need to look deeper into how we parse arrow functions to fix it myself.
This line is wrong: isExpression = type == AST_ARROW_EXPR. Apparently we need/want to tell the difference between () => 1 and () => { return 1; }.
Keywords: leave-open
Comment 22•7 years ago
|
||
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dce3a6c9866b
Remove basic expression closure code. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cac0a8b22db
Remove expression closures from tests. r=jandem
Comment 23•7 years ago
|
||
bugherder |
Assignee | ||
Comment 24•7 years ago
|
||
So the basic problem here is the "expression" value in Reflect.parse.
For () => 1 this should be true, but false for () => { return 1 } it should be false, because the right hand side is a block and not an expression.
However for `async () => 1` we build the same StatementList as `async () => { return 1`. I solved this by pattern matching, which is incorrect for that case. I think we could also keep isExprBody on FunctionBox?
Attachment #8961331 -
Attachment is obsolete: true
Attachment #8964182 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 25•7 years ago
|
||
Attachment #8964183 -
Flags: review?(jdemooij)
Comment 26•7 years ago
|
||
Comment on attachment 8964182 [details] [diff] [review]
Remove isExprBody
Review of attachment 8964182 [details] [diff] [review]:
-----------------------------------------------------------------
I'd be happier if this were done in steps. Remove {is,note}ExpressionClosure as one standalone patch and land that. Then, in a next patch, after Searchfox has updated so we can reasonably see what's dead/not-dead/barely-used, start removing the exprbody stuff.
I don't think I'm okay with removing exprbody in advance of figuring out a different way to record arrow functions with expression bodies, tho -- which I gather from the async.js changes is what this patch would produce. I'd be inclined to see if we can't figure out a way to split up the is-arrow-function representation into is-arrow-block and is-arrow-expression, but I'm not at all sure of that. Right now there's too much moving parts to this patch for me to be sure I grasp it all well.
Attachment #8964182 -
Flags: review?(jwalden+bmo)
Comment 27•7 years ago
|
||
Comment on attachment 8964183 [details] [diff] [review]
Remove expression closure warning
Review of attachment 8964183 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/js.msg
@@ -242,4 @@
> MSG_DEF(JSMSG_DECLARATION_AFTER_EXPORT,0, JSEXN_SYNTAXERR, "missing declaration after 'export' keyword")
> MSG_DEF(JSMSG_DECLARATION_AFTER_IMPORT,0, JSEXN_SYNTAXERR, "missing declaration after 'import' keyword")
> MSG_DEF(JSMSG_DEPRECATED_DELETE_OPERAND, 0, JSEXN_SYNTAXERR, "applying the 'delete' operator to an unqualified name is deprecated")
> -MSG_DEF(JSMSG_DEPRECATED_EXPR_CLOSURE, 0, JSEXN_WARN, "expression closures are deprecated")
I think we can also remove:
* JSMSG_DEPRECATED_BLOCK_SCOPE_FUN_REDECL
* JSMSG_DEPRECATED_FOR_EACH
Also, JSMSG_DEPRECATED_EXPR_CLOSURE is used here: https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/devtools/server/actors/errordocs.js#56
Not sure what the process is for removing from that list...
Attachment #8964183 -
Flags: review?(jdemooij) → review+
Comment 28•7 years ago
|
||
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d7010443cce
Remove expression closure warning. r=jandem
Assignee | ||
Comment 29•7 years ago
|
||
Comment on attachment 8964182 [details] [diff] [review]
Remove isExprBody
Splitting up the isExpressionClosure part makes sense. I think after that it's pretty clear to see that isExprBody is only used by Reflect.parse though.
Attachment #8964182 -
Attachment is obsolete: true
Assignee | ||
Comment 30•7 years ago
|
||
We know that isExpressionClosure in FullParseHandler always returns false, because isExprBody && !isArrow can't be true anymore without the shorthand function syntax.
Attachment #8964527 -
Flags: review?(jwalden+bmo)
Attachment #8828212 -
Attachment is obsolete: true
Assignee | ||
Comment 31•7 years ago
|
||
I am pretty sure Reflect.parse only ever sees non-lazy functions, so we should be totally fine with removing this flag from everything but the FunctionBox.
Attachment #8964534 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 32•7 years ago
|
||
In the asm.js case we call ParseFunction first, which looks for the "function" token, so it can't be an arrow function.
Comment 33•7 years ago
|
||
Comment on attachment 8964527 [details] [diff] [review]
Remove isExpressionClosure from Parser
Review of attachment 8964527 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/frontend/SyntaxParseHandler.h
@@ +137,4 @@
> };
>
> bool isNonArrowFunctionExpression(Node node) const {
> + return node == NodeFunctionExpressionBlockBody;
We should rename that to just NodeFunctionExpression now (or in some patch here).
Attachment #8964527 -
Flags: review?(jwalden+bmo) → review+
Comment 34•7 years ago
|
||
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64682e402373
Remove isExpressionClosure from Parser. r=Waldo
Comment 35•7 years ago
|
||
Comment on attachment 8964534 [details] [diff] [review]
Remove isExprBody from everything but FunctionBox to keep Reflect.parse working
Review of attachment 8964534 [details] [diff] [review]:
-----------------------------------------------------------------
Huh, so we were XDRing isExprBody in all this stuff for...no actual reason? Weird.
Attachment #8964534 -
Flags: review?(jwalden+bmo) → review+
Comment 36•7 years ago
|
||
bugherder |
Assignee | ||
Comment 37•7 years ago
|
||
(In reply to Jeff Walden [:Waldo] from comment #35)
> Comment on attachment 8964534 [details] [diff] [review]
> Remove isExprBody from everything but FunctionBox to keep Reflect.parse
> working
>
> Review of attachment 8964534 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Huh, so we were XDRing isExprBody in all this stuff for...no actual reason?
> Weird.
Thanks for the review. The only important use of isExprBody was removed in the first patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/dce3a6c9866b#l2.16
Comment 38•7 years ago
|
||
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a703ca86a96
Remove isExprBody from everything but FunctionBox to keep Reflect.parse working. r=Waldo
Keywords: leave-open
Comment 39•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/64682e402373
https://hg.mozilla.org/mozilla-central/rev/8a703ca86a96
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•