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

Issue 634467 link

Starred by 3 users
Status: Fixed
Owner:
ishell@chromium.org
Closed: Aug 2016
Cc:
mvstan...@chromium.org
hablich@chromium.org
adamk@chromium.org
verwa...@chromium.org
█ rnimmagadda@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Regression



Sign in to add a comment

try/catch breaks subsequent typeof checks

Reported by karoli...@gmail.com, Aug 4 2016 
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.34 Safari/537.36

Steps to reproduce the problem:
1. Evaluate the following code in console (or load script.js):

```
try {
  module.exports = 'foo'
} catch (err) {}

if (typeof module === 'object' && module.exports) {
  module.exports = 'bar'
}

console.log('here')
```

What is the expected behavior?
"here" is logged

What went wrong?
"VM45:4 Uncaught ReferenceError: module is not defined" is thrown.

Did this work before? Yes Chrome 52

Chrome version: 53.0.2785.34  Channel: beta
OS Version: OS X 10.11.5
Flash Version: Shockwave Flash 22.0 r0

This is not specific to module.exports, typeof check of any variable breaks the same way, e.g.:

```
try {
  baz.bak = 'foo'
} catch (err) {}

if (typeof baz === 'object' && baz.bak) {
  baz.bak = 'bar'
}

console.log('here')
```

Removing try/catch makes it work:

```
if (typeof baz === 'object' && baz.bak) {
  baz.bak = 'bar'
}

console.log('here')
```

Comment 1 by tkent@chromium.org, Aug 4 2016

Components: -Blink Blink>JavaScript
Labels: Needs-Bisect

Comment 2 by hablich@chromium.org, Aug 5 2016

Cc: hablich@chromium.org
Components: -Blink>JavaScript Blink>JavaScript>Language

Comment 3 by hablich@chromium.org, Aug 5 2016

Cc: adamk@chromium.org

Comment 4 by tkonch...@chromium.org, Aug 5 2016

Cc: -hablich@chromium.org machenb...@chromium.org vogelheim@chromium.org yangguo@chromium.org
Labels: -Type-Bug -Pri-2 -Needs-Bisect ReleaseBlock-Stable M-53 OS-Linux OS-Windows Pri-1 Type-Bug-Regression
Owner: hablich@chromium.org
Status: Assigned (was: Unconfirmed)
Able to reproduce the issue on mac 10.11.5 chrome version dev 54.0.2816.0 and canary 54.0.2819.0

This is a regression in M53 and below is the info

Manual Bisect Info:
Good Build: 53.0.2778.0
Bad Build: 53.0.2779.0

Bisect Tool Info:
You are probably looking for a change made after 401833 (known good), but no later than 401834 (first known bad).
CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/26b402b376aabbb5873a9c129adf74b60d033fda..a795fd0248b18c3d82db74b93e3fd162b07ce05c

Possible suspect : https://codereview.chromium.org/2088393003

Please reassign if the change is not related to yours

The issue is seen in windows and Linux as well

Comment 5 by hablich@chromium.org, Aug 5 2016

Cc: hablich@chromium.org
Owner: adamk@chromium.org
Adam's changes in the range look relevant. PTAL.

Comment 6 by adamk@chromium.org, Aug 5 2016

Cc: -machenb...@chromium.org -vogelheim@chromium.org -yangguo@chromium.org mvstan...@chromium.org verwa...@chromium.org
Components: -Blink>JavaScript>Language Blink>JavaScript>Runtime
Labels: -OS-Linux -OS-Windows -OS-Mac OS-All
Owner: ishell@chromium.org
I can't reproduce in d8, but I can in chrome and content_shell.

I don't think it's my change, though, but rather ishell's https://codereview.chromium.org/2081143002: it touched global loads and typeof handling.

Comment 7 by ishell@chromium.org, Aug 6 2016 

The issue is that we merge load global IC slots for loads inside typeof and outside typeof. This becomes observable after my CL (https://codereview.chromium.org/2081143002) in case of interceptors when we install a slow stub that suits only one case.

Comment 8 by ishell@chromium.org, Aug 8 2016 

The fix in flight: https://codereview.chromium.org/2219303002/
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 9 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/d634e65fb07fb7c3a79680237c5e90a9fae6645c

commit d634e65fb07fb7c3a79680237c5e90a9fae6645c
Author: ishell <ishell@chromium.org>
Date: Tue Aug 09 16:30:25 2016

[ic] Don't share LoadGlobalIC slots inside typeof and outside typeof.

Because in case of interceptors we will install a slow stub that suits only one case.

BUG= chromium:634467 
TBR=verwaest@chromium.org

Review-Url: https://codereview.chromium.org/2219303002
Cr-Commit-Position: refs/heads/master@{#38503}

[modify] https://crrev.com/d634e65fb07fb7c3a79680237c5e90a9fae6645c/src/ast/ast-numbering.cc
[modify] https://crrev.com/d634e65fb07fb7c3a79680237c5e90a9fae6645c/test/cctest/test-api-interceptors.cc
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 9 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/d634e65fb07fb7c3a79680237c5e90a9fae6645c

commit d634e65fb07fb7c3a79680237c5e90a9fae6645c
Author: ishell <ishell@chromium.org>
Date: Tue Aug 09 16:30:25 2016

[ic] Don't share LoadGlobalIC slots inside typeof and outside typeof.

Because in case of interceptors we will install a slow stub that suits only one case.

BUG= chromium:634467 
TBR=verwaest@chromium.org

Review-Url: https://codereview.chromium.org/2219303002
Cr-Commit-Position: refs/heads/master@{#38503}

[modify] https://crrev.com/d634e65fb07fb7c3a79680237c5e90a9fae6645c/src/ast/ast-numbering.cc
[modify] https://crrev.com/d634e65fb07fb7c3a79680237c5e90a9fae6645c/test/cctest/test-api-interceptors.cc

Comment 11 by adamk@chromium.org, Aug 9 2016 

Seems like we probably want to merge this to M53?

Comment 15 by ishell@chromium.org, Aug 10 2016

Labels: Merge-Request-53

Comment 16 by hablich@chromium.org, Aug 11 2016

Labels: -Merge-Request-53 Merge-Approved-53
Is this whole bug fixed too?

Comment 17 by gov...@chromium.org, Aug 11 2016 

Please merge your change to M53 branch 2785 latest by Friday 5:00 PM PT so we can take it in for next week Beta release. Thank you.
Project Member

Comment 18 by bugdroid1@chromium.org, Aug 12 2016

Labels: merge-merged-5.3
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/426f51890efa76d39a498ce61167225d04326d49

commit 426f51890efa76d39a498ce61167225d04326d49
Author: Jakob Kummerow <jkummerow@chromium.org>
Date: Fri Aug 12 12:20:52 2016

Merged: Squashed multiple commits.

Merged: [ic] Don't share LoadGlobalIC slots inside typeof and outside typeof.
Revision: d634e65

Merged: [ic] Merge LoadGlobalIC_Slow builtins for inside typeof and outside typeof cases.
Revision: b558894

BUG= chromium:634467 , chromium:634467 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=verwaest@chromium.org

Review URL: https://codereview.chromium.org/2237983004 .

Cr-Commit-Position: refs/branch-heads/5.3@{#42}
Cr-Branched-From: 820a23aade5e74a92d794e05a0c2b3597f0da4b5-refs/heads/5.3.332@{#2}
Cr-Branched-From: 37538cb2c1b4d75c41af386cb4fedbe5566f5608-refs/heads/master@{#37308}

[modify] https://crrev.com/426f51890efa76d39a498ce61167225d04326d49/src/builtins.cc
[modify] https://crrev.com/426f51890efa76d39a498ce61167225d04326d49/src/builtins.h
[modify] https://crrev.com/426f51890efa76d39a498ce61167225d04326d49/src/ic/ic.cc
[modify] https://crrev.com/426f51890efa76d39a498ce61167225d04326d49/src/ic/ic.h
[modify] https://crrev.com/426f51890efa76d39a498ce61167225d04326d49/src/runtime/runtime-object.cc
[modify] https://crrev.com/426f51890efa76d39a498ce61167225d04326d49/src/runtime/runtime.h
[modify] https://crrev.com/426f51890efa76d39a498ce61167225d04326d49/test/cctest/test-api-interceptors.cc

Comment 19 by gov...@chromium.org, Aug 12 2016

Labels: -Merge-Approved-53
Per comment #18, this is already merged to M53. So removing "Merge-Approved-53" label.

Comment 20 by gov...@chromium.org, Aug 15 2016 

M53 Stable launch is coming VERY soon.Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix asap so it gets chance to bake in beta before stable promotion later this month. Thank you.

Comment 21 by adamk@chromium.org, Aug 15 2016

Status: Fixed (was: Assigned)

Comment 22 by hablich@chromium.org, Aug 16 2016

Labels: Merge-Merged-53

Comment 23 by rnimmagadda@chromium.org, Aug 22 2016

Cc: rnimmagadda@chromium.org
Labels: TE-Verified-M54 TE-Verified-54.0.2835.0
Verified the fix on Windows 7, MAC (10.11.6) & Ubuntu Trusty (14.04) for Google Chrome Version - 54.0.2835.0 

Screen-recording is attached.

TE-Verified labels are added.
634467.mp4
1.0 MB View Download
►

Sign in to add a comment