Bug 171345 – URLSearchParams should be reflective

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Adam Duren]

Overview:

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

Steps to Reproduce: 

```
var testParam = '[phone]';
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 '[phone]'

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 '[phone]'

[Chris Dumez]

Test case:
- http://jsbin.com/wipeqozazo/1/edit?html,output

Passes in Firefox and Chrome, fails in STP.

[Chris Dumez]

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().

[Chris Dumez]

Created attachment 308553 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Chris Dumez]

(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.

[Alex Christensen]

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.

[Chris Dumez]

(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

[WebKit Commit Bot]

Comment on attachment 308553 [details]
Patch

Clearing flags on attachment: 308553

Committed r215940: <http://trac.webkit.org/changeset/215940>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.