Changeset 40221 in webkit


Ignore:
Timestamp:
Jan 25, 2009, 3:59:14 AM (16 years ago)
Author:
ap@webkit.org
Message:

Reviewed by Darin Adler.

<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.
Location:
trunk
Files:
2 added
12 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r40219 r40221  
     12009-01-24  Alexey Proskuryakov  <ap@webkit.org>
     2
     3        Reviewed by Darin Adler.
     4
     5        <rdar://problem/5954398> REGRESSION: 1.1% PLT regression from 33577 and 33578 (encoding fixes)
     6
     7        * http/tests/misc/dns-prefetch-control-expected.txt:
     8        * http/tests/misc/dns-prefetch-control.html:
     9        Google documentation for DNS Prefetch makes use of net-path relative URLs (//server-name),
     10        explaining that scheme is not necessary. This is of course true, but this test uses data:
     11        subframes, and data: is a non-hierachical scheme, so resolving such URLs fails, resulting
     12        in a KURL object that is not valid. WebKit used to ignore this, and tried to create a URL
     13        from this string again, now with a single argument KURL constructor, which resulted in a
     14        valid file: URL, which was successfully used! Both issues have been corrected in WebCore,
     15        so I had to change the test to no longer use relative net-path URLs.
     16
     17        * http/tests/security/postMessage/invalid-origin-throws-exception-expected.txt:
     18        * http/tests/security/postMessage/invalid-origin-throws-exception.html:
     19        URLs that start with "/" are no longer converted to "file:" ones, so the results now
     20        match Firefox.
     21
     22        * http/tests/uri/resolve-encoding-relative-expected.txt: Added.
     23        * http/tests/uri/resolve-encoding-relative.html: Added.
     24        Added a test to cover some cases of relative URL resolving that were not covered before.
     25        Expected results are taken from Firefox 3, and WebKit doesn't match in how fragments are
     26        encoded (we use document encoding, while Firefox uses UTF-8). Since fragments are not
     27        sent in HTTP requests, this is not too dangerous, but the Firefox behavior looks more
     28        consistent.
     29
    1302009-01-24  Eric Carlson  <eric.carlson@apple.com>
    231

  • trunk/LayoutTests/http/tests/misc/dns-prefetch-control-expected.txt

    r36654 r40221  
    33The following frames contain links that are expected to trigger a DNS prefetch.
    44
    5              
     5           
    66The following frames contain links that are not expected to cause a DNS prefetch.
    77

  • trunk/LayoutTests/http/tests/misc/dns-prefetch-control.html

    r36650 r40221  
    3131        if (dnsPrefetchControl)
    3232            contents += "<meta http-equiv='x-dns-prefetch-control' content='" + dnsPrefetchControl + "'>";
    33         contents += "<link rel='dns-prefetch' href='//" + host + "'>" + host;
     33        contents += "<link rel='dns-prefetch' href='http://" + host + "'>" + host;
    3434        emitFrameWithContents(contents);
    3535    }
    … …  
    4747  <script>emitFrameForScheme("https:")</script>
    4848  <script>emitFrameForScheme("ftp:")</script>
    49   <script>emitFrameForScheme("")</script>
    5049  <iframe src="resources/dns-prefetch-control.php"></iframe>
    5150  <iframe src="resources/dns-prefetch-control.php?value=on"></iframe>

  • trunk/LayoutTests/http/tests/security/postMessage/invalid-origin-throws-exception-expected.txt

    r32922 r40221  
    22
    33CONSOLE MESSAGE: line 0: Unable to post message to http://. Recipient has origin http://localhost:8000.
    4 
    5 CONSOLE MESSAGE: line 0: Unable to post message to localhost:8000.
    6 
    7 CONSOLE MESSAGE: line 0: Unable to post message to localhost:8000.
    84
    95window.location.href = http://127.0.0.1:8000/security/postMessage/invalid-origin-throws-exception.html
    … …  
    128Encountered exception Error: SYNTAX_ERR: DOM Exception 12 while posting message to ''.
    139Encountered exception Error: SYNTAX_ERR: DOM Exception 12 while posting message to 'asdf'.
     10Encountered exception Error: SYNTAX_ERR: DOM Exception 12 while posting message to '/tmp/foo'.
     11Encountered exception Error: SYNTAX_ERR: DOM Exception 12 while posting message to '//localhost'.
    1412Posted message to 'asdf:' without any exceptions.
    1513Posted message to 'http:' without any exceptions.
    16 Posted message to '/tmp/foo' without any exceptions.
    17 Posted message to '//localhost' without any exceptions.
    1814Received message: data="Received message: data="done" origin="http://127.0.0.1:8000"" origin="http://localhost:8000"

  • trunk/LayoutTests/http/tests/security/postMessage/invalid-origin-throws-exception.html

    r33006 r40221  
    2727    tryPostMessage("");
    2828    tryPostMessage("asdf");
     29    tryPostMessage("/tmp/foo");
     30    tryPostMessage("//localhost");
    2931
    3032    // URLs without an origin should fail without generating any errors.
    3133    tryPostMessage("asdf:");
    3234    tryPostMessage("http:");
    33 
    34     // WebKit converts URLs that start with / to file:// URLs. They don't match the target frame, so they fail silently.
    35     tryPostMessage("/tmp/foo");
    36     tryPostMessage("//localhost");
    37 
    3835   
    3936    win.postMessage('done', '*');

  • trunk/WebCore/ChangeLog

    r40216 r40221  
     12009-01-24  Alexey Proskuryakov  <ap@webkit.org>
     2
     3        Reviewed by Darin Adler.
     4
     5        <rdar://problem/5954398> REGRESSION: 1.1% PLT regression from 33577 and 33578 (encoding fixes)
     6
     7        Changed single argument KURL constructors back to always expect an already encoded string,
     8        eliminating extra conversions.
     9
     10        This is a rather unstable situation, as it is often unclear whether a given string is safe
     11        to convert to KURL without resolving. I think that going forward, the solution is to try to
     12        keep encoded URLs as KURL instances, and not as strings.
     13
     14        * platform/KURL.h: Updated comments.
     15
     16        * platform/KURL.cpp:
     17        (WebCore::KURL::KURL): In debug builds, verify that the passed string is ASCII-only. The
     18        intention is to verify that it is already parsed and encoded by KURL or equivalent code, but
     19        since encoding is scheme-dependent, such a verification would be quite complicated.
     20        Don't encode the string as UTF-8, as it supposed to be ASCII-only.
     21        Removed a hack that made strings beginning with "/" turn into "file:" URLs. I didn't find
     22        any reason for it to exist, but I saw several cases where this code path was taken
     23        inadvertently (see examples in LayoutTests/ChangeLog).
     24        (WebCore::KURL::setProtocol): Using a user-provided string without validation or encoding
     25        is clearly wrong here (e.g., the "protocol" can be set to a full URL, effectively replacing
     26        the old one), and an already encoded string is expected by parse().
     27        In debug builds, non-ASCII input will make an assertion in parse() fail. Added a FIXME.
     28        (WebCore::KURL::setHost): Ditto.
     29        (WebCore::KURL::setPort): Ditto.
     30        (WebCore::KURL::setHostAndPort): Ditto.
     31        (WebCore::KURL::setUser): Ditto.
     32        (WebCore::KURL::setPass): Ditto.
     33        (WebCore::KURL::setRef): Ditto.
     34        (WebCore::KURL::setQuery): Ditto.
     35        (WebCore::KURL::setPath): Ditto.  Note that problems described in bug 23500 mask some of
     36        the problems in release builds currently, as the incorrectly parsed URL is ignored.
     37        (WebCore::KURL::parse): Verify that the passed string is already encoded.
     38
     39        * html/HTMLLinkElement.cpp:
     40        (WebCore::HTMLLinkElement::parseMappedAttribute):
     41        (WebCore::HTMLLinkElement::process):
     42        * html/HTMLLinkElement.h:
     43        Changed to avoid using invalid URLs (this was causing problems on DNS prefetch tests, see
     44        LayoutTests/ChangeLog).
     45
     46        * loader/FrameLoader.cpp:
     47        (WebCore::FrameLoader::init): Create an empty KURL without indirection for a small speedup.
     48        (WebCore::FrameLoader::requestFrame): Resolve and encode javascript URLs properly, now that
     49        String to KURL conversion requires the string to be already encoded.
     50
     51        * page/DOMWindow.cpp: (WebCore::DOMWindow::postMessage): Resolve and encode the origin.
     52        HTML5 seems a little unclear on how this should work (it talks about "either parsing it as
     53        a URL, or resolving it", and then somehow compares unaltered targetOrigin string to a
     54        security origin object), so I just made the code as close to what we already had as possible.
     55
    1562009-01-24  Darin Adler  <darin@apple.com>
    257

  • trunk/WebCore/html/HTMLLinkElement.cpp

    r39904 r40221  
    115115        process();
    116116    } else if (attr->name() == hrefAttr) {
    117         m_url = document()->completeURL(parseURL(attr->value())).string();
     117        m_url = document()->completeURL(parseURL(attr->value()));
    118118        process();
    119119    } else if (attr->name() == typeAttr) {
    … …  
    174174    // IE extension: location of small icon for locationbar / bookmarks
    175175    // We'll record this URL per document, even if we later only use it in top level frames
    176     if (m_isIcon && !m_url.isEmpty())
    177         document()->setIconURL(m_url, type);
    178 
    179     if (m_isDNSPrefetch && !m_url.isEmpty())
    180         prefetchDNS(KURL(m_url).host());
     176    if (m_isIcon && m_url.isValid() && !m_url.isEmpty())
     177        document()->setIconURL(m_url.string(), type);
     178
     179    if (m_isDNSPrefetch && m_url.isValid() && !m_url.isEmpty())
     180        prefetchDNS(m_url.host());
    181181
    182182    // Stylesheet
    183183    // This was buggy and would incorrectly match <link rel="alternate">, which has a different specified meaning. -dwh
    184     if (m_disabledState != 2 && m_isStyleSheet && document()->frame()) {
     184    if (m_disabledState != 2 && m_isStyleSheet && document()->frame() && m_url.isValid()) {
    185185        // no need to load style sheets which aren't for the screen output
    186186        // ### there may be in some situations e.g. for an editor or script to manipulate
    … …  
    207207            }
    208208            m_loading = true;
    209             m_cachedSheet = document()->docLoader()->requestCSSStyleSheet(m_url, chset);
     209            m_cachedSheet = document()->docLoader()->requestCSSStyleSheet(m_url.string(), chset);
    210210            if (m_cachedSheet)
    211211                m_cachedSheet->addClient(this);

  • trunk/WebCore/html/HTMLLinkElement.h

    r39180 r40221  
    103103    CachedResourceHandle<CachedCSSStyleSheet> m_cachedSheet;
    104104    RefPtr<CSSStyleSheet> m_sheet;
    105     String m_url;
     105    KURL m_url;
    106106    String m_type;
    107107    String m_media;

  • trunk/WebCore/loader/FrameLoader.cpp

    r40210 r40221  
    284284    m_isDisplayingInitialEmptyDocument = false;
    285285    m_creatingInitialEmptyDocument = true;
    286     setPolicyDocumentLoader(m_client->createDocumentLoader(ResourceRequest(String("")), SubstituteData()).get());
     286    setPolicyDocumentLoader(m_client->createDocumentLoader(ResourceRequest(KURL("")), SubstituteData()).get());
    287287    setProvisionalDocumentLoader(m_policyDocumentLoader.get());
    288288    setState(FrameStateProvisional);
    … …  
    428428    KURL url;
    429429    if (protocolIs(urlString, "javascript")) {
    430         scriptURL = KURL(urlString);
     430        scriptURL = completeURL(urlString); // completeURL() encodes the URL.
    431431        url = blankURL();
    432432    } else

  • trunk/WebCore/page/DOMWindow.cpp

    r39818 r40221  
    364364    RefPtr<SecurityOrigin> target;
    365365    if (targetOrigin != "*") {
    366         target = SecurityOrigin::create(KURL(targetOrigin));
     366        target = SecurityOrigin::create(KURL(KURL(), targetOrigin, UTF8Encoding()));
    367367        if (target->isEmpty()) {
    368368            ec = SYNTAX_ERR;

  • trunk/WebCore/platform/KURL.cpp

    r40213 r40221  
    268268}
    269269
     270#ifndef NDEBUG
     271static void checkEncodedString(const String& url)
     272{
     273    for (unsigned i = 0; i < url.length(); ++i)
     274        ASSERT(!(url[i] & ~0x7F));
     275
     276    // FIXME: The first character should be checked with isSchemeFirstChar(), but some layout tests currently trigger this assertion.
     277    ASSERT(!url.length() || url[0] != '/');
     278}
     279#else
     280static inline void checkEncodedString(const String&)
     281{
     282}
     283#endif
     284
    270285inline bool KURL::protocolIs(const String& string, const char* protocol)
    271286{
    … …  
    290305KURL::KURL(const char* url)
    291306{
    292     if (!url || url[0] != '/') {
    293         parse(url, 0);
    294         return;
    295     }
    296 
    297     size_t urlLength = strlen(url) + 1;
    298     CharBuffer buffer(urlLength + 5); // 5 for "file:".
    299     buffer[0] = 'f';
    300     buffer[1] = 'i';
    301     buffer[2] = 'l';
    302     buffer[3] = 'e';
    303     buffer[4] = ':';
    304     memcpy(&buffer[5], url, urlLength);
    305     parse(buffer.data(), 0);
     307    parse(url, 0);
    306308}
    307309
    308310KURL::KURL(const String& url)
    309311{
    310     if (url[0] != '/') {
    311         parse(url.utf8().data(), &url);
    312         return;
    313     }
    314 
    315     CharBuffer buffer(url.length() + 6); // 5 for "file:", 1 for terminator.
    316     buffer[0] = 'f';
    317     buffer[1] = 'i';
    318     buffer[2] = 'l';
    319     buffer[3] = 'e';
    320     buffer[4] = ':';
    321     copyASCII(url.characters(), url.length(), &buffer[5]);
    322     buffer[url.length() + 5] = '\0'; // Need null terminator.
    323 
    324     parse(buffer.data(), 0);
     312    checkEncodedString(url);
     313
     314    parse(url);
    325315}
    326316
    … …  
    658648void KURL::setProtocol(const String& s)
    659649{
     650    // FIXME: Non-ASCII characters must be encoded and escaped to match parse() expectations,
     651    // and to avoid changing more than just the protocol.
     652
    660653    if (!m_isValid) {
    661654        parse(s + ":" + m_string);
    … …  
    671664        return;
    672665
     666    // FIXME: Non-ASCII characters must be encoded and escaped to match parse() expectations,
     667    // and to avoid changing more than just the host.
     668
    673669    bool slashSlashNeeded = m_userStart == m_schemeEnd + 1;
    674670
    … …  
    681677        return;
    682678
     679    // FIXME: Non-ASCII characters must be encoded and escaped to match parse() expectations,
     680    // and to avoid changing more than just the port.
     681
    683682    bool colonNeeded = m_portEnd == m_hostEnd;
    684683    int portStart = (colonNeeded ? m_hostEnd : m_hostEnd + 1);
    … …  
    692691        return;
    693692
     693    // FIXME: Non-ASCII characters must be encoded and escaped to match parse() expectations,
     694    // and to avoid changing more than just host and port.
     695
    694696    bool slashSlashNeeded = m_userStart == m_schemeEnd + 1;
    695697
    … …  
    702704        return;
    703705
     706    // FIXME: Non-ASCII characters must be encoded and escaped to match parse() expectations,
     707    // and to avoid changing more than just the user login.
    704708    String u;
    705709    int end = m_userEnd;
    … …  
    724728        return;
    725729
     730    // FIXME: Non-ASCII characters must be encoded and escaped to match parse() expectations,
     731    // and to avoid changing more than just the user password.
    726732    String p;
    727733    int end = m_passwordEnd;
    … …  
    745751    if (!m_isValid)
    746752        return;
     753
     754    // FIXME: Non-ASCII characters must be encoded and escaped to match parse() expectations.
    747755    parse(m_string.left(m_queryEnd) + (s.isNull() ? "" : "#" + s));
    748756}
    … …  
    760768        return;
    761769
     770    // FIXME: '#' and non-ASCII characters must be encoded and escaped.
     771    // Usually, the query is encoded using document encoding, not UTF-8, but we don't have
     772    // access to the document in this function.
    762773    if ((query.isEmpty() || query[0] != '?') && !query.isNull())
    763774        parse(m_string.left(m_pathEnd) + "?" + query + m_string.substring(m_queryEnd));
    … …  
    772783        return;
    773784
     785    // FIXME: encodeWithURLEscapeSequences does not correctly escape '#' and '?', so fragment and query parts
     786    // may be inadvertently affected.
    774787    parse(m_string.left(m_portEnd) + encodeWithURLEscapeSequences(s) + m_string.substring(m_pathEnd));
    775788}
    … …  
    983996void KURL::parse(const String& string)
    984997{
    985     // FIXME: What should this do for non-ASCII URLs?
    986     // Currently it throws away the high bytes of the characters in the string in that case, matching createCFURL().
     998    checkEncodedString(string);
     999
    9871000    CharBuffer buffer(string.length() + 1);
    9881001    copyASCII(string.characters(), string.length(), buffer.data());

  • trunk/WebCore/platform/KURL.h

    r40213 r40221  
    6767
    6868    // The argument is an absolute URL string. The string is assumed to be
    69     // already encoded.
    70     // FIXME: This constructor has a special case for strings that start with
    71     // "/", prepending "file://" to such strings; it would be good to get
    72     // rid of that special case.
     69    // an already encoded (ASCII-only) valid absolute URL.
    7370    explicit KURL(const char*);
    74 
    75     // The argument is an absolute URL string. The string is assumed to be
    76     // already encoded.
    77     // FIXME: This constructor has a special case for strings that start with
    78     // "/", prepending "file://" to such strings; it would be good to get
    79     // rid of that special case.
    8071    explicit KURL(const String&);
    8172
Note: See TracChangeset for help on using the changeset viewer.