Unreviewed, rolling out r183393.
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 27 Apr 2015 19:36:25 +0000 (19:36 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 27 Apr 2015 19:36:25 +0000 (19:36 +0000)
https://bugs.webkit.org/show_bug.cgi?id=144272

Caused memory corruption detected by GuardMalloc (Requested by
ap on #webkit).

Reverted changeset:

"Synchronous XMLHttpRequest should get access to AppCache
resources stored as flat files"
https://bugs.webkit.org/show_bug.cgi?id=143711
http://trac.webkit.org/changeset/183393

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/http/tests/appcache/resources/fake-video.mp4 [deleted file]
LayoutTests/http/tests/appcache/resources/simple-video-sync.manifest [deleted file]
LayoutTests/http/tests/appcache/simple-video-sync-expected.txt [deleted file]
LayoutTests/http/tests/appcache/simple-video-sync.html [deleted file]
Source/WebCore/ChangeLog
Source/WebCore/html/HTMLMediaElement.cpp
Source/WebCore/loader/DocumentThreadableLoader.cpp
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/FrameLoader.h
Source/WebCore/loader/appcache/ApplicationCacheHost.cpp
Source/WebCore/loader/appcache/ApplicationCacheHost.h
Source/WebCore/xml/XSLTProcessorLibxslt.cpp
Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp

index ab16305..cc0e8ed 100644 (file)
@@ -1,3 +1,18 @@
+2015-04-27  Commit Queue  <commit-queue@webkit.org>
+
+        Unreviewed, rolling out r183393.
+        https://bugs.webkit.org/show_bug.cgi?id=144272
+
+        Caused memory corruption detected by GuardMalloc (Requested by
+        ap on #webkit).
+
+        Reverted changeset:
+
+        "Synchronous XMLHttpRequest should get access to AppCache
+        resources stored as flat files"
+        https://bugs.webkit.org/show_bug.cgi?id=143711
+        http://trac.webkit.org/changeset/183393
+
 2015-04-27  Yoav Weiss  <yoav@yoav.ws>
 
         Fix viewport units in Media Queries
diff --git a/LayoutTests/http/tests/appcache/resources/fake-video.mp4 b/LayoutTests/http/tests/appcache/resources/fake-video.mp4
deleted file mode 100644 (file)
index fa57e7f..0000000
+++ /dev/null
@@ -1 +0,0 @@
-This is a fake video
diff --git a/LayoutTests/http/tests/appcache/resources/simple-video-sync.manifest b/LayoutTests/http/tests/appcache/resources/simple-video-sync.manifest
deleted file mode 100644 (file)
index 872ccd6..0000000
+++ /dev/null
@@ -1,2 +0,0 @@
-CACHE MANIFEST
-fake-video.mp4
diff --git a/LayoutTests/http/tests/appcache/simple-video-sync-expected.txt b/LayoutTests/http/tests/appcache/simple-video-sync-expected.txt
deleted file mode 100644 (file)
index e1570f4..0000000
+++ /dev/null
@@ -1,2 +0,0 @@
-This tests that the application cache works for video retrieved by sync XMLHttpRequest
-SUCCESS
diff --git a/LayoutTests/http/tests/appcache/simple-video-sync.html b/LayoutTests/http/tests/appcache/simple-video-sync.html
deleted file mode 100644 (file)
index e509413..0000000
+++ /dev/null
@@ -1,36 +0,0 @@
-<html manifest="resources/simple-video-sync.manifest">
-<script>
-if (window.testRunner) {
-    testRunner.dumpAsText()
-    testRunner.waitUntilDone();
-}
-
-function finishTest(message) {
-    document.getElementById('result').innerHTML = message;
-    testRunner.notifyDone();
-}
-
-function cached()
-{
-    try {
-        var req = new XMLHttpRequest();
-        req.open("GET", "resources/fake-video.mp4", false);
-        req.send();
-        if (req.getResponseHeader("Content-Type") != "video/mp4")
-            finishTest("FAILURE: Did not get correct content type from cached resource");
-        if (req.responseText.trim() != "This is a fake video")
-            finishTest("FAILURE: Did not get correct data from cached resource");
-        finishTest("SUCCESS");
-    } catch (e) {
-        finishTest("FAILURE: Could not load video data from cache");
-    }
-}
-
-applicationCache.addEventListener('cached', cached, false);
-applicationCache.addEventListener('noupdate', cached, false);
-
-</script>
-<div>This tests that the application cache works for video retrieved by sync XMLHttpRequest</div>
-
-<div id="result">FAILURE</div>
-</html>
index e61cb03..44c7e64 100644 (file)
@@ -1,3 +1,18 @@
+2015-04-27  Commit Queue  <commit-queue@webkit.org>
+
+        Unreviewed, rolling out r183393.
+        https://bugs.webkit.org/show_bug.cgi?id=144272
+
+        Caused memory corruption detected by GuardMalloc (Requested by
+        ap on #webkit).
+
+        Reverted changeset:
+
+        "Synchronous XMLHttpRequest should get access to AppCache
+        resources stored as flat files"
+        https://bugs.webkit.org/show_bug.cgi?id=143711
+        http://trac.webkit.org/changeset/183393
+
 2015-04-27  Per Arne Vollan  <peavo@outlook.com>
 
         [Curl] Favicons loaded from disc cache are ignored.
index 1e66a34..e561655 100644 (file)
@@ -1063,6 +1063,25 @@ void HTMLMediaElement::loadNextSourceChild()
     loadResource(mediaURL, contentType, keySystem);
 }
 
+static URL createFileURLForApplicationCacheResource(const String& path)
+{
+    // URL should have a function to create a url from a path, but it does not. This function
+    // is not suitable because URL::setPath uses encodeWithURLEscapeSequences, which it notes
+    // does not correctly escape '#' and '?'. This function works for our purposes because
+    // app cache media files are always created with encodeForFileName(createCanonicalUUIDString()).
+
+#if USE(CF) && PLATFORM(WIN)
+    RetainPtr<CFURLRef> cfURL = adoptCF(CFURLCreateWithFileSystemPath(0, path.createCFString().get(), kCFURLWindowsPathStyle, false));
+    URL url(cfURL.get());
+#else
+    URL url;
+
+    url.setProtocol(ASCIILiteral("file"));
+    url.setPath(path);
+#endif
+    return url;
+}
+
 void HTMLMediaElement::loadResource(const URL& initialURL, ContentType& contentType, const String& keySystem)
 {
     ASSERT(isSafeToLoadURL(initialURL, Complain));
@@ -1127,7 +1146,7 @@ void HTMLMediaElement::loadResource(const URL& initialURL, ContentType& contentT
     m_currentSrc = url;
 
     if (resource) {
-        url = ApplicationCacheHost::createFileURL(resource->path());
+        url = createFileURLForApplicationCacheResource(resource->path());
         LOG(Media, "HTMLMediaElement::loadResource(%p) - will load from app cache -> %s", this, urlForLoggingMedia(url).utf8().data());
     }
 
index 1e86fac..531e9a3 100644 (file)
@@ -375,7 +375,7 @@ void DocumentThreadableLoader::loadRequest(const ResourceRequest& request, Secur
     }
     
     // FIXME: ThreadableLoaderOptions.sniffContent is not supported for synchronous requests.
-    RefPtr<SharedBuffer> data;
+    Vector<char> data;
     ResourceError error;
     ResourceResponse response;
     unsigned long identifier = std::numeric_limits<unsigned long>::max();
@@ -404,8 +404,10 @@ void DocumentThreadableLoader::loadRequest(const ResourceRequest& request, Secur
 
     didReceiveResponse(identifier, response);
 
-    if (data)
-        didReceiveData(identifier, data->data(), data->size());
+    const char* bytes = static_cast<const char*>(data.data());
+    int len = static_cast<int>(data.size());
+    didReceiveData(identifier, bytes, len);
+
     didFinishLoading(identifier, 0.0);
 }
 
index 9509156..2808280 100644 (file)
@@ -2660,7 +2660,7 @@ void FrameLoader::loadPostRequest(const ResourceRequest& inRequest, const String
     }
 }
 
-unsigned long FrameLoader::loadResourceSynchronously(const ResourceRequest& request, StoredCredentials storedCredentials, ClientCredentialPolicy clientCredentialPolicy, ResourceError& error, ResourceResponse& response, RefPtr<SharedBuffer>& data)
+unsigned long FrameLoader::loadResourceSynchronously(const ResourceRequest& request, StoredCredentials storedCredentials, ClientCredentialPolicy clientCredentialPolicy, ResourceError& error, ResourceResponse& response, Vector<char>& data)
 {
     ASSERT(m_frame.document());
     String referrer = SecurityPolicy::generateReferrerHeader(m_frame.document()->referrerPolicy(), request.url(), outgoingReferrer());
@@ -2682,15 +2682,13 @@ unsigned long FrameLoader::loadResourceSynchronously(const ResourceRequest& requ
 
     if (error.isNull()) {
         ASSERT(!newRequest.isNull());
-
+        
         if (!documentLoader()->applicationCacheHost()->maybeLoadSynchronously(newRequest, error, response, data)) {
-            Vector<char> buffer;
-            platformStrategies()->loaderStrategy()->loadResourceSynchronously(networkingContext(), identifier, newRequest, storedCredentials, clientCredentialPolicy, error, response, buffer);
-            data = SharedBuffer::adoptVector(buffer);
+            platformStrategies()->loaderStrategy()->loadResourceSynchronously(networkingContext(), identifier, newRequest, storedCredentials, clientCredentialPolicy, error, response, data);
             documentLoader()->applicationCacheHost()->maybeLoadFallbackSynchronously(newRequest, error, response, data);
         }
     }
-    notifier().sendRemainingDelegateMessages(m_documentLoader.get(), identifier, request, response, data ? data->data() : nullptr, data ? data->size() : 0, -1, error);
+    notifier().sendRemainingDelegateMessages(m_documentLoader.get(), identifier, request, response, data.data(), data.size(), -1, error);
     return identifier;
 }
 
index 616650c..cc2bd7c 100644 (file)
@@ -43,7 +43,6 @@
 #include "ResourceLoadNotifier.h"
 #include "ResourceRequestBase.h"
 #include "SecurityContext.h"
-#include "SharedBuffer.h"
 #include "Timer.h"
 #include <wtf/Forward.h>
 #include <wtf/HashSet.h>
@@ -117,7 +116,7 @@ public:
 #if ENABLE(WEB_ARCHIVE) || ENABLE(MHTML)
     WEBCORE_EXPORT void loadArchive(PassRefPtr<Archive>);
 #endif
-    unsigned long loadResourceSynchronously(const ResourceRequest&, StoredCredentials, ClientCredentialPolicy, ResourceError&, ResourceResponse&, RefPtr<SharedBuffer>& data);
+    unsigned long loadResourceSynchronously(const ResourceRequest&, StoredCredentials, ClientCredentialPolicy, ResourceError&, ResourceResponse&, Vector<char>& data);
 
     void changeLocation(SecurityOrigin*, const URL&, const String& referrer, LockHistory = LockHistory::Yes,
         LockBackForwardList = LockBackForwardList::Yes, bool refresh = false, AllowNavigationToInvalidURL = AllowNavigationToInvalidURL::Yes);
index 101da23..1f36a95 100644 (file)
@@ -31,7 +31,6 @@
 #include "ApplicationCacheResource.h"
 #include "DocumentLoader.h"
 #include "DOMApplicationCache.h"
-#include "FileSystem.h"
 #include "Frame.h"
 #include "FrameLoader.h"
 #include "FrameLoaderClient.h"
@@ -169,10 +168,10 @@ bool ApplicationCacheHost::maybeLoadResource(ResourceLoader* loader, const Resou
     ApplicationCacheResource* resource;
     if (!shouldLoadResourceFromApplicationCache(request, resource))
         return false;
-
+    
     m_documentLoader.m_pendingSubstituteResources.set(loader, resource);
     m_documentLoader.deliverSubstituteResourcesAfterDelay();
-
+        
     return true;
 }
 
@@ -203,48 +202,22 @@ bool ApplicationCacheHost::maybeLoadFallbackForError(ResourceLoader* resourceLoa
     return false;
 }
 
-URL ApplicationCacheHost::createFileURL(const String& path)
-{
-    // FIXME: Can we just use fileURLWithFileSystemPath instead?
-
-    // fileURLWithFileSystemPath function is not suitable because URL::setPath uses encodeWithURLEscapeSequences, which it notes
-    // does not correctly escape '#' and '?'. This function works for our purposes because
-    // app cache media files are always created with encodeForFileName(createCanonicalUUIDString()).
-
-#if USE(CF) && PLATFORM(WIN)
-    RetainPtr<CFURLRef> cfURL = adoptCF(CFURLCreateWithFileSystemPath(0, path.createCFString().get(), kCFURLWindowsPathStyle, false));
-    URL url(cfURL.get());
-#else
-    URL url;
-
-    url.setProtocol(ASCIILiteral("file"));
-    url.setPath(path);
-#endif
-    return url;
-}
-
-bool ApplicationCacheHost::maybeLoadSynchronously(ResourceRequest& request, ResourceError& error, ResourceResponse& response, RefPtr<SharedBuffer>& data)
+bool ApplicationCacheHost::maybeLoadSynchronously(ResourceRequest& request, ResourceError& error, ResourceResponse& response, Vector<char>& data)
 {
     ApplicationCacheResource* resource;
     if (shouldLoadResourceFromApplicationCache(request, resource)) {
         if (resource) {
-            // FIXME: Clients proably do not need a copy of the SharedBuffer.
-            // Remove the call to copy() once we ensure SharedBuffer will not be modified.
-            if (resource->path().isEmpty())
-                data = resource->data()->copy();
-            else
-                data = SharedBuffer::createWithContentsOfFile(resource->path());
-        }
-        if (!data)
-            error = m_documentLoader.frameLoader()->client().cannotShowURLError(request);
-        else
             response = resource->response();
+            data.append(resource->data()->data(), resource->data()->size());
+        } else {
+            error = m_documentLoader.frameLoader()->client().cannotShowURLError(request);
+        }
         return true;
     }
     return false;
 }
 
-void ApplicationCacheHost::maybeLoadFallbackSynchronously(const ResourceRequest& request, ResourceError& error, ResourceResponse& response, RefPtr<SharedBuffer>& data)
+void ApplicationCacheHost::maybeLoadFallbackSynchronously(const ResourceRequest& request, ResourceError& error, ResourceResponse& response, Vector<char>& data)
 {
     // If normal loading results in a redirect to a resource with another origin (indicative of a captive portal), or a 4xx or 5xx status code or equivalent,
     // or if there were network errors (but not if the user canceled the download), then instead get, from the cache, the resource of the fallback entry
@@ -255,9 +228,8 @@ void ApplicationCacheHost::maybeLoadFallbackSynchronously(const ResourceRequest&
         ApplicationCacheResource* resource;
         if (getApplicationCacheFallbackResource(request, resource)) {
             response = resource->response();
-            // FIXME: Clients proably do not need a copy of the SharedBuffer.
-            // Remove the call to copy() once we ensure SharedBuffer will not be modified.
-            data = resource->data()->copy();
+            data.clear();
+            data.append(resource->data()->data(), resource->data()->size());
         }
     }
 }
index 6df2b89..45bf05b 100644 (file)
@@ -41,11 +41,10 @@ namespace WebCore {
     class DOMApplicationCache;
     class DocumentLoader;
     class Frame;
-    class ResourceError;
     class ResourceLoader;
+    class ResourceError;
     class ResourceRequest;
     class ResourceResponse;
-    class SharedBuffer;
     class SubstituteData;
     class ApplicationCache;
     class ApplicationCacheGroup;
@@ -111,8 +110,6 @@ namespace WebCore {
         explicit ApplicationCacheHost(DocumentLoader&);
         ~ApplicationCacheHost();
 
-        static URL createFileURL(const String&);
-
         void selectCacheWithoutManifest();
         void selectCacheWithManifest(const URL& manifestURL);
 
@@ -128,8 +125,8 @@ namespace WebCore {
         WEBCORE_EXPORT bool maybeLoadFallbackForResponse(ResourceLoader*, const ResourceResponse&);
         WEBCORE_EXPORT bool maybeLoadFallbackForError(ResourceLoader*, const ResourceError&);
 
-        bool maybeLoadSynchronously(ResourceRequest&, ResourceError&, ResourceResponse&, RefPtr<SharedBuffer>&);
-        void maybeLoadFallbackSynchronously(const ResourceRequest&, ResourceError&, ResourceResponse&, RefPtr<SharedBuffer>&);
+        bool maybeLoadSynchronously(ResourceRequest&, ResourceError&, ResourceResponse&, Vector<char>& data);
+        void maybeLoadFallbackSynchronously(const ResourceRequest&, ResourceError&, ResourceResponse&, Vector<char>& data);
 
         bool canCacheInPageCache();
 
@@ -180,6 +177,7 @@ namespace WebCore {
         ApplicationCache* mainResourceApplicationCache() const { return m_mainResourceApplicationCache.get(); }
         bool maybeLoadFallbackForMainError(const ResourceRequest&, const ResourceError&);
 
+
         // The application cache that the document loader is associated with (if any).
         RefPtr<ApplicationCache> m_applicationCache;
 
index add49f7..54b781a 100644 (file)
@@ -124,19 +124,18 @@ static xmlDocPtr docLoaderFunc(const xmlChar* uri,
         ResourceError error;
         ResourceResponse response;
 
-        RefPtr<SharedBuffer> data;
+        Vector<char> data;
 
         bool requestAllowed = globalCachedResourceLoader->frame() && globalCachedResourceLoader->document()->securityOrigin()->canRequest(url);
         if (requestAllowed) {
             globalCachedResourceLoader->frame()->loader().loadResourceSynchronously(url, AllowStoredCredentials, DoNotAskClientForCrossOriginCredentials, error, response, data);
             if (error.isNull())
                 requestAllowed = globalCachedResourceLoader->document()->securityOrigin()->canRequest(response.url());
-            else if (data)
-                data = nullptr;
+            else
+                data.clear();
         }
         if (!requestAllowed) {
-            if (data)
-                data = nullptr;
+            data.clear();
             globalCachedResourceLoader->printAccessDeniedMessage(url);
         }
 
@@ -149,7 +148,7 @@ static xmlDocPtr docLoaderFunc(const xmlChar* uri,
 
         // We don't specify an encoding here. Neither Gecko nor WinIE respects
         // the encoding specified in the HTTP headers.
-        xmlDocPtr doc = xmlReadMemory(data ? data->data() : nullptr, data ? data->size() : 0, (const char*)uri, 0, options);
+        xmlDocPtr doc = xmlReadMemory(data.data(), data.size(), (const char*)uri, 0, options);
 
         xmlSetStructuredErrorFunc(0, 0);
         xmlSetGenericErrorFunc(0, 0);
index e04d825..60a0a5b 100644 (file)
@@ -454,7 +454,7 @@ static void* openFunc(const char* uri)
 
     ResourceError error;
     ResourceResponse response;
-    RefPtr<SharedBuffer> data;
+    Vector<char> data;
 
 
     {
@@ -470,10 +470,8 @@ static void* openFunc(const char* uri)
     // See <https://bugs.webkit.org/show_bug.cgi?id=21963>.
     if (!shouldAllowExternalLoad(response.url()))
         return &globalDescriptor;
-    Vector<char> buffer;
-    if (data)
-        buffer.append(data->data(), data->size());
-    return new OffsetBuffer(WTF::move(buffer));
+
+    return new OffsetBuffer(WTF::move(data));
 }
 
 static int readFunc(void* context, char* buffer, int len)