New issue
Advanced search Search tips

Issue 677322 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

URLSearchParams iterator snapshots, but it should be live

Project Member Reported by domenic@chromium.org, Dec 28 2016

Issue description

Chrome Version: Version 57.0.2965.0 (Official Build) canary (64-bit)
OS: Windows 10

Test code:

const url = new URL("http://domain.com/?var1=value&var2=");

for (let param of url.searchParams) {
  console.log(param[0]);
  url.searchParams.delete(param[0]);
}

Should output: var1

In Chrome, outputs: var1, var2

Per spec, https://heycam.github.io/webidl/#dfn-iterator-prototype-object next() method step 7, the list of values to iterate over is re-fetched every time the next() method is called, i.e. is consulted live. However, Chrome's implementation takes a snapshot: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/URLSearchParams.cpp?rcl=0&l=16

I'm not sure if this is a more general problem with our Web IDL iterator implementation, so I'll tag this as a bindings issue in case people want to check the other iterables in our codebase.

Via https://github.com/whatwg/url/issues/188
 
Cc: tyoshino@chromium.org

Comment 3 by ricea@chromium.org, Jan 20 2017

Status: Available (was: Untriaged)
Project Member

Comment 4 by sheriffbot@chromium.org, Feb 15 2018

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 ricea@chromium.org, Feb 19 2018

Status: Available (was: Untriaged)
It's still doing it. Is there a wpt test for this behaviour?

I think the issue is that the URLSearchParamsIterationSource() constructor copies. It appears it would work okay to pass a pointer instead.
As far as I can see, there's url/urlsearchparams-delete.html and url/urlsearchparams-foreach.html, but not something combining both (i.e. deleting inside a loop).
Cc: -tyoshino@chromium.org
Labels: -Hotlist-Recharge-Cold

Comment 8 by ricea@chromium.org, Mar 30 2018

Owner: ricea@chromium.org
Status: Started (was: Available)
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c1627f7a35299d6aa99413a348586508d9a34f3

commit 1c1627f7a35299d6aa99413a348586508d9a34f3
Author: Adam Rice <ricea@chromium.org>
Date: Thu Apr 05 07:47:08 2018

Make URLSearchParamsIterationSource not snapshot

Previously URLSearchParamsIterationSource iterated over a copy of
URLSearchParams, but it is specified in the standard as iterating over
the live list.

Modify it to reference the original URLSearchParams object instead of
making a copy.

Also add web-platform tests for delete during foreach, and a chromium-
specific test for GC of URLSearchParams during iteration.

Remove failing expectations.

BUG= 677322 

Change-Id: I8c53fd8dd9863fe1146c5b7849d4f08245b37bc1
Reviewed-on: https://chromium-review.googlesource.com/987839
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548363}
[delete] https://crrev.com/4fc8b7812fee283eed09034ee68dea8cbf39e586/third_party/WebKit/LayoutTests/external/wpt/url/urlsearchparams-foreach-expected.txt
[modify] https://crrev.com/1c1627f7a35299d6aa99413a348586508d9a34f3/third_party/WebKit/LayoutTests/external/wpt/url/urlsearchparams-foreach.html
[add] https://crrev.com/1c1627f7a35299d6aa99413a348586508d9a34f3/third_party/WebKit/LayoutTests/fast/domurl/chromium/urlsearchparams-iterable-gc.html
[modify] https://crrev.com/1c1627f7a35299d6aa99413a348586508d9a34f3/third_party/WebKit/Source/core/url/URLSearchParams.cpp

Comment 10 by ricea@chromium.org, Apr 12 2018

Status: Fixed (was: Started)
Labels: M-67

Sign in to add a comment