Bug 171345 - URLSearchParams should be reflective
Summary: URLSearchParams should be reflective
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-26 15:15 PDT by Adam Duren
Modified: 2017-04-28 13:43 PDT (History)
4 users (show)

See Also:


Attachments
Patch (5.19 KB, patch)
2017-04-28 10:30 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (1.01 MB, application/zip)
2017-04-28 12:42 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Duren 2017-04-26 15:15:17 PDT
Overview:

URLSearchParams does not return the same value for input generated by URLSearchParams when a param includes the `+` character.

Steps to Reproduce: 

```
var testParam = '+15555555555';
var params = new URLSearchParams()
params.set('query', testParam)

var newParams = new URLSearchParams(params.toString())

console.log(params.toString())                    // query=%2B15555555555
console.log(params.get('query') === testParam)    // Pass
console.log(newParams.get('query') === testParam) // Fail
```

Actual Results: 

I would expect that getting the query param from the URLSearchParams instance that was created with the output of URLSearchParams would equal the original value.
In this case that newParams.get('query') is be ' 15555555555' instead of '+15555555555'

Expected Results: 

I would expect that getting the query param from the URLSearchParams instance that was created with the output of URLSearchParams would equal the original value.
In this case that newParams.get('query') would be '+15555555555'
Comment 1 Chris Dumez 2017-04-28 08:47:16 PDT
Test case:
- http://jsbin.com/wipeqozazo/1/edit?html,output

Passes in Firefox and Chrome, fails in STP.
Comment 2 Chris Dumez 2017-04-28 08:57:49 PDT
As per https://url.spec.whatwg.org/#concept-urlencoded-parser, it looks like we should replace '+' with 0x20 *before* percent-decoding, not after.

It looks like we do this in reverse in URLParser::parseURLEncodedForm().
Comment 3 Chris Dumez 2017-04-28 10:30:11 PDT
Created attachment 308553 [details]
Patch
Comment 4 Build Bot 2017-04-28 12:42:57 PDT
Comment on attachment 308553 [details]
Patch

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

New failing tests:
js/dom/Promise-static-all.html
Comment 5 Build Bot 2017-04-28 12:42:58 PDT
Created attachment 308576 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 6 Chris Dumez 2017-04-28 12:48:16 PDT
(In reply to Build Bot from comment #4)
> Comment on attachment 308553 [details]
> Patch
> 
> Attachment 308553 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: http://webkit-queues.webkit.org/results/3627956
> 
> New failing tests:
> js/dom/Promise-static-all.html

this is unlikely to be related.
Comment 7 Alex Christensen 2017-04-28 13:15:50 PDT
Comment on attachment 308553 [details]
Patch

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

> Source/WebCore/platform/URLParser.cpp:2819
> +            auto name = formURLDecode(bytes.toString().replace('+', 0x20));

We should make a StringView.replace that returns a String.  This does an unnecessary copy.  Maybe in a followup.
Comment 8 Chris Dumez 2017-04-28 13:39:14 PDT
(In reply to Alex Christensen from comment #7)
> Comment on attachment 308553 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308553&action=review
> 
> > Source/WebCore/platform/URLParser.cpp:2819
> > +            auto name = formURLDecode(bytes.toString().replace('+', 0x20));
> 
> We should make a StringView.replace that returns a String.  This does an
> unnecessary copy.  Maybe in a followup.

Ok, I can take a look in a follow-up
Comment 9 WebKit Commit Bot 2017-04-28 13:43:29 PDT
Comment on attachment 308553 [details]
Patch

Clearing flags on attachment: 308553

Committed r215940: <http://trac.webkit.org/changeset/215940>
Comment 10 WebKit Commit Bot 2017-04-28 13:43:31 PDT
All reviewed patches have been landed.  Closing bug.