RegExp flag/source getters cause app.jiff.com to fail
Reported by
daniel.d...@jiff.com,
Jan 27 2016
|
|||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.82 Safari/537.36 Steps to reproduce the problem: 1. Go to https://app.jiff.com 2. Check the console, see "ORIGINAL ERROR: Cannot set property 'Local' of undefined" 3. If you console.log "this" inside of the function it returns undefined for all occurences What is the expected behavior? "this" inside a function should not be undefined What went wrong? The entire app fails to load (this happens inside hundreds of anonymous functions) Did this work before? Yes Up until Chrome 47 Chrome version: 48.0.2564.82 Channel: stable OS Version: OS X 10.11.2 Flash Version: Shockwave Flash 20.0 r0 This only happened today after upgrading to Chrome 48, has never happened in any other browser.
,
Jan 27 2016
Able to reproduce on Windows 7, Ubuntu 14.04 and Mac OS 10.11.2 using chrome stable M48 - 48.0.2564.82 . Bisect Information: =================== Good build: 48.0.2555.0 Bad Build : 48.0.2557.0 Change Log URL: https://chromium.googlesource.com/chromium/src/+log/304f01a1e1f7ebeb7d637f847469c90ba008b86d..1b869e36f1ef728adaec10ca492fca77c4dee8c1 adamk@ - Unable to find the suspect from the above log, Could you please have a look on it and help us in assigning it to the right owner. Thanks!
,
Jan 27 2016
Thanks for the bisect info, this v8 roll is in the range: https://chromium.googlesource.com/chromium/src/+/94130c97e66a2169ff0dcda5398180e48213a454 which includes https://chromium.googlesource.com/v8/v8/+/2237ba0dba22841e29e2fba877f4f9cd0f23eff7. That patch doesn't relate to an undefined "this", but it does seem related to the first console message, regarding RegExp.prototype. Will dig into this later this morning.
,
Jan 27 2016
If I roll back to just before https://chromium.googlesource.com/v8/v8/+/2237ba0dba22841e29e2fba877f4f9cd0f23eff7 the app works again. Retitling and passing on to Yang/Dan for further triage.
,
Jan 27 2016
One more note: I get a completely different error when I test against tip-of-tree, so to reproduce this I synced my chromium checkout back to 94130c9 and ran gclient sync.
,
Jan 27 2016
Dan has a better grip on how to deal with this. I suspect that this is due to the breaking change wrt RegExp.prototype.
,
Jan 27 2016
,
Jan 27 2016
Here's the offending code:
if (!RegExp.prototype.flags && u) {
var X = function() {
if (!M.TypeIsObject(this))
throw new TypeError("Method called on incompatible type: must be an object.");
var e = "";
return this.global && (e += "g"),
this.ignoreCase && (e += "i"),
this.multiline && (e += "m"),
this.unicode && (e += "u"),
this.sticky && (e += "y"),
e
}
;
d.getter(RegExp.prototype, "flags", X)
}
It's testing whether it needs to polyfill RegExp.prototype.flags (strangely, looks like 'return' has moved from right before 'e' to before the line referencing global) by reading RegExp.prototype.flags. But RegExp.prototype is not a RegExp instance, so the access to the 'global' getter will throw per the ES2015 spec!
An easy workaround for Chrome is to make all the RegExp flag getters return false or undefined, as I proposed on the TC39 GitHub at https://github.com/tc39/ecma262/issues/262 .
I'm not sure exactly which ES2015 polyfill library Jiff is using, but core.js going back a year has done this in an ES2015-compatible way, by reading flags on a RegExp instance, not RegExp.prototype. I'd recommend that website owners upgrade to something like that.
,
Jan 27 2016
Looks like there are two separate issues here. We are still getting the RegExp message, but we were able to work around the 'this' being undefined issue. The workaround will be going out to production tonight at 8:30pm PST.
That portion of our code was being transpiled with an old version of traceur, newer versions of traceur refuse to compile that code with a "‘this’ is not allowed before super" error. Adding super() calls in the ES6 source in the correct places solved the 'this' getting set to undefined issue.
Here's the code (transpiled to ES5) before and after adding the ES6 super(). The BEFORE code worked in versions of code before v48
BEFORE:
var JiffStorage = function JiffStorage(localStorage, databaseStorage) {
this.Local = localStorage;
this.DB = databaseStorage;
};
AFTER
var JiffStorage = function JiffStorage(localStorage, databaseStorage) {
$traceurRuntime.superConstructor(JiffStorage).call(this);
this.Local = localStorage;
this.DB = databaseStorage;
};
,
Jan 27 2016
Nik, where does your RegExp code come from? It doesn't look to me like you actually use RegExp.prototype.flags, so you might want to try just deleting that code, or updating the library to the most recent version.
,
Jan 27 2016
I'm confused by #9. As app.jiff.com is right now (1:30pm PST), reverting the RegExp.prototype chain is sufficient to make the site work again (with no complaints about trying to set "Local" and undefined). So I still think the "this is undefined" bit is a red herring. As #10 suggests, an easy workaround for this might just be to stop testing for RegExp.prototype.flags.
,
Jan 27 2016
Looks like we're using an old version (v0.24.0) of es6-shim (https://github.com/paulmillr/es6-shim/), which could be causing the problem. I'll try upgrading that and see if it fixes it, but I'm confused how this could be the issue. The change I listed above *does* make the app work again (although we still get an 'Uncaught TypeError: RegExp.prototype.global getter called on non-RegExp object' message in the console log). If it's just due to RegExp.prototype.flags why would adding traceur's super() methods cause the app to work again?
,
Jan 27 2016
Re: #12, No idea why the change to call super() also fixes it, but I suspect that it's having some non-local effect: it shouldn't be possible for the value of "this" to be re-bound during the execution of a function, so the call to superConstructor() would still leave "this" undefined (note that there's nothing that keeps "this" from being undefined in JavaScript strict mode, so it's also not clear to me without more knowledge of your app what exactly is going on there).
,
Jan 27 2016
es6shim seems to have been updated only in November: https://github.com/paulmillr/es6-shim/commit/c563abf238df6d08f293cb54903fd4df54d5a885 . I won't be surprised if this isn't the only website use it and to have not updated in the past two and a half months. I'm confident that the old es6shim is what's causing the issue here because this old version of es6shim does a property read which ES2015 specifies should throw, and which Chrome 48 starts throwing for.
,
Jan 27 2016
Absolutely this is the old es6-shim conflicting with the breaking ES6 change - any site that updates es6-shim will be fine, but there definitely might be people using the old issue.
,
Jan 27 2016
OK, thanks for the help guys. Upgrading to the latest es6-shim is breaking something else in our build system right now, so I'll need to investigate that first. We'll put in the other workaround for tonight even though we don't know why it works. Once I get our build system working with the latest versions I'll try again and confirm that it's the old es6-shim
,
Jan 28 2016
Confirming that upgrading es6-shim fixes the issues
,
Jan 28 2016
+seththompson, who had some thoughts about handling this bug.
,
Feb 22 2016
Issue 588625 has been merged into this issue.
,
Feb 23 2016
To fix the issue, https://codereview.chromium.org/1640803003 should be merged to Chrome 49.
,
Feb 23 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/b22b2588740b059b26b6687095514c03dea0feaf commit b22b2588740b059b26b6687095514c03dea0feaf Author: littledan <littledan@chromium.org> Date: Tue Feb 23 01:48:31 2016 ES2015 web compat workaround: RegExp.prototype.flags => "" It turns out that some old polyfill library uses RegExp.prototype.flags as a way of feature testing. It's not clear how widespread this is. For now, as a minimal workaround, we can return undefined from getters like RegExp.prototype.global when the receiver is RegExp.prototype. This patch implements that strategy but omits a UseCounter to make backports easier. R=adamk CC=yangguo@chromium.org BUG= chromium:581577 LOG=Y CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_chromium_rel_ng;tryserver.blink:linux_blink_rel Review URL: https://codereview.chromium.org/1640803003 Cr-Commit-Position: refs/heads/master@{#34201} [modify] https://crrev.com/b22b2588740b059b26b6687095514c03dea0feaf/src/js/harmony-unicode-regexps.js [modify] https://crrev.com/b22b2588740b059b26b6687095514c03dea0feaf/src/js/regexp.js [modify] https://crrev.com/b22b2588740b059b26b6687095514c03dea0feaf/test/mjsunit/es6/regexp-flags.js [add] https://crrev.com/b22b2588740b059b26b6687095514c03dea0feaf/test/mjsunit/regress/regress-crbug-581577.js [modify] https://crrev.com/b22b2588740b059b26b6687095514c03dea0feaf/test/test262/test262.status [modify] https://crrev.com/b22b2588740b059b26b6687095514c03dea0feaf/test/webkit/fast/regex/toString-expected.txt
,
Feb 23 2016
,
Feb 23 2016
Please merge your change to M49 (branch: 2623) before 5:00 PM PST Today [02/23] if you would like to make it to M49 Beta push on Wednesday [02/24].
,
Feb 23 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/6ba0a05cb29255a80ac6713ecb4106ed52dd9c23 commit 6ba0a05cb29255a80ac6713ecb4106ed52dd9c23 Author: Dan Ehrenberg <littledan@chromium.org> Date: Tue Feb 23 20:06:15 2016 Version 4.9.385.25 (cherry-pick) Merged b22b2588740b059b26b6687095514c03dea0feaf ES2015 web compat workaround: RegExp.prototype.flags => "" BUG= chromium:581577 LOG=N R=adamk@chromium.org Review URL: https://codereview.chromium.org/1725963002 . Cr-Commit-Position: refs/branch-heads/4.9@{#31} Cr-Branched-From: 2fea296569597e5064f81fd8fce58f1848de261a-refs/heads/4.9.385@{#1} Cr-Branched-From: 0c1430ac2b65847559d6a09f883ee7e5a91063c9-refs/heads/master@{#33306} [modify] https://crrev.com/6ba0a05cb29255a80ac6713ecb4106ed52dd9c23/include/v8-version.h [modify] https://crrev.com/6ba0a05cb29255a80ac6713ecb4106ed52dd9c23/src/js/harmony-unicode-regexps.js [modify] https://crrev.com/6ba0a05cb29255a80ac6713ecb4106ed52dd9c23/src/js/regexp.js [modify] https://crrev.com/6ba0a05cb29255a80ac6713ecb4106ed52dd9c23/test/mjsunit/es6/regexp-flags.js [modify] https://crrev.com/6ba0a05cb29255a80ac6713ecb4106ed52dd9c23/test/test262/test262.status [modify] https://crrev.com/6ba0a05cb29255a80ac6713ecb4106ed52dd9c23/test/webkit/fast/regex/toString-expected.txt
,
Feb 24 2016
As per comment #26, this is already merged to M49. If all is done for M49, please remove "Merge-Approved-49" label. Thank you.
,
Feb 24 2016
,
Feb 24 2016
,
Mar 4 2016
,
Mar 4 2016
This patch is already present on the 5.0 branch.
,
Mar 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/4ea11ca941fc074fcbacace405ac684d20e34f61 commit 4ea11ca941fc074fcbacace405ac684d20e34f61 Author: littledan <littledan@chromium.org> Date: Tue Mar 08 19:13:55 2016 Add UseCounters for various RegExp compatibility issues We have compatibility workarounds to return 'undefined' on accessors to RegExp.prototype. This patch adds two UseCounters for two categories of this non-spec-compliant path: - source - ignorecase, multiline, global R=yangguo BUG= chromium:581577 LOG=Y Review URL: https://codereview.chromium.org/1762423002 Cr-Commit-Position: refs/heads/master@{#34597} [modify] https://crrev.com/4ea11ca941fc074fcbacace405ac684d20e34f61/include/v8.h [modify] https://crrev.com/4ea11ca941fc074fcbacace405ac684d20e34f61/src/js/macros.py [modify] https://crrev.com/4ea11ca941fc074fcbacace405ac684d20e34f61/src/js/regexp.js
,
Mar 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/80803aa89e31839b8f73959776fa7e1923c6b461 commit 80803aa89e31839b8f73959776fa7e1923c6b461 Author: littledan <littledan@chromium.org> Date: Mon Mar 28 20:42:38 2016 Remove RegExp.prototype.source getter compat workaround The getter RegExp.prototype.source is specified in ES2015 to throw when called on a non-RegExp instance, such as RegExp.prototype. We had previously put in a compatibility workaround for all RegExp getters to make them throw on access specifically with RegExp.prototype as the receiver; however, we only have evidence that this is needed for properties other than source. This patch removes the compatibility workaround for get RegExp.prototype.source and gives it semantics precisely as per the ES2015 specification. R=adamk BUG= chromium:581577 , v8:4827 LOG=Y Review URL: https://codereview.chromium.org/1837843002 Cr-Commit-Position: refs/heads/master@{#35086} [modify] https://crrev.com/80803aa89e31839b8f73959776fa7e1923c6b461/src/js/regexp.js [modify] https://crrev.com/80803aa89e31839b8f73959776fa7e1923c6b461/test/mjsunit/es6/regexp-flags.js [modify] https://crrev.com/80803aa89e31839b8f73959776fa7e1923c6b461/test/test262/test262.status [modify] https://crrev.com/80803aa89e31839b8f73959776fa7e1923c6b461/test/webkit/fast/regex/toString-expected.txt
,
Apr 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/31e806ebd1a33f6c9207f6d3aed7bbdf3bb72cf7 commit 31e806ebd1a33f6c9207f6d3aed7bbdf3bb72cf7 Author: littledan <littledan@chromium.org> Date: Fri Apr 01 00:36:53 2016 Revert of Remove RegExp.prototype.source getter compat workaround (patchset #2 id:20001 of https://codereview.chromium.org/1837843002/ ) Reason for revert: TC39 decided that this compatibility fix should be standardized. Original issue's description: > Remove RegExp.prototype.source getter compat workaround > > The getter RegExp.prototype.source is specified in ES2015 to throw when > called on a non-RegExp instance, such as RegExp.prototype. We had previously > put in a compatibility workaround for all RegExp getters to make them > throw on access specifically with RegExp.prototype as the receiver; however, > we only have evidence that this is needed for properties other than source. > This patch removes the compatibility workaround for get RegExp.prototype.source > and gives it semantics precisely as per the ES2015 specification. > > R=adamk > BUG= chromium:581577 , v8:4827 > LOG=Y > > Committed: https://crrev.com/80803aa89e31839b8f73959776fa7e1923c6b461 > Cr-Commit-Position: refs/heads/master@{#35086} R=adamk@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= chromium:581577 , v8:4827 LOG=Y Review URL: https://codereview.chromium.org/1847783003 Cr-Commit-Position: refs/heads/master@{#35180} [modify] https://crrev.com/31e806ebd1a33f6c9207f6d3aed7bbdf3bb72cf7/src/js/regexp.js [modify] https://crrev.com/31e806ebd1a33f6c9207f6d3aed7bbdf3bb72cf7/test/mjsunit/es6/regexp-flags.js [modify] https://crrev.com/31e806ebd1a33f6c9207f6d3aed7bbdf3bb72cf7/test/test262/test262.status [modify] https://crrev.com/31e806ebd1a33f6c9207f6d3aed7bbdf3bb72cf7/test/webkit/fast/regex/toString-expected.txt |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by adamk@chromium.org
, Jan 27 2016Labels: -Cr-Blink -OS-Mac Cr-Blink-JavaScript OS-All Needs-Bisect
Status: Available