Bug 162345 - Setting URL.search to '' results in a stringified URL ending in '?'
Summary: Setting URL.search to '' results in a stringified URL ending in '?'
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Safari Technology Preview
Hardware: All All
: P2 Minor
Assignee: Chris Dumez
URL: https://url.spec.whatwg.org/#dom-url-...
Keywords: HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2016-09-21 09:48 PDT by Jeffrey Posnick
Modified: 2017-05-17 14:50 PDT (History)
14 users (show)

See Also:


Attachments
Patch (7.90 KB, patch)
2017-04-24 17:21 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-elcapitan (2.86 MB, application/zip)
2017-04-24 18:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (1.24 MB, application/zip)
2017-04-24 18:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.17 MB, application/zip)
2017-04-24 18:42 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews125 for ios-simulator-wk2 (83.42 MB, application/zip)
2017-04-24 19:29 PDT, Build Bot
no flags Details
Patch (6.34 KB, patch)
2017-05-16 18:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.34 KB, patch)
2017-05-16 18:50 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.78 KB, patch)
2017-05-17 12:17 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeffrey Posnick 2016-09-21 09:48:49 PDT
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.
Comment 1 Anthony Ricaud 2017-04-16 12:02:05 PDT
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 ?
Comment 2 Radar WebKit Bug Importer 2017-04-24 16:45:25 PDT
<rdar://problem/31800441>
Comment 3 Alex Christensen 2017-04-24 17:21:15 PDT
Created attachment 308026 [details]
Patch
Comment 4 Alex Christensen 2017-04-24 17:22:56 PDT
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?
Comment 5 Build Bot 2017-04-24 18:31:02 PDT
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
Comment 6 Build Bot 2017-04-24 18:31:03 PDT
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
Comment 7 Build Bot 2017-04-24 18:35:59 PDT
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
Comment 8 Build Bot 2017-04-24 18:36:00 PDT
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
Comment 9 Build Bot 2017-04-24 18:42:07 PDT
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
Comment 10 Build Bot 2017-04-24 18:42:08 PDT
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
Comment 11 Build Bot 2017-04-24 19:29:26 PDT
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
Comment 12 Build Bot 2017-04-24 19:29:30 PDT
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
Comment 13 Anne van Kesteren 2017-04-25 00:58:16 PDT
(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?
Comment 14 Alex Christensen 2017-04-25 09:28:45 PDT
Comment on attachment 308026 [details]
Patch

Yep, thanks Anne.  Looks like I'll need to do some more serious URL surgery.
Comment 15 Chris Dumez 2017-05-16 18:48:04 PDT
Created attachment 310343 [details]
Patch
Comment 16 Chris Dumez 2017-05-16 18:50:30 PDT
Created attachment 310344 [details]
Patch
Comment 17 Sam Weinig 2017-05-17 10:36:50 PDT
(In reply to Chris Dumez from comment #16)
> Created attachment 310344 [details]
> Patch

I'd like Alex to review this.
Comment 18 Alex Christensen 2017-05-17 11:55:33 PDT
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.
Comment 19 Chris Dumez 2017-05-17 12:04:11 PDT
(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.
Comment 20 Chris Dumez 2017-05-17 12:11:50 PDT
(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.
Comment 21 Chris Dumez 2017-05-17 12:13:05 PDT
(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 = ''
Comment 22 Chris Dumez 2017-05-17 12:17:51 PDT
Created attachment 310425 [details]
Patch
Comment 23 Chris Dumez 2017-05-17 12:18:51 PDT
(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.
Comment 24 WebKit Commit Bot 2017-05-17 14:50:15 PDT
Comment on attachment 310425 [details]
Patch

Clearing flags on attachment: 310425

Committed r217004: <http://trac.webkit.org/changeset/217004>
Comment 25 WebKit Commit Bot 2017-05-17 14:50:17 PDT
All reviewed patches have been landed.  Closing bug.