Reviewed by Darin Adler.
authorap@webkit.org <ap@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 25 Jan 2009 11:59:14 +0000 (11:59 +0000)
committerap@webkit.org <ap@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 25 Jan 2009 11:59:14 +0000 (11:59 +0000)
        <rdar://problem/5954398> REGRESSION: 1.1% PLT regression from 33577 and 33578 (encoding fixes)

WebCore:
        Changed single argument KURL constructors back to always expect an already encoded string,
        eliminating extra conversions.

        This is a rather unstable situation, as it is often unclear whether a given string is safe
        to convert to KURL without resolving. I think that going forward, the solution is to try to
        keep encoded URLs as KURL instances, and not as strings.

        * platform/KURL.h: Updated comments.

        * platform/KURL.cpp:
        (WebCore::KURL::KURL): In debug builds, verify that the passed string is ASCII-only. The
        intention is to verify that it is already parsed and encoded by KURL or equivalent code, but
        since encoding is scheme-dependent, such a verification would be quite complicated.
        Don't encode the string as UTF-8, as it supposed to be ASCII-only.
        Removed a hack that made strings beginning with "/" turn into "file:" URLs. I didn't find
        any reason for it to exist, but I saw several cases where this code path was taken
        inadvertently (see examples in LayoutTests/ChangeLog).
        (WebCore::KURL::setProtocol): Using a user-provided string without validation or encoding
        is clearly wrong here (e.g., the "protocol" can be set to a full URL, effectively replacing
        the old one), and an already encoded string is expected by parse().
        In debug builds, non-ASCII input will make an assertion in parse() fail. Added a FIXME.
        (WebCore::KURL::setHost): Ditto.
        (WebCore::KURL::setPort): Ditto.
        (WebCore::KURL::setHostAndPort): Ditto.
        (WebCore::KURL::setUser): Ditto.
        (WebCore::KURL::setPass): Ditto.
        (WebCore::KURL::setRef): Ditto.
        (WebCore::KURL::setQuery): Ditto.
        (WebCore::KURL::setPath): Ditto.  Note that problems described in bug 23500 mask some of
        the problems in release builds currently, as the incorrectly parsed URL is ignored.
        (WebCore::KURL::parse): Verify that the passed string is already encoded.

        * html/HTMLLinkElement.cpp:
        (WebCore::HTMLLinkElement::parseMappedAttribute):
        (WebCore::HTMLLinkElement::process):
        * html/HTMLLinkElement.h:
        Changed to avoid using invalid URLs (this was causing problems on DNS prefetch tests, see
        LayoutTests/ChangeLog).

        * loader/FrameLoader.cpp:
        (WebCore::FrameLoader::init): Create an empty KURL without indirection for a small speedup.
        (WebCore::FrameLoader::requestFrame): Resolve and encode javascript URLs properly, now that
        String to KURL conversion requires the string to be already encoded.

        * page/DOMWindow.cpp: (WebCore::DOMWindow::postMessage): Resolve and encode the origin.
        HTML5 seems a little unclear on how this should work (it talks about "either parsing it as
        a URL, or resolving it", and then somehow compares unaltered targetOrigin string to a
        security origin object), so I just made the code as close to what we already had as possible.

LayoutTests:
        * http/tests/misc/dns-prefetch-control-expected.txt:
        * http/tests/misc/dns-prefetch-control.html:
        Google documentation for DNS Prefetch makes use of net-path relative URLs (//server-name),
        explaining that scheme is not necessary. This is of course true, but this test uses data:
        subframes, and data: is a non-hierachical scheme, so resolving such URLs fails, resulting
        in a KURL object that is not valid. WebKit used to ignore this, and tried to create a URL
        from this string again, now with a single argument KURL constructor, which resulted in a
        valid file: URL, which was successfully used! Both issues have been corrected in WebCore,
        so I had to change the test to no longer use relative net-path URLs.

        * http/tests/security/postMessage/invalid-origin-throws-exception-expected.txt:
        * http/tests/security/postMessage/invalid-origin-throws-exception.html:
        URLs that start with "/" are no longer converted to "file:" ones, so the results now
        match Firefox.

        * http/tests/uri/resolve-encoding-relative-expected.txt: Added.
        * http/tests/uri/resolve-encoding-relative.html: Added.
        Added a test to cover some cases of relative URL resolving that were not covered before.
        Expected results are taken from Firefox 3, and WebKit doesn't match in how fragments are
        encoded (we use document encoding, while Firefox uses UTF-8). Since fragments are not
        sent in HTTP requests, this is not too dangerous, but the Firefox behavior looks more
        consistent.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@40221 268f45cc-cd09-0410-ab3c-d52691b4dbfc

14 files changed:
LayoutTests/ChangeLog
LayoutTests/http/tests/misc/dns-prefetch-control-expected.txt
LayoutTests/http/tests/misc/dns-prefetch-control.html
LayoutTests/http/tests/security/postMessage/invalid-origin-throws-exception-expected.txt
LayoutTests/http/tests/security/postMessage/invalid-origin-throws-exception.html
LayoutTests/http/tests/uri/resolve-encoding-relative-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/uri/resolve-encoding-relative.html [new file with mode: 0644]
WebCore/ChangeLog
WebCore/html/HTMLLinkElement.cpp
WebCore/html/HTMLLinkElement.h
WebCore/loader/FrameLoader.cpp
WebCore/page/DOMWindow.cpp
WebCore/platform/KURL.cpp
WebCore/platform/KURL.h

index f8b49bf..877c343 100644 (file)
@@ -1,3 +1,32 @@
+2009-01-24  Alexey Proskuryakov  <ap@webkit.org>
+
+        Reviewed by Darin Adler.
+
+        <rdar://problem/5954398> REGRESSION: 1.1% PLT regression from 33577 and 33578 (encoding fixes)
+
+        * http/tests/misc/dns-prefetch-control-expected.txt:
+        * http/tests/misc/dns-prefetch-control.html:
+        Google documentation for DNS Prefetch makes use of net-path relative URLs (//server-name),
+        explaining that scheme is not necessary. This is of course true, but this test uses data:
+        subframes, and data: is a non-hierachical scheme, so resolving such URLs fails, resulting
+        in a KURL object that is not valid. WebKit used to ignore this, and tried to create a URL
+        from this string again, now with a single argument KURL constructor, which resulted in a
+        valid file: URL, which was successfully used! Both issues have been corrected in WebCore,
+        so I had to change the test to no longer use relative net-path URLs.
+
+        * http/tests/security/postMessage/invalid-origin-throws-exception-expected.txt:
+        * http/tests/security/postMessage/invalid-origin-throws-exception.html:
+        URLs that start with "/" are no longer converted to "file:" ones, so the results now
+        match Firefox.
+
+        * http/tests/uri/resolve-encoding-relative-expected.txt: Added.
+        * http/tests/uri/resolve-encoding-relative.html: Added.
+        Added a test to cover some cases of relative URL resolving that were not covered before.
+        Expected results are taken from Firefox 3, and WebKit doesn't match in how fragments are
+        encoded (we use document encoding, while Firefox uses UTF-8). Since fragments are not
+        sent in HTTP requests, this is not too dangerous, but the Firefox behavior looks more
+        consistent.
+
 2009-01-24  Eric Carlson  <eric.carlson@apple.com>
 
         <video> controls visibility test needs to dump render tree as text because
index f16afa8..f5ab8b1 100644 (file)
@@ -2,7 +2,7 @@ This is a test of DNS prefetch control. It's considered a pass if it doesn't cra
 
 The following frames contain links that are expected to trigger a DNS prefetch.
 
-              
+            
 The following frames contain links that are not expected to cause a DNS prefetch.
 
           
index 7f4791c..b24b477 100644 (file)
@@ -30,7 +30,7 @@
         var contents = "";
         if (dnsPrefetchControl)
             contents += "<meta http-equiv='x-dns-prefetch-control' content='" + dnsPrefetchControl + "'>";
-        contents += "<link rel='dns-prefetch' href='//" + host + "'>" + host;
+        contents += "<link rel='dns-prefetch' href='http://" + host + "'>" + host;
         emitFrameWithContents(contents);
     }
 </script>
@@ -46,7 +46,6 @@ a manual test of DNS prefetch using a networking monitoring tool.</p>
   <script>emitFrameForScheme("http:")</script>
   <script>emitFrameForScheme("https:")</script>
   <script>emitFrameForScheme("ftp:")</script>
-  <script>emitFrameForScheme("")</script>
   <iframe src="resources/dns-prefetch-control.php"></iframe>
   <iframe src="resources/dns-prefetch-control.php?value=on"></iframe>
   <iframe src="https://127.0.0.1:8443/misc/resources/dns-prefetch-control.php?value=on"></iframe>
index 818e572..c3000c4 100644 (file)
@@ -2,17 +2,13 @@ CONSOLE MESSAGE: line 0: Unable to post message to asdf://. Recipient has origin
 
 CONSOLE MESSAGE: line 0: Unable to post message to http://. Recipient has origin http://localhost:8000.
 
-CONSOLE MESSAGE: line 0: Unable to post message to localhost:8000.
-
-CONSOLE MESSAGE: line 0: Unable to post message to localhost:8000.
-
 window.location.href = http://127.0.0.1:8000/security/postMessage/invalid-origin-throws-exception.html
 
 waiting...
 Encountered exception Error: SYNTAX_ERR: DOM Exception 12 while posting message to ''.
 Encountered exception Error: SYNTAX_ERR: DOM Exception 12 while posting message to 'asdf'.
+Encountered exception Error: SYNTAX_ERR: DOM Exception 12 while posting message to '/tmp/foo'.
+Encountered exception Error: SYNTAX_ERR: DOM Exception 12 while posting message to '//localhost'.
 Posted message to 'asdf:' without any exceptions.
 Posted message to 'http:' without any exceptions.
-Posted message to '/tmp/foo' without any exceptions.
-Posted message to '//localhost' without any exceptions.
 Received message: data="Received message: data="done" origin="http://127.0.0.1:8000"" origin="http://localhost:8000"
index 3594dc2..39fb203 100644 (file)
@@ -26,15 +26,12 @@ function test() {
     // Non-URLs should fail with a syntax error.
     tryPostMessage("");
     tryPostMessage("asdf");
+    tryPostMessage("/tmp/foo");
+    tryPostMessage("//localhost");
 
     // URLs without an origin should fail without generating any errors.
     tryPostMessage("asdf:");
     tryPostMessage("http:");
-
-    // WebKit converts URLs that start with / to file:// URLs. They don't match the target frame, so they fail silently.
-    tryPostMessage("/tmp/foo");
-    tryPostMessage("//localhost");
-
     
     win.postMessage('done', '*');
 }
diff --git a/LayoutTests/http/tests/uri/resolve-encoding-relative-expected.txt b/LayoutTests/http/tests/uri/resolve-encoding-relative-expected.txt
new file mode 100644 (file)
index 0000000..95c1d57
--- /dev/null
@@ -0,0 +1,9 @@
+Test how non-ASCII characters are encoded in relative URLs.
+
+1 2 3 4 5
+1. PASS
+2. FAIL: http://127.0.0.1:8000/uri/resolve-encoding-relative.html#%F4%F0%E0%E3%EC%E5%ED%F2
+3. PASS
+4. PASS
+5. FAIL: http://127.0.0.1:8000/%D0%BF%D1%83%D1%82%D1%8C?%E7%E0%EF%F0%EE%F1#%F4%F0%E0%E3%EC%E5%ED%F2
+
diff --git a/LayoutTests/http/tests/uri/resolve-encoding-relative.html b/LayoutTests/http/tests/uri/resolve-encoding-relative.html
new file mode 100644 (file)
index 0000000..eb90857
--- /dev/null
@@ -0,0 +1,26 @@
+<meta charset=windows-1251>
+<body>
+<p>Test how non-ASCII characters are encoded in relative URLs.</p>
+<a href="?çàïðîñ">1</a>
+<a href="#ôðàãìåíò">2</a>
+<a href="ïóòü">3</a>
+<a href="/ïóòü">4</a>
+<a href="/ïóòü?çàïðîñ#ôðàãìåíò">5</a>
+<pre id=result></pre>
+<script>
+if (window.layoutTestController)
+    layoutTestController.dumpAsText();
+
+function log(message)
+{
+    document.getElementById("result").innerHTML += message + "\n";
+}
+
+// Everything except for query (search) part is encoded as UTF-8 in Firefox.
+log("1. " + (document.getElementsByTagName("a")[0].href.match(/%E7%E0%EF%F0%EE%F1$/) ? "PASS" : "FAIL: " + document.getElementsByTagName("a")[0].href));
+log("2. " + (document.getElementsByTagName("a")[1].href.match(/#%D1%84%D1%80%D0%B0%D0%B3%D0%BC%D0%B5%D0%BD%D1%82$/) ? "PASS" : "FAIL: " + document.getElementsByTagName("a")[1].href));
+log("3. " + (document.getElementsByTagName("a")[2].href.match(/%D0%BF%D1%83%D1%82%D1%8C$/) ? "PASS" : "FAIL: " + document.getElementsByTagName("a")[2].href));
+log("4. " + (document.getElementsByTagName("a")[3].href.match(/%D0%BF%D1%83%D1%82%D1%8C$/) ? "PASS" : "FAIL: " + document.getElementsByTagName("a")[3].href));
+log("5. " + (document.getElementsByTagName("a")[4].href.match(/%D0%BF%D1%83%D1%82%D1%8C\?%E7%E0%EF%F0%EE%F1#%D1%84%D1%80%D0%B0%D0%B3%D0%BC%D0%B5%D0%BD%D1%82$/) ? "PASS" : "FAIL: " + document.getElementsByTagName("a")[4].href));
+</script>
+</body>
index 2b5e5b4..0530a68 100644 (file)
@@ -1,3 +1,58 @@
+2009-01-24  Alexey Proskuryakov  <ap@webkit.org>
+
+        Reviewed by Darin Adler.
+
+        <rdar://problem/5954398> REGRESSION: 1.1% PLT regression from 33577 and 33578 (encoding fixes)
+
+        Changed single argument KURL constructors back to always expect an already encoded string,
+        eliminating extra conversions.
+
+        This is a rather unstable situation, as it is often unclear whether a given string is safe
+        to convert to KURL without resolving. I think that going forward, the solution is to try to
+        keep encoded URLs as KURL instances, and not as strings.
+
+        * platform/KURL.h: Updated comments.
+
+        * platform/KURL.cpp:
+        (WebCore::KURL::KURL): In debug builds, verify that the passed string is ASCII-only. The
+        intention is to verify that it is already parsed and encoded by KURL or equivalent code, but
+        since encoding is scheme-dependent, such a verification would be quite complicated.
+        Don't encode the string as UTF-8, as it supposed to be ASCII-only.
+        Removed a hack that made strings beginning with "/" turn into "file:" URLs. I didn't find
+        any reason for it to exist, but I saw several cases where this code path was taken
+        inadvertently (see examples in LayoutTests/ChangeLog).
+        (WebCore::KURL::setProtocol): Using a user-provided string without validation or encoding
+        is clearly wrong here (e.g., the "protocol" can be set to a full URL, effectively replacing
+        the old one), and an already encoded string is expected by parse().
+        In debug builds, non-ASCII input will make an assertion in parse() fail. Added a FIXME.
+        (WebCore::KURL::setHost): Ditto.
+        (WebCore::KURL::setPort): Ditto.
+        (WebCore::KURL::setHostAndPort): Ditto.
+        (WebCore::KURL::setUser): Ditto.
+        (WebCore::KURL::setPass): Ditto.
+        (WebCore::KURL::setRef): Ditto.
+        (WebCore::KURL::setQuery): Ditto.
+        (WebCore::KURL::setPath): Ditto.  Note that problems described in bug 23500 mask some of
+        the problems in release builds currently, as the incorrectly parsed URL is ignored. 
+        (WebCore::KURL::parse): Verify that the passed string is already encoded.
+
+        * html/HTMLLinkElement.cpp:
+        (WebCore::HTMLLinkElement::parseMappedAttribute):
+        (WebCore::HTMLLinkElement::process):
+        * html/HTMLLinkElement.h:
+        Changed to avoid using invalid URLs (this was causing problems on DNS prefetch tests, see
+        LayoutTests/ChangeLog).
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::init): Create an empty KURL without indirection for a small speedup.
+        (WebCore::FrameLoader::requestFrame): Resolve and encode javascript URLs properly, now that
+        String to KURL conversion requires the string to be already encoded.
+
+        * page/DOMWindow.cpp: (WebCore::DOMWindow::postMessage): Resolve and encode the origin.
+        HTML5 seems a little unclear on how this should work (it talks about "either parsing it as
+        a URL, or resolving it", and then somehow compares unaltered targetOrigin string to a
+        security origin object), so I just made the code as close to what we already had as possible.
+
 2009-01-24  Darin Adler  <darin@apple.com>
 
         Try to fix Qt build.
index 50cdef1..28bf2fa 100644 (file)
@@ -114,7 +114,7 @@ void HTMLLinkElement::parseMappedAttribute(MappedAttribute *attr)
         tokenizeRelAttribute(attr->value(), m_isStyleSheet, m_alternate, m_isIcon, m_isDNSPrefetch);
         process();
     } else if (attr->name() == hrefAttr) {
-        m_url = document()->completeURL(parseURL(attr->value())).string();
+        m_url = document()->completeURL(parseURL(attr->value()));
         process();
     } else if (attr->name() == typeAttr) {
         m_type = attr->value();
@@ -173,15 +173,15 @@ void HTMLLinkElement::process()
 
     // IE extension: location of small icon for locationbar / bookmarks
     // We'll record this URL per document, even if we later only use it in top level frames
-    if (m_isIcon && !m_url.isEmpty())
-        document()->setIconURL(m_url, type);
+    if (m_isIcon && m_url.isValid() && !m_url.isEmpty())
+        document()->setIconURL(m_url.string(), type);
 
-    if (m_isDNSPrefetch && !m_url.isEmpty())
-        prefetchDNS(KURL(m_url).host());
+    if (m_isDNSPrefetch && m_url.isValid() && !m_url.isEmpty())
+        prefetchDNS(m_url.host());
 
     // Stylesheet
     // This was buggy and would incorrectly match <link rel="alternate">, which has a different specified meaning. -dwh
-    if (m_disabledState != 2 && m_isStyleSheet && document()->frame()) {
+    if (m_disabledState != 2 && m_isStyleSheet && document()->frame() && m_url.isValid()) {
         // no need to load style sheets which aren't for the screen output
         // ### there may be in some situations e.g. for an editor or script to manipulate
         // also, don't load style sheets for standalone documents
@@ -206,7 +206,7 @@ void HTMLLinkElement::process()
                 m_cachedSheet->removeClient(this);
             }
             m_loading = true;
-            m_cachedSheet = document()->docLoader()->requestCSSStyleSheet(m_url, chset);
+            m_cachedSheet = document()->docLoader()->requestCSSStyleSheet(m_url.string(), chset);
             if (m_cachedSheet)
                 m_cachedSheet->addClient(this);
             else if (!isAlternate()) { // request may have been denied if stylesheet is local and document is remote.
index b41fc9e..aacac92 100644 (file)
@@ -102,7 +102,7 @@ public:
 protected:
     CachedResourceHandle<CachedCSSStyleSheet> m_cachedSheet;
     RefPtr<CSSStyleSheet> m_sheet;
-    String m_url;
+    KURL m_url;
     String m_type;
     String m_media;
     int m_disabledState; // 0=unset(default), 1=enabled via script, 2=disabled
index 7460ab6..3569f1d 100644 (file)
@@ -283,7 +283,7 @@ void FrameLoader::init()
     // this somewhat odd set of steps is needed to give the frame an initial empty document
     m_isDisplayingInitialEmptyDocument = false;
     m_creatingInitialEmptyDocument = true;
-    setPolicyDocumentLoader(m_client->createDocumentLoader(ResourceRequest(String("")), SubstituteData()).get());
+    setPolicyDocumentLoader(m_client->createDocumentLoader(ResourceRequest(KURL("")), SubstituteData()).get());
     setProvisionalDocumentLoader(m_policyDocumentLoader.get());
     setState(FrameStateProvisional);
     m_provisionalDocumentLoader->setResponse(ResourceResponse(KURL(), "text/html", 0, String(), String()));
@@ -427,7 +427,7 @@ bool FrameLoader::requestFrame(HTMLFrameOwnerElement* ownerElement, const String
     KURL scriptURL;
     KURL url;
     if (protocolIs(urlString, "javascript")) {
-        scriptURL = KURL(urlString);
+        scriptURL = completeURL(urlString); // completeURL() encodes the URL.
         url = blankURL();
     } else
         url = completeURL(urlString);
index 088e62b..11cf1b6 100644 (file)
@@ -363,7 +363,7 @@ void DOMWindow::postMessage(const String& message, MessagePort* messagePort, con
     // to generate the SYNTAX_ERR exception correctly.
     RefPtr<SecurityOrigin> target;
     if (targetOrigin != "*") {
-        target = SecurityOrigin::create(KURL(targetOrigin));
+        target = SecurityOrigin::create(KURL(KURL(), targetOrigin, UTF8Encoding()));
         if (target->isEmpty()) {
             ec = SYNTAX_ERR;
             return;
index 620cbeb..3498f6a 100644 (file)
@@ -267,6 +267,21 @@ static int findFirstOf(const UChar* s, int sLen, int startPos, const char* toFin
     return -1;
 }
 
+#ifndef NDEBUG
+static void checkEncodedString(const String& url)
+{
+    for (unsigned i = 0; i < url.length(); ++i)
+        ASSERT(!(url[i] & ~0x7F));
+
+    // FIXME: The first character should be checked with isSchemeFirstChar(), but some layout tests currently trigger this assertion.
+    ASSERT(!url.length() || url[0] != '/');
+}
+#else
+static inline void checkEncodedString(const String&)
+{
+}
+#endif
+
 inline bool KURL::protocolIs(const String& string, const char* protocol)
 {
     return WebCore::protocolIs(string, protocol);
@@ -289,39 +304,14 @@ void KURL::invalidate()
 
 KURL::KURL(const char* url)
 {
-    if (!url || url[0] != '/') {
-        parse(url, 0);
-        return;
-    }
-
-    size_t urlLength = strlen(url) + 1;
-    CharBuffer buffer(urlLength + 5); // 5 for "file:".
-    buffer[0] = 'f';
-    buffer[1] = 'i';
-    buffer[2] = 'l';
-    buffer[3] = 'e';
-    buffer[4] = ':';
-    memcpy(&buffer[5], url, urlLength);
-    parse(buffer.data(), 0);
+    parse(url, 0);
 }
 
 KURL::KURL(const String& url)
 {
-    if (url[0] != '/') {
-        parse(url.utf8().data(), &url);
-        return;
-    }
+    checkEncodedString(url);
 
-    CharBuffer buffer(url.length() + 6); // 5 for "file:", 1 for terminator.
-    buffer[0] = 'f';
-    buffer[1] = 'i';
-    buffer[2] = 'l';
-    buffer[3] = 'e';
-    buffer[4] = ':';
-    copyASCII(url.characters(), url.length(), &buffer[5]);
-    buffer[url.length() + 5] = '\0'; // Need null terminator.
-
-    parse(buffer.data(), 0);
+    parse(url);
 }
 
 KURL::KURL(const KURL& base, const String& relative)
@@ -657,6 +647,9 @@ String KURL::path() const
 
 void KURL::setProtocol(const String& s)
 {
+    // FIXME: Non-ASCII characters must be encoded and escaped to match parse() expectations,
+    // and to avoid changing more than just the protocol.
+
     if (!m_isValid) {
         parse(s + ":" + m_string);
         return;
@@ -670,6 +663,9 @@ void KURL::setHost(const String& s)
     if (!m_isValid)
         return;
 
+    // FIXME: Non-ASCII characters must be encoded and escaped to match parse() expectations,
+    // and to avoid changing more than just the host.
+
     bool slashSlashNeeded = m_userStart == m_schemeEnd + 1;
 
     parse(m_string.left(hostStart()) + (slashSlashNeeded ? "//" : "") + s + m_string.substring(m_hostEnd));
@@ -680,6 +676,9 @@ void KURL::setPort(unsigned short i)
     if (!m_isValid)
         return;
 
+    // FIXME: Non-ASCII characters must be encoded and escaped to match parse() expectations,
+    // and to avoid changing more than just the port.
+
     bool colonNeeded = m_portEnd == m_hostEnd;
     int portStart = (colonNeeded ? m_hostEnd : m_hostEnd + 1);
 
@@ -691,6 +690,9 @@ void KURL::setHostAndPort(const String& hostAndPort)
     if (!m_isValid)
         return;
 
+    // FIXME: Non-ASCII characters must be encoded and escaped to match parse() expectations,
+    // and to avoid changing more than just host and port.
+
     bool slashSlashNeeded = m_userStart == m_schemeEnd + 1;
 
     parse(m_string.left(hostStart()) + (slashSlashNeeded ? "//" : "") + hostAndPort + m_string.substring(m_portEnd));
@@ -701,6 +703,8 @@ void KURL::setUser(const String& user)
     if (!m_isValid)
         return;
 
+    // FIXME: Non-ASCII characters must be encoded and escaped to match parse() expectations,
+    // and to avoid changing more than just the user login.
     String u;
     int end = m_userEnd;
     if (!user.isEmpty()) {
@@ -723,6 +727,8 @@ void KURL::setPass(const String& password)
     if (!m_isValid)
         return;
 
+    // FIXME: Non-ASCII characters must be encoded and escaped to match parse() expectations,
+    // and to avoid changing more than just the user password.
     String p;
     int end = m_passwordEnd;
     if (!password.isEmpty()) {
@@ -744,6 +750,8 @@ void KURL::setRef(const String& s)
 {
     if (!m_isValid)
         return;
+
+    // FIXME: Non-ASCII characters must be encoded and escaped to match parse() expectations.
     parse(m_string.left(m_queryEnd) + (s.isNull() ? "" : "#" + s));
 }
 
@@ -759,6 +767,9 @@ void KURL::setQuery(const String& query)
     if (!m_isValid)
         return;
 
+    // FIXME: '#' and non-ASCII characters must be encoded and escaped.
+    // Usually, the query is encoded using document encoding, not UTF-8, but we don't have
+    // access to the document in this function.
     if ((query.isEmpty() || query[0] != '?') && !query.isNull())
         parse(m_string.left(m_pathEnd) + "?" + query + m_string.substring(m_queryEnd));
     else
@@ -771,6 +782,8 @@ void KURL::setPath(const String& s)
     if (!m_isValid)
         return;
 
+    // FIXME: encodeWithURLEscapeSequences does not correctly escape '#' and '?', so fragment and query parts
+    // may be inadvertently affected.
     parse(m_string.left(m_portEnd) + encodeWithURLEscapeSequences(s) + m_string.substring(m_pathEnd));
 }
 
@@ -982,8 +995,8 @@ static inline bool matchLetter(char c, char lowercaseLetter)
 
 void KURL::parse(const String& string)
 {
-    // FIXME: What should this do for non-ASCII URLs?
-    // Currently it throws away the high bytes of the characters in the string in that case, matching createCFURL().
+    checkEncodedString(string);
+
     CharBuffer buffer(string.length() + 1);
     copyASCII(string.characters(), string.length(), buffer.data());
     buffer[string.length()] = '\0';
index 0b6cb29..7f6b102 100644 (file)
@@ -66,17 +66,8 @@ public:
     KURL() { invalidate(); }
 
     // The argument is an absolute URL string. The string is assumed to be
-    // already encoded.
-    // FIXME: This constructor has a special case for strings that start with
-    // "/", prepending "file://" to such strings; it would be good to get
-    // rid of that special case.
+    // an already encoded (ASCII-only) valid absolute URL.
     explicit KURL(const char*);
-
-    // The argument is an absolute URL string. The string is assumed to be
-    // already encoded.
-    // FIXME: This constructor has a special case for strings that start with
-    // "/", prepending "file://" to such strings; it would be good to get
-    // rid of that special case.
     explicit KURL(const String&);
 
     // Resolves the relative URL with the given base URL. If provided, the