Last Comment Bug 738617 - 'let' reserved keyword breaks some websites
: 'let' reserved keyword breaks some websites
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All Linux
: -- normal (vote)
: ---
Assigned To: Jason Orendorff [:jorendorff]
:
: Jason Orendorff [:jorendorff]
Mentors:
https://bvi.bnc.ca/index/bnc/indexfr....
Depends on:
Blocks: 730139
  Show dependency treegraph
 
Reported: 2012-03-23 06:04 PDT by Jean-Marc Valin (:jmspeex)
Modified: 2012-05-06 19:48 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed
+
fixed
verified


Attachments
v1 - backout 6d139ebc0f43 (5.85 KB, patch)
2012-04-17 13:16 PDT, Jason Orendorff [:jorendorff]
jwalden+bmo: review+
akeybl: approval‑mozilla‑aurora+
mark.finkle: approval‑mozilla‑central+
Details | Diff | Splinter Review

Description Jean-Marc Valin (:jmspeex) 2012-03-23 06:04:04 PDT
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] 2012-03-23 06:11:33 PDT
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 2012-03-23 06:15:24 PDT
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 2012-03-23 06:22:51 PDT
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] 2012-03-23 06:29:46 PDT
Regressed from bug 730139.
Comment 5 Anthony Ricaud (:rik) 2012-03-23 07:17:12 PDT
Why is that an Evangelism bug? It looks like a Javascript engine bug.
Comment 6 Chris Lawson (gone) 2012-03-23 07:40:26 PDT
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) 2012-03-23 07:48:33 PDT
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) 2012-03-28 06:53:23 PDT
Moving this to Javascript engine to see if it's a JS engine bug or requires evangelism.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-03-28 14:24:44 PDT
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 2012-03-28 14:46:27 PDT
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] 2012-03-28 16:07:05 PDT
(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] 2012-03-28 16:09:25 PDT
(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 (:jmspeex) 2012-03-28 16:14:03 PDT
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] 2012-03-28 16:24:10 PDT
(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] (remove +bmo to email) 2012-03-28 16:37:13 PDT
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] 2012-03-28 16:45:49 PDT
(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) 2012-04-13 01:34:34 PDT
WebKit has un-reserved let for exactly this case:
http://trac.webkit.org/changeset/113352
Comment 18 Jason Orendorff [:jorendorff] 2012-04-17 13:16:12 PDT
Created attachment 615841 [details] [diff] [review]
v1 - backout 6d139ebc0f43

https://hg.mozilla.org/mozilla-central/raw-rev/6d139ebc0f43
Comment 19 Jason Orendorff [:jorendorff] 2012-04-18 13:16:00 PDT
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] 2012-04-19 11:32:55 PDT
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] 2012-04-19 15:12:10 PDT
Sorry, the "looks good" link is this one.
  https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d8a908424870
Comment 22 :Ehsan Akhgari 2012-04-20 10:29:21 PDT
Backout merged:

https://hg.mozilla.org/mozilla-central/rev/d8a908424870
Comment 23 Alex Keybl [:akeybl] 2012-04-23 14:55:23 PDT
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] 2012-04-26 11:44:23 PDT
This landed before the merge, so it's on Aurora.
Comment 25 David Mandelin [:dmandelin] 2012-04-26 11:45:48 PDT
http://hg.mozilla.org/releases/mozilla-beta/rev/d4aa73deb585
Comment 26 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-26 14:09:51 PDT
Jean-Marc or Thomas, can you please verify this is fixed in Firefox 13.0b1, 14.0a2, and 15.0a1?
Comment 27 Thomas Ahlblom 2012-04-26 14:28:07 PDT
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-26 14:30:46 PDT
(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 2012-04-26 14:43:46 PDT
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) 2012-04-26 21:14:37 PDT
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 2012-05-06 19:48:23 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.