Bug 162345 – Setting URL.search to '' results in a stringified URL ending in '?'

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeffrey Posnick]

Summary:
A JavaScript developer can clear out the search portion of a URL object by using the search setter with the value ''. In Safari TP, doing so results in a URL object that includes a trailing '?' character, which does not appear to match the behavior in other browsers that support the URL object. Other browsers stringify that URL object without a trailing '?'.

Steps to Reproduce:
In any JavaScript execution context, e.g. the Safari Developer Tools' console, run the following:

    var url = new URL('https://example.com/?testing');
    url.search = '';
    console.log(url.toString());

Expected Results:
The expected logged value is

'https://example.com/'

Actual Results:
The actual logged value is

'https://example.com/?'

Version:
Release 13 (Safari 10.0, WebKit 11603.1.5)

Notes:
The relevant specification is at https://url.spec.whatwg.org/?a=b#concept-urlsearchparams-update

All other modern browsers (Chrome, Firefox, and Edge) appear to properly stringify that example without a trailing '?'.

Configuration:
This happens all the time.

[Anthony Ricaud]

This is also an issue when using URLSearchParams:

const url = new URL('https://foo.com/?bar=baz');
url.searchParams.delete('bar');
console.log(url.toString());

This will include an extra ?

[Radar WebKit Bug Importer]

<rdar://problem/31800441>

[Alex Christensen]

Created attachment 308026 [details]
Patch

[Alex Christensen]

Comment on attachment 308026 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308026&action=review

> LayoutTests/imported/w3c/web-platform-tests/url/url-setters-expected.txt:545
> +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"

Anne, these tests fail in Chrome and will fail in WebKit if we make this change.  They pass in Firefox, which is against my interpretation of https://url.spec.whatwg.org/#dom-url-search setter step 3.  What do you think?

[Build Bot]

Comment on attachment 308026 [details]
Patch

Attachment 308026 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3598005

New failing tests:
fast/forms/get-forms-to-about-blank.html
fast/forms/submit-nil-value-field-assert.html
http/tests/navigation/onload-navigation-iframe.html
http/tests/navigation/onload-navigation-iframe-timeout.html
fast/forms/empty-get.html
http/tests/navigation/back-send-referrer.html
http/tests/history/redirect-js-form-submit-2-seconds.html
http/tests/download/inherited-encoding-form-submission-result.html
fast/loader/form-submission-before-load-get.html
fast/history/gesture-before-onload-form-submit.html
http/tests/navigation/onload-navigation-iframe-2.html
fast/forms/missing-action.html
fast/dom/HTMLAnchorElement/set-href-attribute-search.html
http/tests/history/redirect-js-form-submit-0-seconds.html
http/tests/history/redirect-js-form-submit-before-load.html

[Build Bot]

Created attachment 308037 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308026 [details]
Patch

Attachment 308026 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3598104

New failing tests:
fast/forms/submit-nil-value-field-assert.html
http/tests/navigation/onload-navigation-iframe.html
http/tests/history/redirect-js-form-submit-2-seconds.html
http/tests/navigation/onload-navigation-iframe-timeout.html
fast/forms/empty-get.html
http/tests/navigation/back-send-referrer.html
http/tests/download/inherited-encoding-form-submission-result.html
fast/dom/HTMLAnchorElement/set-href-attribute-search.html
fast/forms/missing-action.html
fast/history/gesture-before-onload-form-submit.html
http/tests/navigation/onload-navigation-iframe-2.html
fast/loader/form-submission-before-load-get.html
http/tests/history/redirect-js-form-submit-before-load.html
http/tests/history/redirect-js-form-submit-0-seconds.html
fast/forms/get-forms-to-about-blank.html

[Build Bot]

Created attachment 308038 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308026 [details]
Patch

Attachment 308026 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3598102

New failing tests:
fast/forms/submit-nil-value-field-assert.html
fast/forms/get-forms-to-about-blank.html
http/tests/navigation/onload-navigation-iframe-timeout.html
fast/forms/empty-get.html
http/tests/navigation/back-send-referrer.html
http/tests/history/redirect-js-form-submit-2-seconds.html
fast/dom/HTMLAnchorElement/set-href-attribute-search.html
fast/forms/missing-action.html
fast/history/gesture-before-onload-form-submit.html
http/tests/navigation/onload-navigation-iframe-2.html
fast/loader/form-submission-before-load-get.html
http/tests/history/redirect-js-form-submit-before-load.html
http/tests/history/redirect-js-form-submit-0-seconds.html
http/tests/navigation/onload-navigation-iframe.html

[Build Bot]

Created attachment 308040 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308026 [details]
Patch

Attachment 308026 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3598178

New failing tests:
fast/forms/submit-nil-value-field-assert.html
fast/forms/get-forms-to-about-blank.html
http/tests/navigation/onload-navigation-iframe-timeout.html
fast/forms/empty-get.html
http/tests/navigation/back-send-referrer.html
http/tests/history/redirect-js-form-submit-2-seconds.html
fast/dom/HTMLAnchorElement/set-href-attribute-search.html
http/tests/navigation/onload-navigation-iframe.html
http/tests/navigation/onload-navigation-iframe-2.html
fast/forms/missing-action.html
http/tests/history/redirect-js-form-submit-before-load.html
http/tests/history/redirect-js-form-submit-0-seconds.html

[Build Bot]

Created attachment 308047 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Anne van Kesteren]

(In reply to Alex Christensen from comment #4)
> Anne, these tests fail in Chrome and will fail in WebKit if we make this
> change.  They pass in Firefox, which is against my interpretation of
> https://url.spec.whatwg.org/#dom-url-search setter step 3.  What do you
> think?

I think you're reading it wrong or WebKit has perhaps the wrong internal model for a URL query. There's a distinction between query being null and it being the empty string. There's also a distinction between setting url.search to "" (makes query null) and setting it to "?" (makes query the empty string).

Does that help?

[Alex Christensen]

Comment on attachment 308026 [details]
Patch

Yep, thanks Anne.  Looks like I'll need to do some more serious URL surgery.

[Chris Dumez]

Created attachment 310343 [details]
Patch

[Chris Dumez]

Created attachment 310344 [details]
Patch

[Sam Weinig]

(In reply to Chris Dumez from comment #16)
> Created attachment 310344 [details]
> Patch

I'd like Alex to review this.

[Alex Christensen]

Comment on attachment 310344 [details]
Patch

Are there no tests in LayoutTests/imported/w3c/web-platform-tests/url/url-setters.html that start passing because of this change?  If not, then I think something is wrong.  Maybe it's skipped or marked as pass/fail from the days before URLParser when it used to assert.

[Chris Dumez]

(In reply to Alex Christensen from comment #18)
> Comment on attachment 310344 [details]
> Patch
> 
> Are there no tests in
> LayoutTests/imported/w3c/web-platform-tests/url/url-setters.html that start
> passing because of this change?  If not, then I think something is wrong. 
> Maybe it's skipped or marked as pass/fail from the days before URLParser
> when it used to assert.

Test seems to be skipped on wk1 but not wk2. I can check if the test is somehow outdated. I don't know if this test is supposed to cover the issue in the bug.

You can see however that the tests I updated / rebaselined covers *exactly* the case covered in this bug and that we now get the correct result. Given this, saying if no WPT tests started passing, then the patch is wrong, seems odd.

Anyway, I'll take a look at url-setters.html to see if it covers this case and if it is up-to-date.

[Chris Dumez]

(In reply to Chris Dumez from comment #19)
> (In reply to Alex Christensen from comment #18)
> > Comment on attachment 310344 [details]
> > Patch
> > 
> > Are there no tests in
> > LayoutTests/imported/w3c/web-platform-tests/url/url-setters.html that start
> > passing because of this change?  If not, then I think something is wrong. 
> > Maybe it's skipped or marked as pass/fail from the days before URLParser
> > when it used to assert.
> 
> Test seems to be skipped on wk1 but not wk2. I can check if the test is
> somehow outdated. I don't know if this test is supposed to cover the issue
> in the bug.
> 
> You can see however that the tests I updated / rebaselined covers *exactly*
> the case covered in this bug and that we now get the correct result. Given
> this, saying if no WPT tests started passing, then the patch is wrong, seems
> odd.
> 
> Anyway, I'll take a look at url-setters.html to see if it covers this case
> and if it is up-to-date.

I looked at url-setters.html and it is up-to-date. It also seems like it should cover this case (or similar cases at least). I'll investigate why the bots do not see new passes.

[Chris Dumez]

(In reply to Chris Dumez from comment #20)
> (In reply to Chris Dumez from comment #19)
> > (In reply to Alex Christensen from comment #18)
> > > Comment on attachment 310344 [details]
> > > Patch
> > > 
> > > Are there no tests in
> > > LayoutTests/imported/w3c/web-platform-tests/url/url-setters.html that start
> > > passing because of this change?  If not, then I think something is wrong. 
> > > Maybe it's skipped or marked as pass/fail from the days before URLParser
> > > when it used to assert.
> > 
> > Test seems to be skipped on wk1 but not wk2. I can check if the test is
> > somehow outdated. I don't know if this test is supposed to cover the issue
> > in the bug.
> > 
> > You can see however that the tests I updated / rebaselined covers *exactly*
> > the case covered in this bug and that we now get the correct result. Given
> > this, saying if no WPT tests started passing, then the patch is wrong, seems
> > odd.
> > 
> > Anyway, I'll take a look at url-setters.html to see if it covers this case
> > and if it is up-to-date.
> 
> I looked at url-setters.html and it is up-to-date. It also seems like it
> should cover this case (or similar cases at least). I'll investigate why the
> bots do not see new passes.

The test must be skipped as I see this when rebaselining it locally:
-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"
-FAIL URL: Setting <https://example.net?lang=en-US>.search = '' assert_equals: expected "https://example.net/" but got "https://example.net/?"
-FAIL <a>: Setting <https://example.net?lang=en-US>.search = '' assert_equals: expected "https://example.net/" but got "https://example.net/?"
-FAIL <area>: Setting <https://example.net?lang=en-US>.search = '' assert_equals: expected "https://example.net/" but got "https://example.net/?"
-FAIL URL: Setting <https://example.net>.search = '' assert_equals: expected "https://example.net/" but got "https://example.net/?"
-FAIL <a>: Setting <https://example.net>.search = '' assert_equals: expected "https://example.net/" but got "https://example.net/?"
-FAIL <area>: Setting <https://example.net>.search = '' assert_equals: expected "https://example.net/" but got "https://example.net/?"
+PASS URL: Setting <https://example.net?lang=en-US#nav>.search = '' 
+PASS <a>: Setting <https://example.net?lang=en-US#nav>.search = '' 
+PASS <area>: Setting <https://example.net?lang=en-US#nav>.search = '' 
+PASS URL: Setting <https://example.net?lang=en-US>.search = '' 
+PASS <a>: Setting <https://example.net?lang=en-US>.search = '' 
+PASS <area>: Setting <https://example.net?lang=en-US>.search = '' 
+PASS URL: Setting <https://example.net>.search = '' 
+PASS <a>: Setting <https://example.net>.search = '' 
+PASS <area>: Setting <https://example.net>.search = ''

[Chris Dumez]

Created attachment 310425 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #22)
> Created attachment 310425 [details]
> Patch

Rebaselined the WPT and unskipped it for WK1. I cannot find any reason why it would have been skipped for WK2.

[WebKit Commit Bot]

Comment on attachment 310425 [details]
Patch

Clearing flags on attachment: 310425

Committed r217004: <http://trac.webkit.org/changeset/217004>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.