New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

RegExp flag/source getters cause app.jiff.com to fail

Reported by daniel.d...@jiff.com, Jan 27 2016

Issue description

UserAgent: 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.
 
Screen Shot 2016-01-26 at 5.42.01 PM.png
412 KB View Download

Comment 1 by adamk@chromium.org, Jan 27 2016

Cc: adamk@chromium.org
Labels: -Cr-Blink -OS-Mac Cr-Blink-JavaScript OS-All Needs-Bisect
Status: Available
I see the same error, would be great to get a bisect of this.
Cc: brajkumar@chromium.org
Labels: -Needs-Bisect hasBisect M-49
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!

Comment 3 by adamk@chromium.org, Jan 27 2016

Cc: -adamk@chromium.org littledan@chromium.org yangguo@chromium.org
Labels: -Cr-Blink-JavaScript Cr-Blink-JavaScript-Language
Owner: adamk@chromium.org
Status: Assigned
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.

Comment 4 by adamk@chromium.org, Jan 27 2016

Cc: -yangguo@chromium.org adamk@chromium.org
Owner: yangguo@chromium.org
Summary: RegExp flag/source getters cause app.jiff.com to fail (was: "This" keyword inside javascript function somehow is undefined)
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.

Comment 5 by adamk@chromium.org, Jan 27 2016

Labels: -M-49 M-48
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.
Dan has a better grip on how to deal with this. I suspect that this is due to the breaking change wrt RegExp.prototype.

Comment 7 by adamk@chromium.org, Jan 27 2016

Cc: -littledan@chromium.org yangguo@chromium.org
Owner: littledan@chromium.org
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.
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;
        };



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.

Comment 11 by adamk@chromium.org, 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.
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?

Comment 13 by adamk@chromium.org, 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).
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.

Comment 15 Deleted

Comment 16 by ljh...@gmail.com, 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.

Comment 17 Deleted

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
Confirming that upgrading es6-shim fixes the issues

Comment 20 by adamk@chromium.org, Jan 28 2016

Cc: seththompson@chromium.org
+seththompson, who had some thoughts about handling this bug.
Cc: jkummerow@chromium.org danno@chromium.org hablich@chromium.org ishell@chromium.org nyerramilli@chromium.org msrchandra@chromium.org jochen@chromium.org bmeu...@chromium.org
Labels: -hasbisect hasBisect
 Issue 588625  has been merged into this issue.
Labels: Merge-Request-49
To fix the issue, https://codereview.chromium.org/1640803003 should be merged to Chrome 49.
Project Member

Comment 23 by bugdroid1@chromium.org, 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

Labels: -Merge-Request-49 Merge-Approved-49
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].
As per comment #26, this is already merged to M49. If all is done for M49, please remove "Merge-Approved-49" label. Thank you.
Labels: -Merge-Approved-49
Status: Fixed (was: Assigned)
Labels: Merge-Approved-5.0
Labels: -Merge-Approved-5.0
This patch is already present on the 5.0 branch.
Project Member

Comment 32 by bugdroid1@chromium.org, 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

Project Member

Comment 33 by bugdroid1@chromium.org, 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

Project Member

Comment 34 by bugdroid1@chromium.org, 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