New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 682150 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Jul 9
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 651770



Sign in to add a comment

Fix http://w3c-test.org/url/url-setters.html failures

Project Member Reported by yhirano@chromium.org, Jan 18 2017

Issue description

See issue 651770 for the context
 
From https://docs.google.com/spreadsheets/d/1dIBAZwkuGy-H1AEk9pT1xF-pLb2Du8zZUnSRmrNdtj0/edit#gid=1409104618 linked from  issue 651572 :

/url/url-setters.html
 Setting <https://example.net?lang=en-US#nav>.search = '??lang=fr'
 FAIL (Chrome) PASS (Firefox) PASS (Edge)
Cc: sigbjo...@opera.com

url = new URL('https://example.com/');
url.search = '??hoge'

We expect url.search returns '??hoge' but it actually returns '?hoge'. The reason is that both DOMURLUtils::setSearchInternal and KURL::setQuery called in it trim off the leading question character.

The former was added in Sep 2014 by sof[1]. The latter was added before that. IIUC the former is not needed at all.

sof@, can you tell me if my understanding is correct?

1: https://chromium.googlesource.com/chromium/src/+/4db1e7203ea51d375f11835c2ad56de335684d6a

Not sure how it ended up like this in setSearchInternal(), https://codereview.chromium.org/537103002/diff/20001/Source/core/dom/DOMURLUtils.cpp#newcode126 suggests some more invasive fixup of the input was needed at the time. But if it aligns with the spec to drop the special case, then we should definitely try.
Thank you. I uploaded https://codereview.chromium.org/2686653003/.
Project Member

Comment 5 by bugdroid1@chromium.org, Feb 8 2017

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

commit 29fc3042b1b3bea3e71a2c5b0a9a81ce435c2d39
Author: yhirano <yhirano@chromium.org>
Date: Wed Feb 08 10:03:30 2017

URL.search should return the set value via setSearch

DOMURLUtils and KURL trim off the leading question mark independently, which
leads to trimming off two leading questions in addition.
DOMURLUtilsReadOnly::search recovers one question mark and we lose one question
mark in total.

The special handling in DOMURLUtils is needed because KURL::setQuery handles
an empty string and a null string differently. KURL::setQuery("?") is
equivalent to KURL::setQuery("") but what we need is KURL::setQuery(String()).

This CL fixes the problem by calling KURL::setQuery(String()) if the given
value is "?", and calling KURL::setQuery with the given string if it starts
with a question mark and consists of two or more characters.

BUG= 682150 

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

[modify] https://crrev.com/29fc3042b1b3bea3e71a2c5b0a9a81ce435c2d39/third_party/WebKit/LayoutTests/fast/domurl/url-search.html
[modify] https://crrev.com/29fc3042b1b3bea3e71a2c5b0a9a81ce435c2d39/third_party/WebKit/Source/core/dom/DOMURLUtils.cpp

Status: Fixed (was: Assigned)

Comment 7 by sim...@opera.com, Apr 4 2017

We now fail when setting .search = '?'

Fail	URL: Setting <https://example.net?lang=en-US#nav>.search = '?'	assert_equals: expected "https://example.net/?#nav" but got "https://example.net/#nav"
Fail	<a>: Setting <https://example.net?lang=en-US#nav>.search = '?'	assert_equals: expected "https://example.net/?#nav" but got "https://example.net/#nav"
Fail	<area>: Setting <https://example.net?lang=en-US#nav>.search = '?'	assert_equals: expected "https://example.net/?#nav" but got "https://example.net/#nav"

Is that a regression from the above change, or was it like that before?

I get 299 failures on http://web-platform.test:8000/url/url-setters.html using
Google Chrome	59.0.3062.0 (Official Build) canary (64-bit)
Revision	fb5c907d801f5fb0d6542c14f29c5749ae78abc7-refs/heads/master@{#461587}

(I'm happy to report a new issue if you like, let me know.)

Comment 8 by ricea@chromium.org, Jul 6 2017

There are still 299 failures for this file on Linux and 326 on Windows. URL on Windows attempts to be smart about Windows drive letters which leads to more failures.

Should I reopen this issue or open a new issue for the remaining failures?
Cc: yhirano@chromium.org
Owner: ----
Status: Available (was: Fixed)
> #7

It's not a regression. Even if I reverted the change I see the failure.

The test succeeds with Firefox and Edge, so I think we should reopen the issue.
Correction: The first one fails on Edge. The second and third ones pass on Edge and Firefox.
Project Member

Comment 11 by sheriffbot@chromium.org, Jul 6

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.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Status: Fixed (was: Untriaged)
Let's close this bug as Fixed though there are still assertion failures on url-setters.html. This was not for all the assertions in the test.

Sign in to add a comment