Bug 174051

Summary: REGRESSION(r215096) Queries of URLs with non-special schemes should not percent-encode single quotes
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, jlewis3, ryanhaddad, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch thorton: review+

Description Alex Christensen 2017-06-30 16:08:00 PDT
REGRESSION(r215096) Queries of URLs with non-special schemes should not percent-encode single quotes
Comment 1 Alex Christensen 2017-06-30 16:09:35 PDT
Created attachment 314310 [details]
Patch
Comment 2 Tim Horton 2017-06-30 16:13:00 PDT
Comment on attachment 314310 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=174051

RADAR

> Tools/TestWebKitAPI/Tests/WebCore/URLParser.cpp:1
> -/*
> +/*

Best revert this change
Comment 3 Alex Christensen 2017-06-30 16:23:18 PDT
http://trac.webkit.org/r219024
Comment 4 Alex Christensen 2017-06-30 17:51:44 PDT
http://trac.webkit.org/r219030
Comment 5 Matt Lewis 2017-06-30 18:01:29 PDT
It looks like this also caused these test to fail:
http/tests/security/no-popup-from-sandbox-top.html
imported/w3c/web-platform-tests/url/url-setters.html

Would these be a rebaseline like fast/events/popup-blocked-from-unique-frame-via-window-open-named-sibling-frame.html ?


Build
https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r219025%20(2667)/results.html

http/tests/security/no-popup-from-sandbox-top.html diff:
--- /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/http/tests/security/no-popup-from-sandbox-top-expected.txt
+++ /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/http/tests/security/no-popup-from-sandbox-top-actual.txt
@@ -1,4 +1,4 @@
-CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to initiate navigation for frame with URL 'http://127.0.0.1:8000/security/no-popup-from-sandbox-top.html' from frame with URL 'data:text/html,       <script>       var win = window.open('about:blank', '_top');       alert(win ?%20%27FAIL%27%20:%20%27PASS%27);%20%20%20%20%20%20%20%3C/script%3E'. The frame attempting navigation of the top-level window is sandboxed, but the 'allow-top-navigation' flag is not set.
+CONSOLE MESSAGE: line 1: Unsafe JavaScript attempt to initiate navigation for frame with URL 'http://127.0.0.1:8000/security/no-popup-from-sandbox-top.html' from frame with URL 'data:text/html,       <script>       var win = window.open('about:blank', '_top');       alert(win ?%20'FAIL'%20:%20'PASS');%20%20%20%20%20%20%20%3C/script%3E'. The frame attempting navigation of the top-level window is sandboxed, but the 'allow-top-navigation' flag is not set.
 
 ALERT: PASS
 To run this test outside of DumpRenderTree, please disable your popup blocker!


imported/w3c/web-platform-tests/url/url-setters.html diff:
--- /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/url/url-setters-expected.txt
+++ /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/imported/w3c/web-platform-tests/url/url-setters-actual.txt
@@ -552,12 +552,12 @@
 PASS URL: Setting <https://example.net>.search = '' 
 PASS <a>: Setting <https://example.net>.search = '' 
 PASS <area>: Setting <https://example.net>.search = '' 
-FAIL URL: Setting <a:/>.search = '\0	
-\r !"#$%&'()*+,-./09:;<=>?@AZ[\]^_`az{|}~Éé' UTF-8 percent encoding with the query encode set. Tabs and newlines are removed. assert_equals: expected "a:/?%00%01%1F%20!%22%23$%&'()*+,-./09:;%3C=%3E?@AZ[\\]^_`az{|}~%7F%C2%80%C2%81%C3%89%C3%A9" but got "a:/?%00%01%1F%20!%22%23$%&%27()*+,-./09:;%3C=%3E?@AZ[\\]^_`az{|}~%7F%C2%80%C2%81%C3%89%C3%A9"
-FAIL <a>: Setting <a:/>.search = '\0	
-\r !"#$%&'()*+,-./09:;<=>?@AZ[\]^_`az{|}~Éé' UTF-8 percent encoding with the query encode set. Tabs and newlines are removed. assert_equals: expected "a:/?%00%01%1F%20!%22%23$%&'()*+,-./09:;%3C=%3E?@AZ[\\]^_`az{|}~%7F%C2%80%C2%81%C3%89%C3%A9" but got "a:/?%00%01%1F%20!%22%23$%&%27()*+,-./09:;%3C=%3E?@AZ[\\]^_`az{|}~%7F%C2%80%C2%81%C3%89%C3%A9"
-FAIL <area>: Setting <a:/>.search = '\0	
-\r !"#$%&'()*+,-./09:;<=>?@AZ[\]^_`az{|}~Éé' UTF-8 percent encoding with the query encode set. Tabs and newlines are removed. assert_equals: expected "a:/?%00%01%1F%20!%22%23$%&'()*+,-./09:;%3C=%3E?@AZ[\\]^_`az{|}~%7F%C2%80%C2%81%C3%89%C3%A9" but got "a:/?%00%01%1F%20!%22%23$%&%27()*+,-./09:;%3C=%3E?@AZ[\\]^_`az{|}~%7F%C2%80%C2%81%C3%89%C3%A9"
+PASS URL: Setting <a:/>.search = '\0	
+\r !"#$%&'()*+,-./09:;<=>?@AZ[\]^_`az{|}~Éé' UTF-8 percent encoding with the query encode set. Tabs and newlines are removed. 
+PASS <a>: Setting <a:/>.search = '\0	
+\r !"#$%&'()*+,-./09:;<=>?@AZ[\]^_`az{|}~Éé' UTF-8 percent encoding with the query encode set. Tabs and newlines are removed. 
+PASS <area>: Setting <a:/>.search = '\0	
+\r !"#$%&'()*+,-./09:;<=>?@AZ[\]^_`az{|}~Éé' UTF-8 percent encoding with the query encode set. Tabs and newlines are removed. 
 PASS URL: Setting <http://example.net>.search = '%c3%89té' Bytes already percent-encoded are left as-is 
 PASS <a>: Setting <http://example.net>.search = '%c3%89té' Bytes already percent-encoded are left as-is 
 PASS <area>: Setting <http://example.net>.search = '%c3%89té' Bytes already percent-encoded are left as-is
Comment 6 Alexey Proskuryakov 2017-07-02 02:06:32 PDT
What was the resolution of this? Are tests still failing?

I don't understand why the patch wasn't immediately rolled out if it broke tests.
Comment 7 Matt Lewis 2017-07-03 09:56:09 PDT
Reverted r219030 for reason:

This was a rebaseline of a test that was broken with revision r219024

Committed r219072: <http://trac.webkit.org/changeset/219072>
Comment 8 Matt Lewis 2017-07-03 09:57:30 PDT
Reverted r219024 for reason:

This patch cause 3 didferent test to fail.

Committed r219073: <http://trac.webkit.org/changeset/219073>
Comment 9 Alex Christensen 2017-07-03 10:18:02 PDT
http://trac.webkit.org/r219074
Comment 10 Alex Christensen 2017-07-03 10:21:25 PDT
The tests should've been rebaselined or the patch should've been rolled out immediately.

Re-landed in http://trac.webkit.org/r219076 after tests had been fixed.  Will watch bots.
Comment 11 Alex Christensen 2017-07-03 12:12:06 PDT
http://trac.webkit.org/r219086