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

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: miketaylr, Unassigned)

Tracking

({dev-doc-complete, site-compat})

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
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
(Reporter)

Comment 1

3 years ago
Posted image spark.png

Comment 2

3 years ago
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.
(Reporter)

Comment 3

3 years ago
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?
Flags: needinfo?(shu)

Comment 4

3 years ago
(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.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: needinfo?(shu)
Resolution: --- → WONTFIX

Comment 5

3 years ago
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.
Chrome Canary also breaks.
FYIing JST and Dherman
Flags: needinfo?(jst)
Flags: needinfo?(dherman)

Comment 8

3 years ago
(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

3 years ago
> 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.
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.
Flags: needinfo?(dherman)
My plan is to phase out deprecation in 4 cycles, at Firefox 50. Any objections?
(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...
(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.
(Reporter)

Comment 15

3 years ago
Patrick, how long do you think this would take to fix on your end?
Flags: needinfo?(plinskey)

Comment 16

3 years ago
We rolled out a fix to our JS earlier today. I just validated that it's working with Nightly now.
Flags: needinfo?(plinskey)
(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.
In light of Dave's comment 10.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
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.
Attachment #8710797 - Flags: review?(jorendorff) → review+

Comment 21

3 years ago
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
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
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 26

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0e1c61bab502
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(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
Flags: needinfo?(jst) → needinfo?(dherman)
(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.
Flags: needinfo?(dherman)
Please see comment 29.
Flags: needinfo?(kohei.yoshino)
Depends on: 1243793
(Reporter)

Comment 32

3 years ago
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

Updated

3 years ago
Depends on: 1246362
You need to log in before you can comment on or make changes to this bug.