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 2 users

Issue metadata

Status: ExternalDependency
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on: View detail
issue 510994
issue 771959
issue 753073

Blocking:
issue 651572



Sign in to add a comment
link

Issue 651774: 3 failing web-platform-tests for Web Storage that pass in Firefox and Edge

Reported by foolip@chromium.org, Sep 30 2016 Project Member

Issue description

http://w3c-test.org/webstorage/idlharness.html
http://w3c-test.org/webstorage/storage_builtins.html
http://w3c-test.org/webstorage/storage_setitem.html

These failures need analysis, but may point to low-hanging fruit for improving interop. Judgement is required, the failures may be entangled with spec issues.

See  issue 651572  for the source of this data, which includes failing subtests.

Note: Results may have changed in the interim.
 

Comment 1 by foolip@chromium.org, Sep 30 2016

I suspect that much of the failures are due to generic bindings issues, but I think not all of them, attention required.

Comment 2 by jsb...@chromium.org, Sep 30 2016

Blockedon: 510994
Components: -Blink>Storage Blink>Storage>DOMStorage

Comment 3 by jsb...@chromium.org, Sep 30 2016

Status: Available (was: Untriaged)
storage_builtins.html is issue 510994

storage_setitem.html appears to pass in M53

Comment 4 by sheriffbot@chromium.org, Oct 2 2017

Project Member
Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

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

Comment 5 by jsb...@chromium.org, Oct 2 2017

Owner: raphael....@intel.com
Status: aval (was: Untriaged)

Comment 6 by jsb...@chromium.org, Oct 2 2017

Status: Available (was: Aval)
raphael, maybe you can check to see if we're passing or what the remaining failures are?

I'm pretty sure we're still failing cases covered by issue 510994 but the others may be fixed with your CL?

Comment 7 by bugdroid1@chromium.org, Oct 2 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/950f7f23ed5a060b0af65f914230f46eee858c01

commit 950f7f23ed5a060b0af65f914230f46eee858c01
Author: Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com>
Date: Mon Oct 02 20:04:10 2017

Storage.idl: Drop [NotEnumerable], remove anonymous getter/setter/deleter.

Fix a few TODO items by making our IDL closer to the spec:

* There is nothing in the spec saying the operations declared in the Storage
  interface should not be enumerable. Since the default according to WebIDL
  is for operations to be enumerable, drop the non-standard [NotEnumerable]
  extended attribute.
  This is a user-visible behavior change that matches the spec, as well as
  Gecko (since Firefox 34) and WebKit (since r195760, Safari 10.1).

* Merge the anonymous getter, setter and deleter into the existing getItem,
  setItem and removeItem operations as mandated by the spec. One visible
  difference is that the anonymous getter returned a DOMString, whereas
  getItem() returns a DOMString? (i.e. null is valid return value).

Adjust a few tests in the process:
* external/wpt/webstorage/storage_enumerate.html: the first test did not
  make a lot of sense; now that Storage's operations are enumerable, looping
  with a for-in would add them to |enumeratedArray| unless we add a
  hasOwnProperty() check. Adding that check no longer adds
  "prototypeTestKey" to |enumeratedArray| though, but that is working as
  expected since it is not really a key but rather something inherited from
  Storage's prototype. Add a test for that as well.
* Drop a few Blink-only tests from storage/domstorage that were duplicating
  the same checks done by the test above. It makes sense to rely on WPT
  instead of fixing them as well.

Bug: 651774
Change-Id: Ibf90700eb3d472f66b4ff2dc6ab9b6e6a4b2f211
Reviewed-on: https://chromium-review.googlesource.com/691974
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Reviewed-by: Joshua Bell <jsbell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505745}
[modify] https://crrev.com/950f7f23ed5a060b0af65f914230f46eee858c01/third_party/WebKit/LayoutTests/external/wpt/html/dom/interfaces-expected.txt
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/external/wpt/webstorage/idlharness-expected.txt
[modify] https://crrev.com/950f7f23ed5a060b0af65f914230f46eee858c01/third_party/WebKit/LayoutTests/external/wpt/webstorage/storage_enumerate.html
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/platform/linux/virtual/mojo-localstorage/external/wpt/webstorage/idlharness-expected.txt
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/platform/mac/virtual/mojo-localstorage/external/wpt/webstorage/idlharness-expected.txt
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/storage/domstorage/localstorage/enumerate-storage-expected.txt
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/storage/domstorage/localstorage/enumerate-storage.html
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/storage/domstorage/sessionstorage/enumerate-storage-expected.txt
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/storage/domstorage/sessionstorage/enumerate-storage.html
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/storage/domstorage/sessionstorage/enumerate-with-length-and-key-expected.txt
[delete] https://crrev.com/b7a2616cce819b0d9eefc6af199918da865fef3d/third_party/WebKit/LayoutTests/storage/domstorage/sessionstorage/enumerate-with-length-and-key.html
[modify] https://crrev.com/950f7f23ed5a060b0af65f914230f46eee858c01/third_party/WebKit/Source/modules/storage/Storage.cpp
[modify] https://crrev.com/950f7f23ed5a060b0af65f914230f46eee858c01/third_party/WebKit/Source/modules/storage/Storage.h
[modify] https://crrev.com/950f7f23ed5a060b0af65f914230f46eee858c01/third_party/WebKit/Source/modules/storage/Storage.idl

Comment 8 by raphael....@intel.com, Oct 3 2017

Labels: -Hotlist-Recharge-Cold
Status: Started (was: Available)
Sure; I don't have access to my primary build machine until Thursday to quickly build content_shell and run the webstorage tests, but I'll get to that as soon as I can.

Comment 9 by raphael....@intel.com, Oct 3 2017

Meanwhile, I've added a few chromestatus.com entries for the user-visible changes mentioned in comment #7:
- https://www.chromestatus.com/feature/5120599789666304 (Web Storage: Methods are now enumerable)
- https://www.chromestatus.com/feature/5700418495578112 (Web Storage: Anonymous getter may return null)

Comment 10 by phistuck@gmail.com, Oct 3 2017

#9 - does that mean that one or more of the following snippets (for example) -
1. for (x in localStorage) {...}
2. Object.keys(localStorage)
Would now also include getItem and similar?
If so, it seems like a potential compatibility issue and probably needs an intent thread.

Comment 11 by phistuck@gmail.com, Oct 3 2017

#9 - And for me localStorage.a returns undefined, not the empty string, so how can I trigger this anonymous getter?

Anyway, on the surface, those two changes sounds like compatibility breaks that (I think) merit intents, use counters, and/or a deprecation period.

Comment 12 by raphael....@intel.com, Oct 4 2017

Cc: michaeln@chromium.org
> 1. for (x in localStorage) {...}
> 2. Object.keys(localStorage)
> Would now also include getItem and similar?

Yes for 1, no for 2: getItem() and friends are in Storage.prototype, so
they show up in for-in loops but not Object.keys().

> #9 - And for me localStorage.a returns undefined, not the empty
> string, so how can I trigger this anonymous getter?

Thanks for spotting that; I misread the old code and the bindings side
indeed just turns the empty strings returned by the old anonymous getter
code into undefined. I've updated the chromestatus.com entry.

> Anyway, on the surface, those two changes sounds like compatibility
> breaks that (I think) merit intents, use counters, and/or a
> deprecation period.

I asked the reviewers in Gerrit if they'd prefer I sent an I2I email
first, but jsbell seemed fine with the patch.

jsbell/michaeln: would you prefer that I reverted it (at least until M63
branches) and/or started an intent thread on blink-dev?

Comment 13 by jsb...@chromium.org, Oct 4 2017

This seems below the bar for requiring an intent to me, but sending a note to blink-dev wouldn't hurt. Sites that relied on this would have been broken in Firefox/Safari, and it's unfortunately not the kind of change we can validate using HTTPArchive or UMA stats and we can't issue console warnings. It'll have to get feedback from anyone with an affected (Chrome-only) site.

But as noted, an FYI message to blink-dev wouldn't hurt.

Comment 15 by raphael....@intel.com, Oct 5 2017

Blockedon: 771959

Comment 16 by raphael....@intel.com, Oct 5 2017

Blockedon: v8:4958

Comment 17 by raphael....@intel.com, Oct 5 2017

> raphael, maybe you can check to see if we're passing or what
> the remaining failures are?
>
> I'm pretty sure we're still failing cases covered by issue 510994
> but the others may be fixed with your CL?

Right now, we're failing 3 tests:
* storage_builtins.html (bug 510994)
* storage_session_window_noopener.html (new test from Sept 2017, we need to adapt to a change in the HTML spec, see bug 771959)
* storage_string_conversion.html (started failing recently, blocked on  bug v8:4958 ).

The 3 bugs mentioned above are now marked as blocking this one; for now, there isn't much else to do here.

Comment 18 by raphael....@intel.com, Oct 5 2017

Status: ExternalDependency (was: Started)

Comment 19 by jsb...@chromium.org, Oct 5 2017

Thank you raphael - for the investigation, tracking bugs, and follow-up on the v8 issue!

\o/

Comment 20 by raphael....@intel.com, Feb 1 2018

Blockedon: -v8:4958 753073
I'm changing the dependency on  bug v8:4958  to a dependency on  issue 753073 , which tracks the LayoutTests side of that V8 bug more closely.

FWIW, storage_string_conversion.html started working after the rebaseline commit from the bug above.

Comment 21 by foolip@chromium.org, Oct 12

https://wpt.fyi/results/webstorage/storage_builtins.html?sha=434ca47448 still shows Chrome failing and Edge and Firefox passing. Safari also fails though.

And https://wpt.fyi/results/webstorage/storage_setitem.html?sha=434ca47448 is reported as a crash for Chrome. Needs investigation.

Sign in to add a comment