Closed
Bug 1083458
Opened 9 years ago
Closed 5 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: evilpie)
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•9 years ago
|
Component: General → JavaScript Engine
Comment 1•9 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•9 years ago
|
Keywords: site-compat
Comment 2•8 years ago
|
||
Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/expression-closure-support-will-be-removed/
Comment 3•7 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•7 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•7 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•7 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•7 years ago
|
||
Or `getHelperForLanguage: () => null`
Comment 8•7 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•7 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•6 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•6 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•6 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•6 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•6 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•5 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•5 years ago
|
Keywords: dev-doc-needed,
site-compat
Assignee | ||
Comment 17•5 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•5 years ago
|
||
Attachment #8961065 -
Flags: review?(jdemooij)
Comment 19•5 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•5 years ago
|
Attachment #8961065 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 20•5 years ago
|
||
Thanks for the review. I actually forgot about that comment, we are probably clear to land this now.
Assignee | ||
Comment 21•5 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; }.
Assignee | ||
Updated•5 years ago
|
Keywords: leave-open
Comment 22•5 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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dce3a6c9866b https://hg.mozilla.org/mozilla-central/rev/0cac0a8b22db
Assignee | ||
Comment 24•5 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•5 years ago
|
||
Attachment #8964183 -
Flags: review?(jdemooij)
Comment 26•5 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•5 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•5 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•5 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•5 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)
Assignee | ||
Updated•5 years ago
|
Attachment #8828212 -
Attachment is obsolete: true
Assignee | ||
Comment 31•5 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•5 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•5 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•5 years ago
|
||
Pushed by evilpies@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/64682e402373 Remove isExpressionClosure from Parser. r=Waldo
Comment 35•5 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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7d7010443cce
Assignee | ||
Comment 37•5 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•5 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
Assignee | ||
Updated•5 years ago
|
Keywords: leave-open
Comment 39•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/64682e402373 https://hg.mozilla.org/mozilla-central/rev/8a703ca86a96
Status: NEW → RESOLVED
Closed: 5 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
•