Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 673008 hasOwnProperty returning inconsistent results
Starred by 16 users Reported by canada.r...@gmail.com, Dec 9 Back to list
Status: Fixed
Owner:
Closed: Dec 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Regression



Sign in to add a comment
UserAgent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0

Steps to reproduce the problem:
1. Create an object with two keys.  One is the string "33", the other is the string "-10"
2. Run o.hasOwnProperty with the negative number and the string representation of it.  This will return true for the string, false for the integer.
3. Create a second object with two keys.  Use "32" and "-10".
4. Run o.hasOwnProperty with the negative number and the string representation of it.  This will return true for both the string and integer.

The order doesn't seem to matter.  The size of the negative number does not seem to matter.  Both keys have to be integers.

What is the expected behavior?
The result should be the same regardless of other keys

What went wrong?
Detecting whether a negative number is a key does not work if there is a positive number as a key.

Did this work before? Yes 54

Chrome version: 56.0.2924.18  Channel: dev
OS Version: 6.1 (Windows 7, Windows Server 2008 R2)
Flash Version: Shockwave Flash 24.0 r0

I have created a jsfiddle demonstrating the issue: https://jsfiddle.net/pheafph4/2/

The issue also reproduces on Chrome 55.
 
Labels: Needs-Bisect M-56
Components: -Blink Blink>DOM
Status: Available
Confirmed this is reproducing.
Waiting for bisect result.
 Issue 673094  has been merged into this issue.
Components: -Blink>DOM Blink>Bindings
Labels: -Pri-2 Pri-1
Reassigning to bindings team.

Looks like this is a regression that may affect interoperability.
Raising priority.
Cc: jochen@chromium.org verwa...@chromium.org
Components: Blink>JavaScript
Owner: jochen@chromium.org
This issue seems caused by V8.  jochen@, could you triage this issue?

Cc: machenb...@chromium.org vogelheim@chromium.org hablich@chromium.org
Labels: -Needs-Bisect ReleaseBlock-Stable prestable-55.0.2883.87 OS-Linux OS-Mac
Able to reproduce the issue on win10 chrome version 57.0.2948.0 and dev 56.0.2924.18 - output displays as shown in the screenshot

This is a regression in M55 and below is the info

Manual Bisect Info
==================
Good Build: 55.0.2852.0  
Bad Build: 55.0.2853.0

Bisect Tool Info:
=================
You are probably looking for a change made after 416716 (known good), but no later than 416717 (first known bad).
CHANGELOG URL:
The script might not always return single CL as suspectas some perf builds might get missing due to failure.
  https://chromium.googlesource.com/chromium/src/+log/fed504f8ede5e387e9a96dee229d5ce779be547b..fbe37a3e6651922530417a53ea7af5442a2370ff


Screen Shot 2016-12-12 at 2.43.30 PM.png
303 KB View Download
Labels: -M-56 M-55
Owner: jkummerow@chromium.org
Status: Assigned
Repros nicely w/ repro in #8. Behaviour changes at crrev.com/2277363002.

Here's a copy&pastable version usable in d8:

regress-673008.js
-------------------------------------------
var a = {
  "33": true, // 32 works
  "-1": true,
}

var strkeys = Object.keys(a).map(function (n) { return '' + n })
var numkeys = Object.keys(a).map(function (n) { return +n })
var keys = strkeys.concat(numkeys)

keys.forEach(function (n) {
  print(n + '\t' + (typeof n) + '\t' + a.hasOwnProperty(n))
})


Labels: -OS-Linux -OS-Windows -OS-Mac OS-All
Cc: amineer@chromium.org
jkummerow@ can you please update the bug regarding the ReleaseBlock-Stable label after your investigation today? 
Components: -Blink>Bindings
Status: Started
Fix: https://codereview.chromium.org/2568943002/

Not sure what to update regarding RB-S. As previous comments describe, it's wrong behavior.
Project Member Comment 15 by bugdroid1@chromium.org, Dec 12
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/bb753b6dd72e0d132456983248c8f695a692f25d

commit bb753b6dd72e0d132456983248c8f695a692f25d
Author: jkummerow <jkummerow@chromium.org>
Date: Mon Dec 12 20:12:18 2016

[stubs] Fix negative index lookup in hasOwnProperty

...and HasProperty, for dictionary-elements receivers.

BUG= chromium:673008 

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

[modify] https://crrev.com/bb753b6dd72e0d132456983248c8f695a692f25d/src/builtins/builtins-object.cc
[modify] https://crrev.com/bb753b6dd72e0d132456983248c8f695a692f25d/src/code-stub-assembler.cc
[add] https://crrev.com/bb753b6dd72e0d132456983248c8f695a692f25d/test/mjsunit/regress/regress-crbug-673008.js

Re #14: Indeed. Leaving RB-Stable label.

This should be rolled in next so if everything goes right it should be on tomorrows Canary which means we might be able to merge it tomorrow too.
Cc: kavvaru@chromium.org
Labels: Needs-Feedback
Tested the issue on windows 7 , Linux Ubuntu 14.04 and Mac 10.12.1 using chrome version 57.0.2950.0 and 57.0.2950.2(windows) with the URL https://jsfiddle.net/pheafph4/2/.Still seeing the last value as negative and false.

Please find the attached screen shot and confirm on the fix.

Thanks,
673008.png
113 KB View Download
Status: Fixed
The 2950 Canary does not contain the fix. It contains V8 r41637, whereas as you can see in #15, the fix is r41656.
Labels: Merge-TBD
[Auto-generated comment by a script] We noticed that this issue is targeted for M-55; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-55 label, otherwise remove Merge-TBD label. Thanks.
Cc: pbomm...@chromium.org gov...@chromium.org
Labels: TE-Verified-M57 TE-Verified-57.0.2951.0
Verified the issue on Mac 10.11.6,Ubuntu 14.04 and Win 10 using 57.0.2951.0 and its working fine now.

Could you please merge this into M56 and M55 if you have a plan for another stable push.
673008_Dec_15.mp4
310 KB View Download
Labels: merge-approved-5.5 Merge-approved-5.6
please merge to 5.5 and 5.6
 Issue 674245  has been merged into this issue.
Cc: jkummerow@chromium.org
 Issue 674085  has been merged into this issue.
Project Member Comment 24 by sheriffbot@chromium.org, Dec 19
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 25 by bugdroid1@chromium.org, Dec 19
Labels: merge-merged-5.6
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/6ce1e0a02544eb3080dd0a6511cda55ae0df8f92

commit 6ce1e0a02544eb3080dd0a6511cda55ae0df8f92
Author: Jakob Kummerow <jkummerow@chromium.org>
Date: Mon Dec 19 17:08:57 2016

Merged: [stubs] Fix negative index lookup in hasOwnProperty

Revision: bb753b6dd72e0d132456983248c8f695a692f25d

BUG= chromium:673008 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=ishell@chromium.org

Review-Url: https://codereview.chromium.org/2580413002 .
Cr-Commit-Position: refs/branch-heads/5.6@{#58}
Cr-Branched-From: bdd3886218dfe76e8560eb8a18401942452ae859-refs/heads/5.6.326@{#1}
Cr-Branched-From: 879f6599eee6e1dfcbe9a24bf688b261c03e9558-refs/heads/master@{#41014}

[modify] https://crrev.com/6ce1e0a02544eb3080dd0a6511cda55ae0df8f92/src/builtins/builtins-object.cc
[modify] https://crrev.com/6ce1e0a02544eb3080dd0a6511cda55ae0df8f92/src/code-stub-assembler.cc
[add] https://crrev.com/6ce1e0a02544eb3080dd0a6511cda55ae0df8f92/test/mjsunit/regress/regress-crbug-673008.js

Project Member Comment 26 by bugdroid1@chromium.org, Dec 19
Labels: merge-merged-5.5
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/7546f68620778181bc22923730471a39804afa86

commit 7546f68620778181bc22923730471a39804afa86
Author: Jakob Kummerow <jkummerow@chromium.org>
Date: Mon Dec 19 17:15:25 2016

Merged: [stubs] Fix negative index lookup in hasOwnProperty

Revision: bb753b6dd72e0d132456983248c8f695a692f25d

BUG= chromium:673008 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=ishell@chromium.org

Review-Url: https://codereview.chromium.org/2585323002 .
Cr-Commit-Position: refs/branch-heads/5.5@{#70}
Cr-Branched-From: 3cbd5838bd8376103daa45d69dade929ee4e0092-refs/heads/5.5.372@{#1}
Cr-Branched-From: b3c8b0ce2c9af0528837d8309625118d4096553b-refs/heads/master@{#40015}

[modify] https://crrev.com/7546f68620778181bc22923730471a39804afa86/src/builtins/builtins-object.cc
[modify] https://crrev.com/7546f68620778181bc22923730471a39804afa86/src/code-stub-assembler.cc
[add] https://crrev.com/7546f68620778181bc22923730471a39804afa86/test/mjsunit/regress/regress-crbug-673008.js

Labels: -Merge-TBD -merge-approved-5.5 -Merge-approved-5.6
When will this fix make it to Chrome BETA for platform = Android?
jkummerow@ can you please request merge to M56(branch : 2924) 
This is already merged, see c#25 - we just haven't shipped a beta with the fix yet.  That should be coming later today.
Ok cool. Any idea what final stable release will this be in for the following and what approx dates?

Windows
ioS
Android

Can I expect this in stable version 56? Your help is appreciated.


https://www.chromium.org/developers/calendar shows expected stable release dates; note they are projections, and are subject to change.  And yes, stable follows beta, so since the change is now showing up in beta you can expect the change to be in M56 stable.
Labels: NodeJS-Backport-Rejected
No further action needed for Node.js as this issue started in 5.5 and is already merged to 5.5.


 Issue 681139  has been merged into this issue.
Comment 35 Deleted
Chrome 56 for Android began shipping to 100% of users yesterday; it was unfortunately stuck at 15% and then 50% for ~2 weeks.
Sign in to add a comment