WebCore:
authorlevin@chromium.org <levin@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 25 Feb 2009 18:06:20 +0000 (18:06 +0000)
committerlevin@chromium.org <levin@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 25 Feb 2009 18:06:20 +0000 (18:06 +0000)
2009-02-25  David Levin  <levin@chromium.org>

        Reviewed by Alexey Proskuryakov.

        Bug 24089: ThreadableLoader::loadResourceSynchronously should do callbacks like the async code.
        <https://bugs.webkit.org/show_bug.cgi?id=24089>

        Make threadable loader callbacks to happen during the sync load call.

        Changes the behavior of sync xhr for insecure redirects in two ways:
        + Sends an error event instead of an abort event (which is the same as async xhr's behavior).
        + Throws a network exception which is what other browsers do and what the spec
        says to do (http://www.w3.org/TR/XMLHttpRequest/).

        * loader/DocumentThreadableLoader.cpp:
        (WebCore::DocumentThreadableLoader::loadResourceSynchronously):
        * loader/DocumentThreadableLoader.h:
        * loader/ThreadableLoader.cpp:
        (WebCore::ThreadableLoader::loadResourceSynchronously):
        * loader/ThreadableLoader.h:
        * xml/XMLHttpRequest.cpp:
        (WebCore::XMLHttpRequest::XMLHttpRequest):
        (WebCore::XMLHttpRequest::loadRequestSynchronously):
        (WebCore::XMLHttpRequest::loadRequestAsynchronously):
        (WebCore::XMLHttpRequest::didFail):
        (WebCore::XMLHttpRequest::didFailRedirectCheck):
        * xml/XMLHttpRequest.h:

LayoutTests:

2009-02-25  David Levin  <levin@chromium.org>

        Reviewed by Alexey Proskuryakov.

        Bug 24089: ThreadableLoader::loadResourceSynchronously should do callbacks like the async code.
        <https://bugs.webkit.org/show_bug.cgi?id=24089>

        Modified the test to account for behavior change and output more information to better detect
        future changes in behavior.

        * http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect-expected.txt:
        * http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html:

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

LayoutTests/ChangeLog
LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect-expected.txt
LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html
WebCore/ChangeLog
WebCore/loader/DocumentThreadableLoader.cpp
WebCore/loader/DocumentThreadableLoader.h
WebCore/loader/ThreadableLoader.cpp
WebCore/loader/ThreadableLoader.h
WebCore/xml/XMLHttpRequest.cpp
WebCore/xml/XMLHttpRequest.h

index d804e63..7d5c757 100644 (file)
@@ -1,3 +1,16 @@
+2009-02-25  David Levin  <levin@chromium.org>
+
+        Reviewed by Alexey Proskuryakov.
+
+        Bug 24089: ThreadableLoader::loadResourceSynchronously should do callbacks like the async code.
+        <https://bugs.webkit.org/show_bug.cgi?id=24089>
+
+        Modified the test to account for behavior change and output more information to better detect
+        future changes in behavior.
+
+        * http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect-expected.txt:
+        * http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html:
+
 2009-02-24  Chris Marrin  <cmarrin@apple.com>
 
         Reviewed by David Hyatt.
index d84fa91..98c5512 100644 (file)
@@ -1,2 +1,8 @@
 This tests that unsafe redirects won't be allowed when making an XMLHttpRequest.
+Sync XHR started.
+readyState change 1
+Error event.
+Exception: Error: NETWORK_ERR: XMLHttpRequest Exception 101
+Async XHR started.
+readyState change 1
 SUCCESS: Error handler was called with readyState 1
index 33ca71c..afc8b56 100644 (file)
@@ -8,13 +8,31 @@ function didReadFile()
         layoutTestController.notifyDone();    
 }
 
+function log(message)
+{
+    var logElement = document.getElementById('log');
+    logElement.appendChild(document.createTextNode(message));
+    logElement.appendChild(document.createElement("p"));
+}
+
 function onReqreadystatechange()
 {
+    log('readyState change ' +  req.readyState);
     if (req.readyState == 4)
         didReadFile();
 }
 
-function onReqError() 
+function onReqAbort()
+{
+    log('Abort event.');
+}
+
+function onSyncReqError()
+{
+    log('Error event.');
+}
+
+function onReqError(e)
 {
     document.getElementById('result').innerHTML = 'SUCCESS: Error handler was called with readyState ' + req.readyState;
     if (window.layoutTestController)
@@ -28,23 +46,35 @@ function runTest() {
     }
     
     // First, make a sync request.
+    log ('Sync XHR started.');
     req = new XMLHttpRequest();
+    req.onreadystatechange = onReqreadystatechange;
+    req.onerror = onSyncReqError;
+    req.onabort = onReqAbort;
     req.open('GET', '/xmlhttprequest/resources/redirect.php?url=http://localhost:8080/xmlhttprequest/resources/forbidden.txt', false);
-    req.send(null);
+    try {
+        req.send(null);
+    } catch (e) {
+        log('Exception: ' + (e.description ? e.description : e));
+    }
+
     if (req.responseText != '') {
         didReadFile();
         return;
     }
-    
+
+    log ('Async XHR started.');
     req = new XMLHttpRequest();
     req.onreadystatechange = onReqreadystatechange;
     req.onerror = onReqError;
+    req.onabort = onReqAbort;
     req.open('GET', '/xmlhttprequest/resources/redirect.php?url=http://localhost:8080/xmlhttprequest/resources/forbidden.txt');
     req.send(null);
 }
 </script>
 <body onload="runTest()">
 <div>This tests that unsafe redirects won't be allowed when making an XMLHttpRequest.</div>
+<div id="log"></div>
 <div id="result">FAILURE</div>
 </body>
 </html>
index ab4021b..e9fad2a 100644 (file)
@@ -1,3 +1,31 @@
+2009-02-25  David Levin  <levin@chromium.org>
+
+        Reviewed by Alexey Proskuryakov.
+
+        Bug 24089: ThreadableLoader::loadResourceSynchronously should do callbacks like the async code.
+        <https://bugs.webkit.org/show_bug.cgi?id=24089>
+
+        Make threadable loader callbacks to happen during the sync load call.
+
+        Changes the behavior of sync xhr for insecure redirects in two ways:
+        + Sends an error event instead of an abort event (which is the same as async xhr's behavior).
+        + Throws a network exception which is what other browsers do and what the spec
+        says to do (http://www.w3.org/TR/XMLHttpRequest/).
+
+        * loader/DocumentThreadableLoader.cpp:
+        (WebCore::DocumentThreadableLoader::loadResourceSynchronously):
+        * loader/DocumentThreadableLoader.h:
+        * loader/ThreadableLoader.cpp:
+        (WebCore::ThreadableLoader::loadResourceSynchronously):
+        * loader/ThreadableLoader.h:
+        * xml/XMLHttpRequest.cpp:
+        (WebCore::XMLHttpRequest::XMLHttpRequest):
+        (WebCore::XMLHttpRequest::loadRequestSynchronously):
+        (WebCore::XMLHttpRequest::loadRequestAsynchronously):
+        (WebCore::XMLHttpRequest::didFail):
+        (WebCore::XMLHttpRequest::didFailRedirectCheck):
+        * xml/XMLHttpRequest.h:
+
 2009-02-24  Chris Marrin  <cmarrin@apple.com>
 
         Reviewed by David Hyatt.
index d519069..685db8c 100644 (file)
@@ -33,6 +33,9 @@
 
 #include "AuthenticationChallenge.h"
 #include "Document.h"
+#include "DocumentThreadableLoader.h"
+#include "Frame.h"
+#include "FrameLoader.h"
 #include "ResourceRequest.h"
 #include "SecurityOrigin.h"
 #include "SubresourceLoader.h"
 
 namespace WebCore {
 
+void DocumentThreadableLoader::loadResourceSynchronously(Document* document, const ResourceRequest& request, ThreadableLoaderClient& client)
+{
+    bool sameOriginRequest = document->securityOrigin()->canRequest(request.url());
+
+    Vector<char> data;
+    ResourceError error;
+    ResourceResponse response;
+    unsigned long identifier = std::numeric_limits<unsigned long>::max();
+    if (document->frame())
+        identifier = document->frame()->loader()->loadResourceSynchronously(request, error, response, data);
+
+    // No exception for file:/// resources, see <rdar://problem/4962298>.
+    // Also, if we have an HTTP response, then it wasn't a network error in fact.
+    if (!error.isNull() && !request.url().isLocalFile() && response.httpStatusCode() <= 0) {
+        client.didFail(error);
+        return;
+    }
+
+    // FIXME: This check along with the one in willSendRequest is specific to xhr and
+    // should be made more generic.
+    if (sameOriginRequest && !document->securityOrigin()->canRequest(response.url())) {
+        client.didFailRedirectCheck();
+        return;
+    }
+
+    client.didReceiveResponse(response);
+
+    const char* bytes = static_cast<const char*>(data.data());
+    int len = static_cast<int>(data.size());
+    client.didReceiveData(bytes, len);
+
+    client.didFinishLoading(identifier);
+}
+
 PassRefPtr<DocumentThreadableLoader> DocumentThreadableLoader::create(Document* document, ThreadableLoaderClient* client, const ResourceRequest& request, LoadCallbacks callbacksSetting, ContentSniff contentSniff)
 {
     ASSERT(document);
index 9fd64a0..a085644 100644 (file)
@@ -44,6 +44,7 @@ namespace WebCore {
 
     class DocumentThreadableLoader : public RefCounted<DocumentThreadableLoader>, public ThreadableLoader, private SubresourceLoaderClient  {
     public:
+        static void loadResourceSynchronously(Document*, const ResourceRequest&, ThreadableLoaderClient&);
         static PassRefPtr<DocumentThreadableLoader> create(Document*, ThreadableLoaderClient*, const ResourceRequest&, LoadCallbacks, ContentSniff);
         virtual ~DocumentThreadableLoader();
 
index 9939bce..2cdca7b 100644 (file)
 #include "config.h"
 #include "ThreadableLoader.h"
 
-#include "DocumentThreadableLoader.h"
-#include "Document.h"
-#include "Frame.h"
-#include "FrameLoader.h"
 #include "ScriptExecutionContext.h"
+#include "Document.h"
+#include "DocumentThreadableLoader.h"
 #include "WorkerContext.h"
 #include "WorkerRunLoop.h"
 #include "WorkerThreadableLoader.h"
@@ -56,15 +54,11 @@ PassRefPtr<ThreadableLoader> ThreadableLoader::create(ScriptExecutionContext* co
     return DocumentThreadableLoader::create(static_cast<Document*>(context), client, request, callbacksSetting, contentSniff);
 }
 
-unsigned long ThreadableLoader::loadResourceSynchronously(ScriptExecutionContext* context, const ResourceRequest& request, ResourceError& error, ResourceResponse& response, Vector<char>& data)
+void ThreadableLoader::loadResourceSynchronously(ScriptExecutionContext* context, const ResourceRequest& request, ThreadableLoaderClient& client)
 {
     ASSERT(context);
     ASSERT(context->isDocument());
-
-    Document* document = static_cast<Document*>(context);
-    if (!document->frame())
-        return std::numeric_limits<unsigned long>::max();
-    return document->frame()->loader()->loadResourceSynchronously(request, error, response, data);
+    DocumentThreadableLoader::loadResourceSynchronously(static_cast<Document*>(context), request, client);
 }
 
 } // namespace WebCore
index 4b939f0..f5162e1 100644 (file)
@@ -57,8 +57,8 @@ namespace WebCore {
     // just able to run on threads other than the main thread).
     class ThreadableLoader : Noncopyable {
     public:
+        static void loadResourceSynchronously(ScriptExecutionContext*, const ResourceRequest&, ThreadableLoaderClient&);
         static PassRefPtr<ThreadableLoader> create(ScriptExecutionContext*, ThreadableLoaderClient*, const ResourceRequest&, LoadCallbacks, ContentSniff);
-        static unsigned long loadResourceSynchronously(ScriptExecutionContext*, const ResourceRequest&, ResourceError&, ResourceResponse&, Vector<char>& data);
 
         virtual void cancel() = 0;
         void ref() { refThreadableLoader(); }
index aad0725..79098d3 100644 (file)
@@ -331,6 +331,7 @@ XMLHttpRequest::XMLHttpRequest(ScriptExecutionContext* context)
     , m_inPreflight(false)
     , m_receivedLength(0)
     , m_lastSendLineNumber(0)
+    , m_exceptionCode(0)
 {
     initializeXMLHttpRequestStaticData();
 }
@@ -808,34 +809,19 @@ void XMLHttpRequest::handleAsynchronousPreflightResult()
 void XMLHttpRequest::loadRequestSynchronously(ResourceRequest& request, ExceptionCode& ec)
 {
     ASSERT(!m_async);
-    Vector<char> data;
-    ResourceError error;
-    ResourceResponse response;
 
-    unsigned long identifier = ThreadableLoader::loadResourceSynchronously(scriptExecutionContext(), request, error, response, data);
     m_loader = 0;
-
-    // No exception for file:/// resources, see <rdar://problem/4962298>.
-    // Also, if we have an HTTP response, then it wasn't a network error in fact.
-    if (error.isNull() || request.url().isLocalFile() || response.httpStatusCode() > 0) {
-        processSyncLoadResults(identifier, data, response, ec);
-        return;
-    }
-
-    if (error.isCancellation()) {
-        abortError();
-        ec = XMLHttpRequestException::ABORT_ERR;
-        return;
-    }
-
-    networkError();
-    ec = XMLHttpRequestException::NETWORK_ERR;
+    m_exceptionCode = 0;
+    ThreadableLoader::loadResourceSynchronously(scriptExecutionContext(), request, *this);
+    if (!m_exceptionCode && m_error)
+        m_exceptionCode = XMLHttpRequestException::NETWORK_ERR;
+    ec = m_exceptionCode;
 }
 
-
 void XMLHttpRequest::loadRequestAsynchronously(ResourceRequest& request)
 {
     ASSERT(m_async);
+    m_exceptionCode = 0;
     // SubresourceLoader::create can return null here, for example if we're no longer attached to a page.
     // This is true while running onunload handlers.
     // FIXME: We need to be able to send XMLHttpRequests from onunload, <http://bugs.webkit.org/show_bug.cgi?id=10904>.
@@ -843,7 +829,7 @@ void XMLHttpRequest::loadRequestAsynchronously(ResourceRequest& request)
     // We need to keep content sniffing enabled for local files due to CFNetwork not providing a MIME type
     // for local files otherwise, <rdar://problem/5671813>.
     LoadCallbacks callbacks = m_inPreflight ? DoNotSendLoadCallbacks : SendLoadCallbacks;
-    ContentSniff contentSniff =  request.url().isLocalFile() ? SniffContent : DoNotSniffContent;
+    ContentSniff contentSniff = request.url().isLocalFile() ? SniffContent : DoNotSniffContent;
     m_loader = ThreadableLoader::create(scriptExecutionContext(), this, request, callbacks, contentSniff);
 
     if (m_loader) {
@@ -1135,25 +1121,6 @@ String XMLHttpRequest::statusText(ExceptionCode& ec) const
     return String();
 }
 
-void XMLHttpRequest::processSyncLoadResults(unsigned long identifier, const Vector<char>& data, const ResourceResponse& response, ExceptionCode& ec)
-{
-    if (m_sameOriginRequest && !scriptExecutionContext()->securityOrigin()->canRequest(response.url())) {
-        abort();
-        return;
-    }
-    
-    didReceiveResponse(response);
-    changeState(HEADERS_RECEIVED);
-
-    const char* bytes = static_cast<const char*>(data.data());
-    int len = static_cast<int>(data.size());
-    didReceiveData(bytes, len);
-
-    didFinishLoading(identifier);
-    if (m_error)
-        ec = XMLHttpRequestException::NETWORK_ERR;
-}
-
 void XMLHttpRequest::didFail(const ResourceError& error)
 {
     // If we are already in an error state, for instance we called abort(), bail out early.
@@ -1161,10 +1128,12 @@ void XMLHttpRequest::didFail(const ResourceError& error)
         return;
 
     if (error.isCancellation()) {
+        m_exceptionCode = XMLHttpRequestException::ABORT_ERR;
         abortError();
         return;
     }
 
+    m_exceptionCode = XMLHttpRequestException::NETWORK_ERR;
     networkError();
 }
 
index 0882ddc..05079b5 100644 (file)
@@ -137,7 +137,6 @@ private:
     void didReceiveResponsePreflight(const ResourceResponse&);
     void didFinishLoadingPreflight();
 
-    void processSyncLoadResults(unsigned long identifier, const Vector<char>& data, const ResourceResponse&, ExceptionCode&);
     void updateAndDispatchOnProgress(unsigned int len);
 
     String responseMIMEType() const;
@@ -235,6 +234,7 @@ private:
     
     unsigned m_lastSendLineNumber;
     String m_lastSendURL;
+    ExceptionCode m_exceptionCode;
 };
 
 } // namespace WebCore