881782 - DCU Bank fails to display any accounts on "Accounts" page, in Nightly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Daniel Holbert [:dholbert]]

STR:
 0. Have a bank account with DCU
 1. Log in at https://www.dcu.org/

EXPECTED RESULTS: Post-login page ("Accounts" view) should show me a list of my accounts

ACTUAL RESULTS: My accounts aren't shown. This message appears in the error console and in my terminal (in a debug build):
{
JavaScript error: https://www.dcu-online.org/onlineserv/HB/sdp/js/ext-compressed.js?012313, line 1: historyEnabled is not defined
}

This is a regression, and mozregression + hg bisect says this started with:
{
changeset:   134272:5d35dc039af7
user:        Sankha Narayan Guria <[e-mail]>
date:        Wed Jun 05 14:17:35 2013 -0500
summary:     Bug 875433 - Array.prototype.iterator is the same function object as .values. r=jorendorff.
}
http://hg.mozilla.org/mozilla-central/rev/5d35dc039af7

Comment 1

[Rick Waldron [:rwaldron]]

This is interesting, there is no |historyEnabled| in that file and the only suspicious code I can see is a piece that defines Array.prototype.indexOf if it doesn't exist. 

Are there other scripts in the page you can link to?

Comment 2

[Daniel Holbert [:dholbert]]

I'll see if I can find |historyEnabled| in another script.

Also: I don't claim to understand what 5d35dc039af7 was doing, but it's specifically this chunk that's responsible for the breakage (i.e. if I comment out these new lines, the site works again):
+    JSFunction *fun = JS_DefineFunction(cx, arrayProto, "values", JS_ArrayIterator, 0, 0);
+    if (!fun)
+        return NULL;
+
+    RootedValue funval(cx, ObjectValue(*fun));
+    if (!JS_DefineProperty(cx, arrayProto, "iterator", funval, NULL, NULL, 0))
+        return NULL;

So -- it's not caused by this, not the other code-change (the JS_FN() line-removal).

I also tried *only* commenting out the second half of the code quoted above (starting with "RootedValue"), to see if that was the problematic chunk, but the site is still broken. I have to comment out that whole chunk to fix things.

Comment 3

[Till Schneidereit [:till]]

Can you try commenting out the second half *and* changing the first line of that chunk to this?

JSFunction *fun = JS_DefineFunction(cx, arrayProto, "iterator", JS_ArrayIterator, 0, 0);

If that works, try changing "values" to something else ("welcometozombocom", for example) and *not* commenting out the second half.

If *that* works, it might be an evangelism issue, as they might do very weird things causing them to fail if arrays have a function `values`.

Comment 4

[Daniel Holbert [:dholbert]]

Replying to comment 1: I found two scripts that get loaded which mention historyEnabled:
  https://www.dcu-online.org/onlineserv/HB/sdp/js/common-compressed.js
  https://www.dcu-online.org/onlineserv/HB/sdp/js/sdp-compressed.js

I'll try Comment 3 now.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Till Schneidereit [:till] from comment #3)
> Can you try commenting out the second half *and* changing the first line of
> that chunk to this?
> 
> JSFunction *fun = JS_DefineFunction(cx, arrayProto, "iterator",
> JS_ArrayIterator, 0, 0);

That works.

> If that works, try changing "values" to something else ("welcometozombocom",
> for example) and *not* commenting out the second half.

That works too.

> If *that* works, it might be an evangelism issue, as they might do very
> weird things causing them to fail if arrays have a function `values`.

Hmm, OK.

Comment 6

[Till Schneidereit [:till]]

Yeah, sounds like an issue on their side. Not sure how we should deal with this one, though: just saying "they have to fix it" won't give you your list of accounts back.

Comment 7

[Daniel Holbert [:dholbert]]

Yeah... So, how backwards-compatible (and/or important & known-correct) was the fix for bug 875433 supposed to be?  Given that I hit a site that was rendered unusable by it in less than a week, I slightly worry that it might not be web-compatible (and we might break a good many other sites with this when it hits aurora/beta/release).

Comment 8

[Till Schneidereit [:till]]

So maybe we should back out bug 875433 for now, try to get some more data on how much of an issue this'll be, and do some evangelism before relanding? We could theoretically also wait for other browsers to land this first, but I guess playing games of "who blinks first" isn't that helpful in the long run.

@dherman, do you think that we should be conservative here and back out for now, or try our luck with aggressive evangelism?

Comment 9

[Jeff Walden [:Waldo]]

Has anyone actually stepped through code to figure out exactly what the site's doing that causes things to go wrong?  I wouldn't be surprised to find there's a bug in the underlying change, that this site is tripping over.  Identity of individual member functions on Array.prototype is a very nitpicky thing to be depending on; I doubt the site is doing exactly that, but much more likely something less weird that could be worked around.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> Has anyone actually stepped through code to figure out exactly what the
> site's doing that causes things to go wrong?

I don't think so, no.

AFAIK, I'm the only one on this bug with a DCU bank account, and I don't know enough about the JS engine to usefully step through its code. (nor am I comfortable giving out my login credentials)

I'd be open to letting you (or another JS hacker) step through the code alongside me on my machine, though.

Comment 11

[Till Schneidereit [:till]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> Identity of individual member functions on Array.prototype is a very
> nitpicky thing to be depending on; I doubt the site is doing exactly that,
> but much more likely something less weird that could be worked around.

That's a fair point, and we should definitely check if at all possible.

> Identity of individual member functions on Array.prototype is a very
> nitpicky thing to be depending on; I doubt the site is doing exactly that,
> but much more likely something less weird that could be worked around.

So my guess would be that they aren't actually depend on that at all. See comment 2 for an experiment that gets rid of one of the two member functions and, hence, the identity issue, altogether. Rather, the sheer existence of the `values` member is probably throwing them off. Maybe they create a field of that name under certain circumstances and use its existense to (not) trigger something somewhere else? Very theoretically, they could, of course, feature-detect iterator existence by checking for Array.values and then rely on identity of that with Array.iterator in strange ways. Or on them *not* being identical.

@Daniel, maybe you could try yet another experiment to check these assumptions? That'd be changing the second half of that chunk to

JSFunction *fun = JS_DefineFunction(cx, arrayProto, "iterator", JS_ArrayIterator, 0, 0);

That'd make both of these members available, but not identical. If that (surprisingly) works, the identity thing is really the underlying issue. Of course, they could still really rely on the identity and we could have a bug in that, somewhere. My money says that's not the case, though.

Comment 12

[Till Schneidereit [:till]]

So, I just discovered this:
https://mail.mozilla.org/pipermail/es-discuss/2013-June/031390.html

Comment 13

[Daniel Holbert [:dholbert]]

First of all: bug 875433 (which regressed this) was backed out during the past day, so this no longer reproduces on mozilla-central trunk.

To reply to the experiment in comment 11, I updated to the parent of the backout cset, so that I'd be using a build affected by the bug.

tl'dr: The experiment from comment 11 didn't fix things.

Firstly: the suggested code...
> JSFunction *fun = JS_DefineFunction(cx, arrayProto, "iterator", JS_ArrayIterator, 0, 0);
...doesn't compile, because a variable "fun" has already been declared (in the first half of the chunk).

So, just to be explicit, I replaced this code (the last part of the chunk from comment 2):
    if (!JS_DefineProperty(cx, arrayProto, "iterator", funval, NULL, NULL, 0))
        return NULL;
with this code:
    fun = JS_DefineFunction(cx, arrayProto, "iterator", JS_ArrayIterator, 0, 0);
    if (!fun)
        return NULL;

Unfortunately, the bug still reproduced.

Comment 14

[Till Schneidereit [:till]]

Daniel, thanks for doing that experiment (and sorry for bungling up the steps involved).

The result shouldn't come as a surprise to anyone, by now, but it's nice to have definite confirmation, nevertheless.

Comment 15

[Jason Orendorff [:jorendorff]]

This shouldn't still be open. No longer reproduces because we fixed it => RESO FIXED.

Direct further discussion to bug 875433 which is REOPENED.

Comment 16

[Daniel Holbert [:dholbert]]

Verified fixed in most recent nightly:
 Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20130620 Firefox/24.0

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Jason Orendorff [:jorendorff] from comment #15)
> This shouldn't still be open. No longer reproduces because we fixed it =>
> RESO FIXED.

To be clear: s/fixed it/backed out the regressing changeset

And now it looks like the regressing changeset has re-landed on inbound (bug 875433 comment 11), so this will become an issue again in nightlies in a day or two, I expect.

Comment 18

[Daniel Holbert [:dholbert]]

[needinfo=me to reopen this bug (assuming I can repro) when that re-landed cset makes it to nightlies]

Comment 19

[Daniel Holbert [:dholbert]]

Per comment 17, I can reproduce this again (and can no longer use my bank's web site) in nightlies, now that bug 875433 has landed again.

Reopening.
Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20130705 Firefox/25.0

Comment 20

[Daniel Holbert [:dholbert]]

This is WORKSFORME in today's nightly [which the about window reports as "26.0a1 (2013-08-06)"], presumably because bug 875433 was backed out again (in bug 875433 comment 16).

Comment 21

[Daniel Holbert [:dholbert]]

which I guess means this is once again FIXED by backout.

(I verified that it's still broken in the old build from comment 19, BTW, so the website & its version of the Sencha library are not yet fixed)

Comment 22

[Till Schneidereit [:till]]

(In reply to Daniel Holbert [:dholbert] from comment #21)
> (I verified that it's still broken in the old build from comment 19, BTW, so
> the website & its version of the Sencha library are not yet fixed)

We're not going to rely on all Ext.js-using websites updating. At the last TC39 meeting, a solution has found approval that'll let us keep the `values` method, while working around this, and similar issues: a new special symbol, @@unscopeable is introduced, that can be used to install a list of property names on an object. The listed properties are then ignored in `with(){}` blocks.

Details can be found here:
https://github.com/rwldrn/tc39-notes/blob/master/es6/2013-07/july-23.md#43-arrayprototypevalues

Comment 23

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 24b7 since bug 875433 was backed out from 24 and pushed to 25.

Comment 24

[Paul Silaghi, QA [:pauly]]

(In reply to Daniel Holbert [:dholbert] (vacation through 8/31) from comment #20)
> This is WORKSFORME in today's nightly [which the about window reports as
> "26.0a1 (2013-08-06)"], presumably because bug 875433 was backed out again
> (in bug 875433 comment 16).
Is this possible if the backout was made only on mozilla-inbound ?

Comment 25

[Daniel Holbert [:dholbert]]

(In reply to Paul Silaghi [QA] from comment #24)
> Is this possible if the backout was made only on mozilla-inbound ?

No, not possible. Everything on inbound gets merged to central every day or so; it should be impossible for anything to be "stranded" on inbound.

(It could hypothetically be possible for something to be landed on inbound and then backed out there before it's merged to central, so that central never gets a nightly build with the fix applied, but I don't think that's what you're asking.

I assume you're asking because there was no central cset posted on bug 875433. That's just a mistake by one of the merge sheriffs; not anything to worry about. As noted in comment 20, this is WORKSFORME *because* that backout made it to central.

Comment 26

[Alex Keybl [:akeybl]]

Jason, can you make sure that this is backed out on all branches? Aurora 25 still appears to have this code.

Comment 27

[Jason Orendorff [:jorendorff]]

Yep.

Comment 17 tells what happened. Basically, I intentionally re-landed the offending patch after the train for FF24 had departed.

I have no idea why I did that. It was stupid. This has got to be commented out for now. Once TC39 makes a decision on @@unscopeable we can implement and try again.

I'll patch soon.

Comment 28

[Jason Orendorff [:jorendorff]]

Oh, then another thing happened. Ms2ger backed it out again in tip, in rev 22c3433bdbc9, which cites bug 892225. Moving over there to get that into FF25.

Setting back to FIXED. It was fixed, once.

Comment 29

[Alex Keybl [:akeybl]]

(In reply to Jason Orendorff [:jorendorff] from comment #28)
> Oh, then another thing happened. Ms2ger backed it out again in tip, in rev
> 22c3433bdbc9, which cites bug 892225. Moving over there to get that into
> FF25.
> 
> Setting back to FIXED. It was fixed, once.

Can we consider this fixed on 25 now? Unclear from bug 892225. If so, please set the status flag accordingly.

Comment 30

[Jason Orendorff [:jorendorff]]

It looks pretty clear to me in bug 892225:

* I noted in bug 892225 comment 28 on 6 Sep that the same correct code was in all branches.
* JoeG confirmed on bug 892225 comment 30 on 16 Sep that it's fixed in FF25.

That was a week ago. No bad news since. Setting the verified bit.