1235590 - idbroker.webex.com sign up form and log-in broken

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Taylor [:miketaylr]]

If you try to set up a new account on Cisco Spark, once you get an email to verify your account the page you're taken to (idbroker.webex.com) is broken. See screenshot. Apparently if you already have an account, you can't login eitehr (according to blassey).

STR:

1) sign up for account @ http://web.ciscospark.com/
2) find confirmation email in spam folder (probably)
3) click link

Expected: can fill out form
Actual: busted/missing form

JS errors in the console:

SyntaxError: redeclaration of let getClassName component_0d5529934138ade385df2533f03225d8.js:68:184538
ReferenceError: $ is not defined
 auth_163eea07432061f10cdfbe626e593486.js:641:1
ReferenceError: $ is not defined
 setPassword:33:3
ReferenceError: $ is not defined
 setPassword:214:2

Here's a link to the troublesome JS: view-source:https://idbroker.webex.com/idb/wwf/js/component_0d5529934138ade385df2533f03225d8.js (which contains an email down at the bottom for the webex code)

Looks like more fallout from Bug 1071646. :(

Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=dd319db81bb855825d851b344fd2da070f1a7e74&tochange=c7a3d4a1a2f817865caeb0004f918d77c728f91e

Comment 1

[Mike Taylor [:miketaylr]]

Attachment: spark.png

Comment 2

[Patrick Linskey]

Attachment: with-block-problem.html

Here's a minimal reproducer for the bug. The problem is that FF Nightly now raises an error if a function is defined more than once inside a 'with' block.

Comment 3

[Mike Taylor [:miketaylr]]

Shu, given the reduced case in Comment 2 should this bug be spun out into its own bug or is leaving it as a dupe of Bug 1071646 OK?

Comment 4

[Shu-yu Guo [:shu]]

(In reply to Mike Taylor [:miketaylr] from comment #3)
> Shu, given the reduced case in Comment 2 should this bug be spun out into
> its own bug or is leaving it as a dupe of Bug 1071646 OK?

Given comment 2 this is not a bug at all. Functions in ES6 are like 'let' declarations and redeclarations are supposed to be errors.

The bug is on Cisco Spark or whatever product this is to update their code to be ES6-compliant.

Comment 5

[Patrick Linskey]

Shu: agreed that our code has a bug.

However, bear in mind that this works with released ff, chrome, ie, and safari. And we are presumably not the only people who have made this mistake. Absent a strict-mode annotation, this probably should be a warning. 

At a minimum, you should gather merrics on this so you can track the scope of the difference.

Comment 6

[Masatoshi Kimura [:emk]]

Chrome Canary also breaks.

Comment 7

[Eric Rescorla (:ekr)]

FYIing JST and Dherman

Comment 8

[Shu-yu Guo [:shu]]

(In reply to Patrick Linskey from comment #5)
> Shu: agreed that our code has a bug.
> 
> However, bear in mind that this works with released ff, chrome, ie, and
> safari. And we are presumably not the only people who have made this
> mistake. Absent a strict-mode annotation, this probably should be a warning. 
> 

Well, sure, because none of those browser versions fully implement block-scoped functions per ES6.

We can't really make this a warning instead of an error. Leaving block-scoped functions unimplemented breaks some sites (Apple Music), and implementing it breaks other sites (like this one, and previously Google Inbox, which Google fixed). As a vendor we're kind of between a rock and a hard place wrt to this part of the spec. Since ES6 is done and all vendors are working towards full compliance at the moment, the only path forward in my opinion is to eat temporary breakages and urge webdevs to fix incompatible code.

> At a minimum, you should gather merrics on this so you can track the scope
> of the difference.

Is what you're suggesting gather metrics, then try to amend the standard if metrics suggest that the amount of backwards incompatibility is too large? It's too late for that, as ES6 has been set in stone for some time now. Presumably TC39 did the necessary data gathering.

Comment 9

[Patrick Linskey]

> Is what you're suggesting gather metrics, then try to amend the standard

No, I'm thinking of more of a roll-out approach than anything else.

In an ideal world, I'd love it if Mozilla could help to close the loop on these sorts of compatibility issues as the roll-out happens, so that site authors can get real-world metrics ahead of any breakage. For example, when rolling out a breaking change:

1. Release a version that triggers a console warning instead of an error, and calls home (if allowed) with some sort of page identifier [1].

2. If an email address has been securely registered [2] to receive remote console warnings for a given page identifier, send an email (maybe batched) to that address that identifies "one of your pages violated language check 'foo' at least once today".

3. After some time has passed (maybe one release?), flip from console-warning mode to error mode.


Of course, there's a huge privacy question with this sort of third-party disclosure, and I don't know what you're already gathering. But if there were some way to route that sort of compatibility-breakage information from browsers in the wild back to site authors in a minimally-disclosing manner, it'd be a huge help when rolling out these sorts of changes.


[1] This shouldn't be the full URL, of course. Maybe the SHA of the domain, or the SHAs of some number of domain + path-prefix strings. Probably just the SHA of the domain listed in the served-up cert, though, for privacy reasons.

[2] Again, I could imagine using certs here to prove ownership.

Comment 10

[Dave Herman [:dherman]]

First, it's helpful to separate the runtime behavior from the early errors. The former is, I think, pretty well cemented: multiple browsers are shipping the ES6 semantics and at least one non-trivial app (Apple Music) is depending on them. That's all good. So this bug is really only concerned with the latter.

No engine is shipping the early errors in a release channel. The engine that's closest is V8 (Chakra hasn't implemented them, and JSC doesn't appear to have either), but that's still only in Canary and maybe Beta. We don't know how much breakage they'll see in the wild; it's conceivable that even if they do ship them they'll have to roll back for compatibility.

We can't afford this risk right now. The best option for SpiderMonkey is, along the lines of what Patrick suggests in #9, to roll back the early errors (preserving the ES6 scoping and runtime behavior), and start block-local function redeclarations down a somewhat more gradual deprecation process. While some of Patrick's recommendations are more than we can afford to implement, deprecation should be able to at produce a warning in the devtools that links to an article explaining a rough time frame for deprecation (probably a couple of a release cycles) and a clear workaround for developers.

Ultimately, deprecation might fail, so we also want to make sure that before we enable the early error, the semantics we choose for redeclarations is portable and future-proof for the eventual standardization. I spoke with Brian Terlson (from the Chakra team and also editor of ECMA-262), and it appears both V8 and Chakra have the same semantics: the last function wins. In other words, except for *other* early errors that have to be detected, it's as if the earlier functions aren't there:

  {
    console.log(foo()); // 2
    function foo() { return 1 }
    console.log(foo()); // 2
    function foo() { return 2 }
    console.log(foo()); // 2
  }

That should be easy to implement, doesn't violate lexical scoping, and is consistent with hoisted initialization of functions elsewhere in JS precedent.

Comment 11

[Shu-yu Guo [:shu]]

Attachment: Allow redeclaring block-scoped functions and warn about deprecation for now.

This patch is so terrible.

Comment 12

[Shu-yu Guo [:shu]]

My plan is to phase out deprecation in 4 cycles, at Firefox 50. Any objections?

Comment 13

[Eric Rescorla (:ekr)]

(In reply to Shu-yu Guo [:shu] from comment #12)
> My plan is to phase out deprecation in 4 cycles, at Firefox 50. Any
> objections?

It seems like we need to see how the server environment looks...

Comment 14

[Shu-yu Guo [:shu]]

(In reply to Eric Rescorla (:ekr) from comment #13)
> (In reply to Shu-yu Guo [:shu] from comment #12)
> > My plan is to phase out deprecation in 4 cycles, at Firefox 50. Any
> > objections?
> 
> It seems like we need to see how the server environment looks...

And how do we see how the server environment looks? The patch included above has a telemetry hook.

Comment 15

[Mike Taylor [:miketaylr]]

Patrick, how long do you think this would take to fix on your end?

Comment 16

[Patrick Linskey]

We rolled out a fix to our JS earlier today. I just validated that it's working with Nightly now.

Comment 17

[Eric Rescorla (:ekr)]

(In reply to Shu-yu Guo [:shu] from comment #14)
> (In reply to Eric Rescorla (:ekr) from comment #13)
> > (In reply to Shu-yu Guo [:shu] from comment #12)
> > > My plan is to phase out deprecation in 4 cycles, at Firefox 50. Any
> > > objections?
> > 
> > It seems like we need to see how the server environment looks...
> 
> And how do we see how the server environment looks? The patch included above
> has a telemetry hook.

Yes, and my point is that it doesn't make any sense to decide on a cutover
date until we have seen that data.

Comment 18

[Shu-yu Guo [:shu]]

In light of Dave's comment 10.

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 8710797 [details] [diff] [review]
Allow redeclaring block-scoped functions and warn about deprecation for now.

Review of attachment 8710797 [details] [diff] [review]:
-----------------------------------------------------------------

r=me.

::: js/src/frontend/ParseMaps.h
@@ +249,5 @@
>          MOZ_ASSERT(isMultiple());
>          return (Node*) (u.bits & ~0x1);
>      }
>  
> +    Node* lastNode() const {

Doesn't appear to be used in this patch?

::: js/src/frontend/Parser.cpp
@@ +487,5 @@
>          }
>  
>          bool aliased;
>          if (pc->sc->isGlobalContext()) {
> +            // Bindings for global and eval scripts are used solely for redeclarationd

Typo.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f01b5d05a13b

Comment 21

[Adam Klein]

For what it's worth, Chromium ran into this on foxnews.com's video player (which appears to be provided by Akamai): https://code.google.com/p/chromium/issues/detail?id=579395

Comment 22

[Kartikaya Gupta (email:[e-mail])]

This seems to have busted jsreftests. There doesn't seem to be a sheriff around so I'm going to back it out. The evidence:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=728713d8e7c6&filter-searchStr=Linux%20x64%20opt%20Reftest%20JSReftest%20R%28J%29

Comment 23

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c49f59a5b828

Comment 24

[Shu-yu Guo [:shu]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> This seems to have busted jsreftests. There doesn't seem to be a sheriff
> around so I'm going to back it out. The evidence:
> 
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&fromchange=728713d8e7c6&filter-
> searchStr=Linux%20x64%20opt%20Reftest%20JSReftest%20R%28J%29

Argh, thanks! I always forget you can't use |evaluate| in reftests because it's not in the browser...

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e1c61bab502

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0e1c61bab502

Comment 27

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/redeclaration-of-block-scoped-functions-has-been-deprecated/

Comment 28

[Eric Rescorla (:ekr)]

(In reply to Kohei Yoshino [:kohei] from comment #27)
> Posted the site compatibility doc:
> https://www.fxsitecompat.com/en-CA/docs/2016/redeclaration-of-block-scoped-
> functions-has-been-deprecated/

Why are you claiming this will be removed in 50? That seems like a question that
needs to be resolved by measurement. needinfoing dherman

Comment 29

[Shu-yu Guo [:shu]]

(In reply to Eric Rescorla (:ekr) from comment #28)
> (In reply to Kohei Yoshino [:kohei] from comment #27)
> > Posted the site compatibility doc:
> > https://www.fxsitecompat.com/en-CA/docs/2016/redeclaration-of-block-scoped-
> > functions-has-been-deprecated/
> 
> Why are you claiming this will be removed in 50? That seems like a question
> that
> needs to be resolved by measurement. needinfoing dherman

Probably because he saw the comments in my patch that say backout when version >= 50. You are right though, this'll need to be resolved by telemetry from us and hopefully other vendors.

Kohei, please remove mention of removal by 50. Clearing NI for dherman.

Comment 30

[Shu-yu Guo [:shu]]

Please see comment 29.

Comment 31

[Kohei Yoshino]

Thanks, updated the document. https://github.com/fxsitecompat/www.fxsitecompat.com/commit/66f3523

Comment 32

[Mike Taylor [:miketaylr]]

Chrome also added a use counter for this ("Allowing duplicate sloppy-mode block-scoped function declarations in the exact same scope)" just a few days ago: https://code.google.com/p/chromium/issues/detail?id=579395#c29