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)

defect
Not set
normal

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)

Component: General → JavaScript Engine
Depends on: 1083459
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
Blocks: 1103158
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Keywords: site-compat
Depends on: 1123124
Depends on: 1149015
Depends on: 1151472
Depends on: 1151473
Depends on: 1151474
Depends on: 1151475
Depends on: 1151476
Depends on: 1203742
See Also: → 1207490
See Also: → 1207491
See Also: → 1207494
See Also: → 1207495
See Also: → 1207496
See Also: → 1207497
See Also: → 1207498
See Also: → 1207499
Depends on: 995610
Depends on: 1220845
Depends on: 1313490
Depends on: 1319512
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) :/
Attached file sca.17.1.7.js (obsolete) —
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).
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;}
(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
Or `getHelperForLanguage: () => null`
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).
(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)
See Also: → 1335400
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)
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)
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 |          - |          - |          -
(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.
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 |          - |          -
Depends on: 1426519
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
Blocks: 1440497
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)
Attachment #8961065 - Flags: review?(jdemooij)
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+
Attachment #8961065 - Flags: review?(jdemooij) → review+
Thanks for the review. I actually forgot about that comment, we are probably clear to land this now.
Attached patch Remove isExprBody (obsolete) — Splinter Review
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
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
Attached patch Remove isExprBody (obsolete) — Splinter Review
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)
Attachment #8964183 - Flags: review?(jdemooij)
Blocks: 1450574
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 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+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d7010443cce
Remove expression closure warning. r=jandem
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
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
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)
In the asm.js case we call ParseFunction first, which looks for the "function" token, so it can't be an arrow function.
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+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64682e402373
Remove isExpressionClosure from Parser. r=Waldo
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+
(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
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
https://hg.mozilla.org/mozilla-central/rev/64682e402373
https://hg.mozilla.org/mozilla-central/rev/8a703ca86a96
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.