738617 - 'let' reserved keyword breaks some websites

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jean-Marc Valin]

My bank's login page located at https://bvi.bnc.ca/index/bnc/indexfr.html does not load properly (e.g. no text field for the login/password). The problem happens with FF13, but not with FF12. This is a bit similar to another bug I filed for the same website (Bug 738614), but that one was happening with FF12 too.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Looks like there's some erroneous JavaScript code on this web page that causes everything else to fail:

Error: let is a reserved identifier
Source Code:
    var let=0;

Comment 2

[Thomas Ahlblom]

WFM:
Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.27) Gecko/20120216 Firefox/3.6.27
Mozilla/5.0 (X11; Linux x86_64; rv:10.0.3) Gecko/20100101 Firefox/10.0.3
Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20100101 Firefox/12.0

Reproduced:
Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120319 Firefox/13.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120322 Firefox/14.0a1

Comment 3

[Thomas Ahlblom]

Last good nightly: 2012-03-09
First bad nightly: 2012-03-10

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=08809a43e082&tochange=3fdc1c14a8ce

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Regressed from bug 730139.

Comment 5

[Anthony Ricaud (:rik)]

Why is that an Evangelism bug? It looks like a Javascript engine bug.

Comment 6

[Chris Lawson (gone)]

Comment 1 agrees with https://developer.mozilla.org/en/JavaScript/Reference/Reserved_Words , which indicates the site should not be using "let" as a variable name. A few other Google results also agree that "let" is verboten. (And let's be honest here -- "let" is a terrible variable name whether it's allowed or not.)

Comment 7

[Anthony Ricaud (:rik)]

The document you link to says that let is a reserved word in strict mode. This code does not run in strict mode.

Comment 8

[Anthony Ricaud (:rik)]

Moving this to Javascript engine to see if it's a JS engine bug or requires evangelism.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Please, request tracking on regression bugs!

Drivers, this is fallout from an experiment to see whether "let" can be a reserved word....

Comment 10

[Thomas Ahlblom]

What is the expected outcome of this experiment? To prove that not a single of the 3 billion webpages worldwide uses "let" as a variable name, despite it's allowed according the the existing standards?

Comment 11

[David Mandelin [:dmandelin]]

(In reply to Thomas Ahlblom from comment #10)
> What is the expected outcome of this experiment? To prove that not a single
> of the 3 billion webpages worldwide uses "let" as a variable name, despite
> it's allowed according the the existing standards?

It's just to find out how much breakage the existing web would take if the change were made, and to find out which sites break so we can tell them that JS is going to make 'let' reserved word someday.

Comment 12

[David Mandelin [:dmandelin]]

(In reply to Anthony Ricaud (:rik) from comment #8)
> Moving this to Javascript engine to see if it's a JS engine bug or requires
> evangelism.

We should consider it both. Can someone from TE reach out to the site to tell them about the coming change and ask them to prepare their code for it? (Should we file a new bug for that?)

Meanwhile, if sites are breaking and don't update in time, we should turn off the change on the beta branch.

Comment 13

[Jean-Marc Valin]

Can this be an opt-in feature? I do my part by running Aurora and help finding out any unknown bugs. However, I don't really appreciate being voluntarily hit with known bugs for the purpose of measuring website conformance. If you want to make this an opt-in feature, fine. But putting this in Aurora without warning people will just lead to fewer testers.

Comment 14

[David Mandelin [:dmandelin]]

(In reply to Jean-Marc Valin from comment #13)
> Can this be an opt-in feature? I do my part by running Aurora and help
> finding out any unknown bugs. However, I don't really appreciate being
> voluntarily hit with known bugs for the purpose of measuring website
> conformance. If you want to make this an opt-in feature, fine. But putting
> this in Aurora without warning people will just lead to fewer testers.

Point taken. Alex, what do you think about this sort of thing on Aurora? Note that we will almost certainly want to ship ES6 things on Aurora first and they will likely cause some amount of breakage.

Comment 15

[Jeff Walden [:Waldo]]

Adding a little context for Alex:

(In reply to David Mandelin from comment #14)
> Note that we will almost certainly want to ship ES6 things on Aurora first
> and they will likely cause some amount of breakage.

"likely", but only, I think "from time to time".  In this case I'd have bet on some number of sites breaking, but I wouldn't have guessed how few or many, other than probably non-zero.  Other cases will be quite different in their effects.  I'd say it's impossible to judge how much or little as a general rule, only on a case-by-case basis.

Comment 16

[Alex Keybl [:akeybl]]

(In reply to David Mandelin from comment #14)
> Point taken. Alex, what do you think about this sort of thing on Aurora?
> Note that we will almost certainly want to ship ES6 things on Aurora first
> and they will likely cause some amount of breakage.

I think we should back this out of Aurora until we have a clear path forward on how to perform this experiment and perform the associated TE.

A couple of questions:

* Is it a requirement of ES6 to make 'let' a reserved word?
* In the final spec, will the user get an error when 'let' is used as, say, a variable name?
* Couldn't we get a measurement of the overall effect using Telemetry before beginning a TE plan so that we know it's definitely a surmountable goal?

Comment 17

[Anthony Ricaud (:rik)]

WebKit has un-reserved let for exactly this case:
http://trac.webkit.org/changeset/113352

Comment 18

[Jason Orendorff [:jorendorff]]

Attachment: v1 - backout 6d139ebc0f43

https://hg.mozilla.org/mozilla-central/raw-rev/6d139ebc0f43

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 615841 [details] [diff] [review]
v1 - backout 6d139ebc0f43

I want to push this to mozilla-inbound. It's ready to go.

Very low risk. The initial patch went in with a minimum of fuss, and this is a straightforward backout of that patch.

I want to push this to aurora too. Should I wait for a green cycle on m-i before asking for approval-mozilla-aurora, or ask now?

Comment 20

[Jason Orendorff [:jorendorff]]

Comment on attachment 615841 [details] [diff] [review]
v1 - backout 6d139ebc0f43

[Approval Request Comment]
Regression caused by (bug #): bug 730139

User impact if declined:
  Some web sites that worked fine before stop working. JS script errors.
  See comment 0.

Testing completed (on m-c, etc.):
  Landed on m-i, looks good.
  https://hg.mozilla.org/integration/mozilla-inbound/rev/d8a908424870
  I also ran the jstests locally, everything's great.

Risk to taking this patch (and alternatives if risky):
  Very low. The initial patch went in with a minimum of fuss, and
  this is a straightforward backout of that patch.

String changes made by this patch: None.

Comment 21

[Jason Orendorff [:jorendorff]]

Sorry, the "looks good" link is this one.
  https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d8a908424870

Comment 22

[(no longer active)]

Backout merged:

https://hg.mozilla.org/mozilla-central/rev/d8a908424870

Comment 23

[Alex Keybl [:akeybl]]

Comment on attachment 615841 [details] [diff] [review]
v1 - backout 6d139ebc0f43

[Triage Comment]
Please land on Aurora 13 asap, or Beta 13 post-merge.

Comment 24

[David Mandelin [:dmandelin]]

This landed before the merge, so it's on Aurora.

Comment 25

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/releases/mozilla-beta/rev/d4aa73deb585

Comment 26

[u279076]

Jean-Marc or Thomas, can you please verify this is fixed in Firefox 13.0b1, 14.0a2, and 15.0a1?

Comment 27

[Thomas Ahlblom]

https://bvi.bnc.ca/index/bnc/indexfr.html

Not broken:
Mozilla/5.0 (X11; Linux x86_64; rv:10.0.4) Gecko/20100101 Firefox/10.0.4
Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120426 Firefox/15.0a1

Broken:
Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120424 Firefox/13.0a2

Comment 28

[u279076]

(In reply to Thomas Ahlblom from comment #27)
> Broken:
> Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20100101 Firefox/13.0
> Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120424 Firefox/13.0a2

Actually, given by comment 25, this won't be fixed until 13.0b2 since 13.0b1 was built a day earlier. Would you mind verifying when we get 13.0b2 next week?

Comment 29

[Thomas Ahlblom]

OK, I'll check again next week.

BTW, I'm trying to understand what's expected from this bug at the moment. Is the intention to get everything backed out and make all versions of Firefox handle the 'let' keyword as if this bug had never exited?

Comment 30

[Phil Ringnalda (:philor)]

Pushed https://hg.mozilla.org/releases/mozilla-beta/rev/dd7abc694b14 as a bustage fix - remember way way back, several weeks ago, when all your tests were listed in a multitude of jstests.list files? If you backport a patch down to 13 or lower that removes a test, you have to remove it from those files to not set the tree aglow.

Comment 31

[Thomas Ahlblom]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #28)
> (In reply to Thomas Ahlblom from comment #27)
> > Broken:
> > Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20100101 Firefox/13.0
> > Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20120424 Firefox/13.0a2
> 
> Actually, given by comment 25, this won't be fixed until 13.0b2 since 13.0b1
> was built a day earlier. Would you mind verifying when we get 13.0b2 next
> week?

Not broken:
Mozilla/5.0 (X11; Linux x86_64; rv:10.0.4) Gecko/20100101 Firefox/10.0.4
Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20100101 Firefox/12.0
Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120506 Firefox/14.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/15.0 Firefox/15.0a1