Bug 171345

Summary: URLSearchParams should be reflective
Product: WebKit Reporter: Adam Duren <adamduren>
Component: JavaScriptCoreAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, buildbot, cdumez, commit-queue
Priority: P2    
Version: Safari 10   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews124 for ios-simulator-wk2 none

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.