"Working as expected in other browsers" is what I meant to say :p

Bisect: 464871 (good) - 464872 (bad), 60.0.3073.0
https://chromium.googlesource.com/chromium/src/+log/ac392218..f18a2124?pretty=fuller
The only change is r464872 "Update V8 to version 6.0.3"

The V8 update log: https://chromium.googlesource.com/v8/v8/+log/fe9bb7e6..9583190f
Suspecting https://codereview.chromium.org/2811333002
"[builtins] HasOwnProperty: handle non-internalized string keys"

Nice bisecting - seems likely.

Still repros in Canary (61) 

jkummerow@ - can you take a look? It looks like this change is not compatible with host objects that have Web IDL "getters" defined.

I want to expand the scope of this reported bug to included BOTH newly created keys AND previously existing keys.

Using Chrome Version 60.0.3112.40 (Official Build) beta (64-bit) and the repro below, hasOwnProperty fails in both cases.


//PART 1: clear local storage, add 100 testing keys
localStorage.clear();
for (var i = 0; i < 100; i++) {
  let propName = 'item' + i;
  localStorage.setItem(propName, 'value' + i);
}


//PART 2: check hasOwnProperty, getItem results
for (var i = 0; i < 100; i++) {
  let propName = 'item' + i;
  console.log(propName + ', hasOwnProperty: '+ localStorage.hasOwnProperty(propName) + ', getItem result:' + localStorage.getItem(propName));
}


The console log will output results like this (hasOwnProperty is false, but getItem has a valid result)
item0, hasOwnProperty: false, getItem result:value0
item1, hasOwnProperty: false, getItem result:value1
item2, hasOwnProperty: false, getItem result:value2

If chrome is then completely closed and re-started and only part 2 is run, the results will be the same.

Even more curious, if I run part 2 multiple times, there will often be 5-10/100 keys that return true for hasOwnProperty...

Deleted: hasOwnProperty_repro.js 337 bytes

hasOwnProperty_repro.js 337 bytes View Download

Updated summary since it isn't scoped to sessionStorage or new keys.

> Even more curious, if I run part 2 multiple times, there will often be 5-10/100 keys that return true for hasOwnProperty...

The v8 change optimizes hasOwnProperty to say "if *no* object has this key as a defined property name then the answer is obviously false and exit early, otherwise do the normal check". The bug manifests because for localStorage/sessionStorage the key is sitting in a database record, not as property name, so the early exit is false, and what's needed is to do the normal check where the storage object overrides normal JS property access behavior and checks the database.

I suspect those 5-10 keys are ones where the key is an existing property name on some object, so the v8 change ends up doing the normal work.

 Issue 734929  has been merged into this issue.

thanks for the update - I did discover in continued testing that after running the loop, querying a single value like:

localStorage.hasOwnProperty('item84');

will return the correct value, and then subsequent runs of the loop will continue to return the correct value, until chrome is restarted.  It sounds like query outside the loop will move the key into a property.

Filed https://bugs.chromium.org/p/v8/issues/detail?id=6525 because I don't know the current best practice for chromium-v8 spanning bugs.

 Issue v8:6525  has been merged into this issue.

Thanks for the bisect. Will fix.

#12: One bug is enough.

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

commit 347e6215c5246ac0347b6fa9a8d0081d164ad1b3
Author: Jakob Kummerow <jkummerow@chromium.org>
Date: Thu Jun 29 14:46:18 2017

Fix HasOwnProperty stub for interceptors

When internalization of the key fails because the string does not
exist in the StringTable yet, then no regular object can possibly
have a property with that name, so just returning "false" is safe.
However, for objects with interceptors this is not true, as there
may well be intercepted properties whose keys have not been
internalized. So "special API objects" must take the slow path to
query any interceptors.

Bug:  chromium:735990 
Change-Id: Ibe6c4f8b14fef65738115f12167d3602bec3d9b7
Reviewed-on: https://chromium-review.googlesource.com/552550
Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46323}
[modify] https://crrev.com/347e6215c5246ac0347b6fa9a8d0081d164ad1b3/src/builtins/builtins-object-gen.cc
[modify] https://crrev.com/347e6215c5246ac0347b6fa9a8d0081d164ad1b3/test/cctest/test-api.cc

This seems like it might be a major regression; should we merge this to the 6.0 branch?

 Issue 739714  has been merged into this issue.

With jkummerow out this week, I'm going to argue we should merge this: failing hasOwnProperty checks have the potential to be a major source of regressions in production.

cbruni, can you confirm that this is fully fixed by 347e6215c5246ac0347b6fa9a8d0081d164ad1b3?

This bug requires manual review: M60 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Thanks adam for picking this up!

In response to #18, yes from what I know, this was a small oversight and should be fully fixed by the CL 347e6215c5246ac0347b6fa9a8d0081d164ad1b3.

I'd recommend a backmerge as well, given that this was broken in M60 and the fix is comparably simple.

Per comment 18, approving merge to M60. 

Needed to pull in fe048410f86f596bd72c0fc456be6021abaa3974 too (from  issue 707580 ) to make this a clean merge. Given that  issue 707580  was a clusterfuzz issue, this seems like a good idea anyway.

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

commit bada4ecc51f61a7287a4e8ddfe5a6a3d6ca3f48f
Author: Adam Klein <adamk@chromium.org>
Date: Fri Jul 07 15:21:14 2017

Merged: Squashed multiple commits.

Merged: [builtins] Make sure to perform ToPrimitive(key, hint string) in hasOwnProperty even if the receiver is a smi.
Revision: fe048410f86f596bd72c0fc456be6021abaa3974

Merged: Fix HasOwnProperty stub for interceptors
Revision: 347e6215c5246ac0347b6fa9a8d0081d164ad1b3

TBR=cbruni@chromium.org
BUG= chromium:707580 , chromium:735990 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true

Change-Id: Ic0c068fbea8e5a3e8c00aba0da3299d9d180dcb6
Reviewed-on: https://chromium-review.googlesource.com/563540
Reviewed-by: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.0@{#57}
Cr-Branched-From: 97dbf624a5eeffb3a8df36d24cdb2a883137385f-refs/heads/6.0.286@{#1}
Cr-Branched-From: 12e6f1cb5cd9616da7b9d4a7655c088778a6d415-refs/heads/master@{#45439}
[modify] https://crrev.com/bada4ecc51f61a7287a4e8ddfe5a6a3d6ca3f48f/src/builtins/builtins-object-gen.cc
[modify] https://crrev.com/bada4ecc51f61a7287a4e8ddfe5a6a3d6ca3f48f/test/cctest/test-api.cc
[add] https://crrev.com/bada4ecc51f61a7287a4e8ddfe5a6a3d6ca3f48f/test/mjsunit/regress/regress-crbug-707580.js

Thanks for picking this up! I agree that this was worthy of merging; it was on my to-do list for next week ;-)