Issue metadata
Sign in to add a comment
|
hasOwnProperty returning inconsistent results
Reported by
canada.r...@gmail.com,
Dec 9 2016
|
||||||||||||||||||||||
Issue descriptionUserAgent: 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.
,
Dec 12 2016
,
Dec 12 2016
Confirmed this is reproducing. Waiting for bisect result.
,
Dec 12 2016
Issue 673094 has been merged into this issue.
,
Dec 12 2016
Reassigning to bindings team. Looks like this is a regression that may affect interoperability. Raising priority.
,
Dec 12 2016
This issue seems caused by V8. jochen@, could you triage this issue?
,
Dec 12 2016
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
,
Dec 12 2016
,
Dec 12 2016
,
Dec 12 2016
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)) })
,
Dec 12 2016
,
Dec 12 2016
,
Dec 12 2016
jkummerow@ can you please update the bug regarding the ReleaseBlock-Stable label after your investigation today?
,
Dec 12 2016
Fix: https://codereview.chromium.org/2568943002/ Not sure what to update regarding RB-S. As previous comments describe, it's wrong behavior.
,
Dec 12 2016
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
,
Dec 12 2016
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.
,
Dec 13 2016
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,
,
Dec 13 2016
[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.
,
Dec 15 2016
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.
,
Dec 15 2016
please merge to 5.5 and 5.6
,
Dec 15 2016
Issue 674245 has been merged into this issue.
,
Dec 15 2016
,
Dec 19 2016
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
,
Dec 19 2016
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
,
Dec 19 2016
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
,
Dec 19 2016
,
Jan 5 2017
When will this fix make it to Chrome BETA for platform = Android?
,
Jan 5 2017
jkummerow@ can you please request merge to M56(branch : 2924)
,
Jan 5 2017
This is already merged, see c#25 - we just haven't shipped a beta with the fix yet. That should be coming later today.
,
Jan 5 2017
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.
,
Jan 6 2017
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.
,
Jan 10 2017
No further action needed for Node.js as this issue started in 5.5 and is already merged to 5.5.
,
Jan 16 2017
Issue 681139 has been merged into this issue.
,
Feb 23 2017
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 |
|||||||||||||||||||||||
Comment 1 by ajha@chromium.org
, Dec 12 2016