Reviewed by Brady Eidson.
authorap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Sep 2009 16:19:03 +0000 (16:19 +0000)
committerap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Sep 2009 16:19:03 +0000 (16:19 +0000)
        <rdar://problem/7259965> REGRESSION: http/tests/xmlhttprequest/cross-origin-authorization.html
        is failing/crashing intermittently
        https://bugs.webkit.org/show_bug.cgi?id=29322

        This was caused by CStringBuffer::encodeBase64() returning a buffer that wasn't zero terminated.
        The code had other issues as well, so I removed it altogether:
        - it claimed to avoid some buffer copies, but it didn't;
        - and I don't think that base64 encoding should be part of CString interface.

WebCore:
        * platform/network/mac/ResourceHandleMac.mm:
        (WebCore::encodeBasicAuthorization): Encode username and password using Base64.h directly.
        (WebCore::ResourceHandle::start): Use encodeBasicAuthorization().
        (+[WebCoreSynchronousLoader loadRequest:allowStoredCredentials:returningResponse:error:]): Ditto.
        (-[WebCoreSynchronousLoader connection:willSendRequest:redirectResponse:]): Extended logging
        to synchronous case.
        (-[WebCoreSynchronousLoader connectionShouldUseCredentialStorage:]): Ditto.
        (-[WebCoreSynchronousLoader connection:didReceiveAuthenticationChallenge:]): Ditto.
        (-[WebCoreSynchronousLoader connection:didReceiveResponse:]): Ditto.
        (-[WebCoreSynchronousLoader connection:didReceiveData:]): Ditto.
        (-[WebCoreSynchronousLoader connectionDidFinishLoading:]): Ditto.
        (-[WebCoreSynchronousLoader connection:didFailWithError:]): Ditto.

        * platform/network/cf/ResourceHandleCFNet.cpp: Matched Mac changes.

        * platform/text/CString.cpp:
        * platform/text/CString.h:
        (WebCore::CStringBuffer::create):
        (WebCore::CStringBuffer::CStringBuffer):
        Removed code that was added for Base64 in r48363.

LayoutTests:
        * http/tests/xmlhttprequest/cross-origin-authorization.html: While at it, made the test
        detect more error conditions

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

LayoutTests/ChangeLog
LayoutTests/http/tests/xmlhttprequest/cross-origin-authorization.html
WebCore/ChangeLog
WebCore/platform/network/cf/ResourceHandleCFNet.cpp
WebCore/platform/network/mac/ResourceHandleMac.mm
WebCore/platform/text/CString.cpp
WebCore/platform/text/CString.h

index d50efa2..3ab5fd6 100644 (file)
@@ -1,3 +1,14 @@
+2009-09-28  Alexey Proskuryakov  <ap@apple.com>
+
+        Reviewed by Brady Eidson.
+
+        <rdar://problem/7259965> REGRESSION: http/tests/xmlhttprequest/cross-origin-authorization.html
+        is failing/crashing intermittently
+        https://bugs.webkit.org/show_bug.cgi?id=29322
+
+        * http/tests/xmlhttprequest/cross-origin-authorization.html: While at it, made the test
+        detect more error conditions
+
 2009-09-29  Andras Becsi  <becsi.andras@stud.u-szeged.hu>
 
         Reviewed by Tor Arne Vestbø.
index ed8c676..b2076ef 100644 (file)
@@ -54,7 +54,10 @@ function test_sync_cookies()
     req.open("GET", "http://localhost:8000/xmlhttprequest/resources/cross-origin-check-cookies.php", false);
     req.withCredentials = true;
     req.send();
-    log(req.responseText.match(/WK\-cross\-origin/) ? "PASS" : "FAIL");
+    if (req.status == 200)
+        log(req.responseText.match(/WK\-cross\-origin/) ? "PASS" : "FAIL");
+    else
+        log("FAIL: Wrong status code " + req.status);
     test_async_auth_stored();
 }
 
@@ -99,7 +102,10 @@ function test_sync_auth_explicit()
     req.withCredentials = true;
     try {
         req.send();
-        log(req.responseText.match(/test2/) ? "FAIL: Explicit credentials were not ignored" : "PASS");
+        if (req.status == 200)
+            log(req.responseText.match(/test2/) ? "FAIL: Explicit credentials were not ignored" : "PASS");
+        else
+            log("FAIL: Wrong status code " + req.status);
     } catch (ex) {
         log("FAIL: Got an exception. " + ex);
     }
@@ -116,7 +122,10 @@ function test_async_auth_explicit()
     req.withCredentials = true;
     req.send();
     req.onload = function() {
-        log(req.responseText.match(/test2/) ? "FAIL: Explicit credentials were not ignored" : "PASS");
+        if (req.status == 200)
+            log(req.responseText.match(/test2/) ? "FAIL: Explicit credentials were not ignored" : "PASS");
+        else
+            log("FAIL: Wrong status code " + req.status);
         log("DONE");
         if (window.layoutTestController)
             layoutTestController.notifyDone();
index 97dd128..6b8fe91 100644 (file)
@@ -1,3 +1,37 @@
+2009-09-28  Alexey Proskuryakov  <ap@apple.com>
+
+        Reviewed by Brady Eidson.
+
+        <rdar://problem/7259965> REGRESSION: http/tests/xmlhttprequest/cross-origin-authorization.html
+        is failing/crashing intermittently
+        https://bugs.webkit.org/show_bug.cgi?id=29322
+
+        This was caused by CStringBuffer::encodeBase64() returning a buffer that wasn't zero terminated.
+        The code had other issues as well, so I removed it altogether:
+        - it claimed to avoid some buffer copies, but it didn't;
+        - and I don't think that base64 encoding should be part of CString interface.
+
+        * platform/network/mac/ResourceHandleMac.mm:
+        (WebCore::encodeBasicAuthorization): Encode username and password using Base64.h directly.
+        (WebCore::ResourceHandle::start): Use encodeBasicAuthorization().
+        (+[WebCoreSynchronousLoader loadRequest:allowStoredCredentials:returningResponse:error:]): Ditto.
+        (-[WebCoreSynchronousLoader connection:willSendRequest:redirectResponse:]): Extended logging
+        to synchronous case.
+        (-[WebCoreSynchronousLoader connectionShouldUseCredentialStorage:]): Ditto.
+        (-[WebCoreSynchronousLoader connection:didReceiveAuthenticationChallenge:]): Ditto.
+        (-[WebCoreSynchronousLoader connection:didReceiveResponse:]): Ditto.
+        (-[WebCoreSynchronousLoader connection:didReceiveData:]): Ditto.
+        (-[WebCoreSynchronousLoader connectionDidFinishLoading:]): Ditto.
+        (-[WebCoreSynchronousLoader connection:didFailWithError:]): Ditto.
+
+        * platform/network/cf/ResourceHandleCFNet.cpp: Matched Mac changes.
+
+        * platform/text/CString.cpp:
+        * platform/text/CString.h:
+        (WebCore::CStringBuffer::create):
+        (WebCore::CStringBuffer::CStringBuffer):
+        Removed code that was added for Base64 in r48363.
+
 2009-09-29  Jedrzej Nowacki  <jedrzej.nowacki@nokia.com>
 
         Reviewed by Simon Hausmann.
index d6138da..cd309a9 100644 (file)
@@ -31,6 +31,7 @@
 
 #include "AuthenticationCF.h"
 #include "AuthenticationChallenge.h"
+#include "Base64.h"
 #include "CString.h"
 #include "CookieStorageWin.h"
 #include "CredentialStorage.h"
@@ -108,6 +109,16 @@ static void setDefaultMIMEType(CFURLResponseRef response)
     CFURLResponseSetMIMEType(response, defaultMIMETypeString);
 }
 
+static String encodeBasicAuthorization(const String& user, const String& password)
+{
+    CString unencodedString = (user + ":" + password).utf8();
+    Vector<char> unencoded(unencodedString.length());
+    std::copy(unencodedString.data(), unencodedString.data() + unencodedString.length(), unencoded.begin());
+    Vector<char> encoded;
+    base64Encode(unencoded, encoded);
+    return String(encoded.data(), encoded.size());
+}
+
 CFURLRequestRef willSendRequest(CFURLConnectionRef conn, CFURLRequestRef cfRequest, CFURLResponseRef cfRedirectResponse, const void* clientInfo)
 {
     ResourceHandle* handle = static_cast<ResourceHandle*>(const_cast<void*>(clientInfo));
@@ -383,10 +394,7 @@ bool ResourceHandle::start(Frame* frame)
         d->m_initialCredential = CredentialStorage::getDefaultAuthenticationCredential(d->m_request.url());
         
     if (!d->m_initialCredential.isEmpty()) {
-        // Apply basic auth header
-        String unencoded = d->m_initialCredential.user() + ":" + d->m_initialCredential.password();
-        CString encoded = unencoded.utf8().base64Encode();
-        String authHeader = String::format("Basic %s", encoded.data());
+        String authHeader = "Basic " + encodeBasicAuthorization(d->m_initialCredential.user(), d->m_initialCredential.password());
         d->m_request.addHTTPHeaderField("Authorization", authHeader);
     }
 
@@ -753,10 +761,7 @@ RetainPtr<CFDataRef> WebCoreSynchronousLoader::load(const ResourceRequest& reque
             loader.m_initialCredential = CredentialStorage::getDefaultAuthenticationCredential(url);
 
         if (!loader.m_initialCredential.isEmpty()) {
-            // Apply basic auth header
-            String unencoded = loader.m_initialCredential.user() + ":" + loader.m_initialCredential.password();
-            CString encoded = unencoded.utf8().base64Encode();
-            String authHeader = String::format("Basic %s", encoded.data());
+            String authHeader = "Basic " + encodeBasicAuthorization(loader.m_initialCredential.user(), loader.m_initialCredential.password());
             requestWithInitialCredential.addHTTPHeaderField("Authorization", authHeader);
         }
 
index 4391970..83a4381 100644 (file)
@@ -28,6 +28,7 @@
 
 #import "AuthenticationChallenge.h"
 #import "AuthenticationMac.h"
+#import "Base64.h"
 #import "BlockExceptions.h"
 #import "CString.h"
 #import "CredentialStorage.h"
@@ -118,6 +119,16 @@ public:
     }
 };
 
+static String encodeBasicAuthorization(const String& user, const String& password)
+{
+    CString unencodedString = (user + ":" + password).utf8();
+    Vector<char> unencoded(unencodedString.length());
+    std::copy(unencodedString.data(), unencodedString.data() + unencodedString.length(), unencoded.begin());
+    Vector<char> encoded;
+    base64Encode(unencoded, encoded);
+    return String(encoded.data(), encoded.size());
+}
+
 ResourceHandleInternal::~ResourceHandleInternal()
 {
 }
@@ -178,10 +189,7 @@ bool ResourceHandle::start(Frame* frame)
         d->m_initialCredential = CredentialStorage::getDefaultAuthenticationCredential(d->m_request.url());
         
     if (!d->m_initialCredential.isEmpty()) {
-        // Apply basic auth header
-        String unencoded = d->m_initialCredential.user() + ":" + d->m_initialCredential.password();
-        CString encoded = unencoded.utf8().base64Encode();
-        String authHeader = String::format("Basic %s", encoded.data());
+        String authHeader = "Basic " + encodeBasicAuthorization(d->m_initialCredential.user(), d->m_initialCredential.password());
         d->m_request.addHTTPHeaderField("Authorization", authHeader);
     }
 
@@ -875,9 +883,11 @@ void ResourceHandle::receivedCancellation(const AuthenticationChallenge& challen
     [super dealloc];
 }
 
-- (NSURLRequest *)connection:(NSURLConnection *)unusedConnection willSendRequest:(NSURLRequest *)newRequest redirectResponse:(NSURLResponse *)redirectResponse
+- (NSURLRequest *)connection:(NSURLConnection *)connection willSendRequest:(NSURLRequest *)newRequest redirectResponse:(NSURLResponse *)redirectResponse
 {
-    UNUSED_PARAM(unusedConnection);
+    UNUSED_PARAM(connection);
+
+    LOG(Network, "WebCoreSynchronousLoader delegate connection:%p willSendRequest:%@ redirectResponse:%p", connection, [newRequest description], redirectResponse);
 
     // FIXME: This needs to be fixed to follow the redirect correctly even for cross-domain requests.
     if (m_url && !protocolHostAndPortAreEqual(m_url, [newRequest URL])) {
@@ -906,17 +916,21 @@ void ResourceHandle::receivedCancellation(const AuthenticationChallenge& challen
     return newRequest;
 }
 
-- (BOOL)connectionShouldUseCredentialStorage:(NSURLConnection *)unusedConnection
+- (BOOL)connectionShouldUseCredentialStorage:(NSURLConnection *)connection
 {
-    UNUSED_PARAM(unusedConnection);
+    UNUSED_PARAM(connection);
+
+    LOG(Network, "WebCoreSynchronousLoader delegate connectionShouldUseCredentialStorage:%p", connection);
 
     // FIXME: We should ask FrameLoaderClient whether using credential storage is globally forbidden.
     return m_allowStoredCredentials;
 }
 
-- (void)connection:(NSURLConnection *)unusedConnection didReceiveAuthenticationChallenge:(NSURLAuthenticationChallenge *)challenge
+- (void)connection:(NSURLConnection *)connection didReceiveAuthenticationChallenge:(NSURLAuthenticationChallenge *)challenge
 {
-    UNUSED_PARAM(unusedConnection);
+    UNUSED_PARAM(connection);
+
+    LOG(Network, "WebCoreSynchronousLoader delegate connection:%p didReceiveAuthenticationChallenge:%p", connection, challenge);
 
     if (m_user && m_pass) {
         NSURLCredential *credential = [[NSURLCredential alloc] initWithUser:m_user
@@ -947,9 +961,11 @@ void ResourceHandle::receivedCancellation(const AuthenticationChallenge& challen
     [[challenge sender] continueWithoutCredentialForAuthenticationChallenge:challenge];
 }
 
-- (void)connection:(NSURLConnection *)unusedConnection didReceiveResponse:(NSURLResponse *)response
+- (void)connection:(NSURLConnection *)connection didReceiveResponse:(NSURLResponse *)response
 {
-    UNUSED_PARAM(unusedConnection);
+    UNUSED_PARAM(connection);
+
+    LOG(Network, "WebCoreSynchronousLoader delegate connection:%p didReceiveResponse:%p (HTTP status %d, reported MIMEType '%s')", connection, response, [response respondsToSelector:@selector(statusCode)] ? [(id)response statusCode] : 0, [[response MIMEType] UTF8String]);
 
     NSURLResponse *r = [response copy];
     
@@ -957,9 +973,11 @@ void ResourceHandle::receivedCancellation(const AuthenticationChallenge& challen
     m_response = r;
 }
 
-- (void)connection:(NSURLConnection *)unusedConnection didReceiveData:(NSData *)data
+- (void)connection:(NSURLConnection *)connection didReceiveData:(NSData *)data
 {
-    UNUSED_PARAM(unusedConnection);
+    UNUSED_PARAM(connection);
+
+    LOG(Network, "WebCoreSynchronousLoader delegate connection:%p didReceiveData:%p", connection, data);
 
     if (!m_data)
         m_data = [[NSMutableData alloc] init];
@@ -967,16 +985,20 @@ void ResourceHandle::receivedCancellation(const AuthenticationChallenge& challen
     [m_data appendData:data];
 }
 
-- (void)connectionDidFinishLoading:(NSURLConnection *)unusedConnection
+- (void)connectionDidFinishLoading:(NSURLConnection *)connection
 {
-    UNUSED_PARAM(unusedConnection);
+    UNUSED_PARAM(connection);
+
+    LOG(Network, "WebCoreSynchronousLoader delegate connectionDidFinishLoading:%p", connection);
 
     m_isDone = YES;
 }
 
-- (void)connection:(NSURLConnection *)unusedConnection didFailWithError:(NSError *)error
+- (void)connection:(NSURLConnection *)connection didFailWithError:(NSError *)error
 {
-    UNUSED_PARAM(unusedConnection);
+    UNUSED_PARAM(connection);
+
+    LOG(Network, "WebCoreSynchronousLoader delegate connection:%p didFailWithError:%@", connection, error);
 
     ASSERT(!m_error);
     
@@ -1001,6 +1023,8 @@ void ResourceHandle::receivedCancellation(const AuthenticationChallenge& challen
 
 + (NSData *)loadRequest:(NSURLRequest *)request allowStoredCredentials:(BOOL)allowStoredCredentials returningResponse:(NSURLResponse **)response error:(NSError **)error
 {
+    LOG(Network, "WebCoreSynchronousLoader loadRequest:%@ allowStoredCredentials:%u", request, allowStoredCredentials);
+
     WebCoreSynchronousLoader *delegate = [[WebCoreSynchronousLoader alloc] init];
 
     KURL url([request URL]);
@@ -1024,10 +1048,7 @@ void ResourceHandle::receivedCancellation(const AuthenticationChallenge& challen
             delegate->m_initialCredential = CredentialStorage::getDefaultAuthenticationCredential(url);
             
         if (!delegate->m_initialCredential.isEmpty()) {
-            // Apply basic auth header
-            String unencoded = delegate->m_initialCredential.user() + ":" + delegate->m_initialCredential.password();
-            CString encoded = unencoded.utf8().base64Encode();
-            String authHeader = String::format("Basic %s", encoded.data());
+            String authHeader = "Basic " + encodeBasicAuthorization(delegate->m_initialCredential.user(), delegate->m_initialCredential.password());
             requestWithInitialCredentials.addHTTPHeaderField("Authorization", authHeader);
         }
         connection = [[NSURLConnection alloc] initWithRequest:requestWithInitialCredentials.nsURLRequest() delegate:delegate startImmediately:NO];
@@ -1047,7 +1068,9 @@ void ResourceHandle::receivedCancellation(const AuthenticationChallenge& challen
     
     [connection release];
     [delegate release];
-    
+
+    LOG(Network, "WebCoreSynchronousLoader done");
+
     return data;
 }
 
index 5b8ac58..25f5fa1 100644 (file)
 #include "config.h"
 #include "CString.h"
 
-#include "Base64.h"
-
 using std::min;
 
 namespace WebCore {
 
-PassRefPtr<CStringBuffer> CStringBuffer::base64Encode()
-{
-    Vector<char> encoded;
-    WebCore::base64Encode(m_vector, encoded);
-    return CStringBuffer::create(encoded);
-}
-
 CString::CString(const char* str)
 {
     init(str, strlen(str));
@@ -99,11 +90,6 @@ void CString::copyBufferIfNeeded()
     memcpy(m_buffer->mutableData(), m_temp->data(), len);
 }
 
-CString CString::base64Encode()
-{
-    return CString(m_buffer->base64Encode().get());
-}
-
 bool operator==(const CString& a, const CString& b)
 {
     if (a.isNull() != b.isNull())
index 2d1cc2b..b9030d6 100644 (file)
@@ -43,12 +43,9 @@ namespace WebCore {
         friend class CString;
 
         static PassRefPtr<CStringBuffer> create(unsigned length) { return adoptRef(new CStringBuffer(length)); }
-        static PassRefPtr<CStringBuffer> create(const Vector<char>& vectorToAdopt) { return adoptRef(new CStringBuffer(vectorToAdopt)); }
         CStringBuffer(unsigned length) : m_vector(length) { }
-        CStringBuffer(const Vector<char>& vectorToAdopt) : m_vector(vectorToAdopt) { }
         char* mutableData() { return m_vector.data(); }
 
-        PassRefPtr<CStringBuffer> base64Encode();
         Vector<char> m_vector;
     };
 
@@ -62,8 +59,6 @@ namespace WebCore {
         CString(CStringBuffer* buffer) : m_buffer(buffer) { }
         static CString newUninitialized(size_t length, char*& characterBuffer);
 
-        CString base64Encode();
-        
         const char* data() const;
         char* mutableData();
         unsigned length() const;