Fetching a Worker with url that isn't allowed from a file based test causes DRT to...
authorlevin@chromium.org <levin@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 11 Jun 2011 00:11:01 +0000 (00:11 +0000)
committerlevin@chromium.org <levin@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 11 Jun 2011 00:11:01 +0000 (00:11 +0000)
https://bugs.webkit.org/show_bug.cgi?id=62469

Reviewed by Dmitry Titov.

Source/WebCore:

Test: fast/workers/worker-crash-with-invalid-location.html

* workers/DefaultSharedWorkerRepository.cpp:
(WebCore::SharedWorkerScriptLoader::load): Changed to using the RefCounted version of WorkerScriptLoader.
* workers/Worker.cpp:
(WebCore::Worker::create): Ditto.
* workers/Worker.h: Ditto.
* workers/WorkerContext.cpp:
(WebCore::WorkerContext::importScripts):  Ditto.
* workers/WorkerScriptLoader.cpp:
(WebCore::WorkerScriptLoader::~WorkerScriptLoader): Created to
allow removing some header includes in WorkerScriptLoader.h.
(WebCore::WorkerScriptLoader::loadAsynchronously): Fix the ordering
of setPendingActivity and keep WorkerScriptLoader alive during a
potential callback.
* workers/WorkerScriptLoader.h: Made this RefCounted to allow for
keeping it alive during callbacks. Also, removed unnecessary header
inclusions (and added a destructor to facilitate that).
(WebCore::WorkerScriptLoader::create):

Source/WebKit/chromium:

Test: fast/workers/worker-crash-with-invalid-location.html

* src/SharedWorkerRepository.cpp:
(WebCore::SharedWorkerScriptLoader::SharedWorkerScriptLoader): Changed to using the RefCounted version
of WorkerScriptLoader.
(WebCore::SharedWorkerScriptLoader::load): Rearranged calls as done in similar places,
which allows for SharedWorkerScriptLoader to be deleted during the laodAsynchronously call
and for unsetPendingActivity to be called.
(WebCore::SharedWorkerScriptLoader::notifyFinished): Changed to using the RefCounted version
of WorkerScriptLoader.

LayoutTests:

* fast/workers/worker-crash-with-invalid-location-expected.txt: Added.
* fast/workers/worker-crash-with-invalid-location.html: Added.

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/workers/worker-crash-with-invalid-location-expected.txt [new file with mode: 0644]
LayoutTests/fast/workers/worker-crash-with-invalid-location.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/workers/DefaultSharedWorkerRepository.cpp
Source/WebCore/workers/Worker.cpp
Source/WebCore/workers/Worker.h
Source/WebCore/workers/WorkerContext.cpp
Source/WebCore/workers/WorkerScriptLoader.cpp
Source/WebCore/workers/WorkerScriptLoader.h
Source/WebKit/chromium/ChangeLog
Source/WebKit/chromium/src/SharedWorkerRepository.cpp

index 5558e2b..b367bc1 100644 (file)
@@ -1,5 +1,15 @@
 2011-06-10  David Levin  <levin@chromium.org>
 
+        Reviewed by Dmitry Titov.
+
+        Fetching a Worker with url that isn't allowed from a file based test causes DRT to crash.
+        https://bugs.webkit.org/show_bug.cgi?id=62469
+
+        * fast/workers/worker-crash-with-invalid-location-expected.txt: Added.
+        * fast/workers/worker-crash-with-invalid-location.html: Added.
+
+2011-06-10  David Levin  <levin@chromium.org>
+
         Reviewed by Adam Barth.
 
         Add tests for Web Workers at invalid urls.
diff --git a/LayoutTests/fast/workers/worker-crash-with-invalid-location-expected.txt b/LayoutTests/fast/workers/worker-crash-with-invalid-location-expected.txt
new file mode 100644 (file)
index 0000000..79eb458
--- /dev/null
@@ -0,0 +1,6 @@
+Blocked access to external URL http://example.com/worker.js
+Blocked access to external URL http://example.com/worker.js
+Test worker fetch of blocked url. Should print a "PASS" statement.
+
+PASS
+
diff --git a/LayoutTests/fast/workers/worker-crash-with-invalid-location.html b/LayoutTests/fast/workers/worker-crash-with-invalid-location.html
new file mode 100644 (file)
index 0000000..8fd90df
--- /dev/null
@@ -0,0 +1,32 @@
+<html>
+<body>
+<p>Test worker fetch of blocked url. Should print a "PASS" statement.</p>
+<div id=result></div>
+<script>
+if (window.layoutTestController)
+    layoutTestController.dumpAsText();
+
+function log(message)
+{
+    document.getElementById("result").innerHTML += message + "<br>";
+}
+
+try {
+    // DumpRenderTree allows a request to this url, but then blocks it
+    // during the "willSendRequest" callback (which caused a crash).
+    // In a browser, this will throw a security exception.
+    new Worker("http://example.com/worker.js");
+} catch (error) {
+}
+
+try {
+    // Ditto.
+    new SharedWorker("http://example.com/worker.js");
+} catch (error) {
+}
+
+log("PASS");
+
+</script>
+</body>
+</html>
index 1b54881..f8090f7 100644 (file)
@@ -1,3 +1,30 @@
+2011-06-10  David Levin  <levin@chromium.org>
+
+        Reviewed by Dmitry Titov.
+
+        Fetching a Worker with url that isn't allowed from a file based test causes DRT to crash.
+        https://bugs.webkit.org/show_bug.cgi?id=62469
+
+        Test: fast/workers/worker-crash-with-invalid-location.html
+
+        * workers/DefaultSharedWorkerRepository.cpp:
+        (WebCore::SharedWorkerScriptLoader::load): Changed to using the RefCounted version of WorkerScriptLoader.
+        * workers/Worker.cpp:
+        (WebCore::Worker::create): Ditto.
+        * workers/Worker.h: Ditto.
+        * workers/WorkerContext.cpp:
+        (WebCore::WorkerContext::importScripts):  Ditto.
+        * workers/WorkerScriptLoader.cpp:
+        (WebCore::WorkerScriptLoader::~WorkerScriptLoader): Created to
+        allow removing some header includes in WorkerScriptLoader.h.
+        (WebCore::WorkerScriptLoader::loadAsynchronously): Fix the ordering
+        of setPendingActivity and keep WorkerScriptLoader alive during a
+        potential callback.
+        * workers/WorkerScriptLoader.h: Made this RefCounted to allow for
+        keeping it alive during callbacks. Also, removed unnecessary header
+        inclusions (and added a destructor to facilitate that).
+        (WebCore::WorkerScriptLoader::create):
+
 2011-06-10  Alok Priyadarshi  <alokp@chromium.org>
 
         Reviewed by James Robinson.
index b46b25d..5f6cebe 100644 (file)
@@ -271,7 +271,7 @@ private:
     RefPtr<SharedWorker> m_worker;
     OwnPtr<MessagePortChannel> m_port;
     RefPtr<SharedWorkerProxy> m_proxy;
-    OwnPtr<WorkerScriptLoader> m_scriptLoader;
+    RefPtr<WorkerScriptLoader> m_scriptLoader;
 };
 
 SharedWorkerScriptLoader::SharedWorkerScriptLoader(PassRefPtr<SharedWorker> worker, PassOwnPtr<MessagePortChannel> port, PassRefPtr<SharedWorkerProxy> proxy)
@@ -283,13 +283,13 @@ SharedWorkerScriptLoader::SharedWorkerScriptLoader(PassRefPtr<SharedWorker> work
 
 void SharedWorkerScriptLoader::load(const KURL& url)
 {
-    // Mark this object as active for the duration of the load.
-    m_scriptLoader = adoptPtr(new WorkerScriptLoader(ResourceRequestBase::TargetIsSharedWorker));
-    m_scriptLoader->loadAsynchronously(m_worker->scriptExecutionContext(), url, DenyCrossOriginRequests, this);
-
     // Stay alive (and keep the SharedWorker and JS wrapper alive) until the load finishes.
     this->ref();
     m_worker->setPendingActivity(m_worker.get());
+
+    // Mark this object as active for the duration of the load.
+    m_scriptLoader = WorkerScriptLoader::create(ResourceRequestBase::TargetIsSharedWorker);
+    m_scriptLoader->loadAsynchronously(m_worker->scriptExecutionContext(), url, DenyCrossOriginRequests, this);
 }
 
 void SharedWorkerScriptLoader::notifyFinished()
index deb786f..bfac593 100644 (file)
@@ -64,12 +64,12 @@ PassRefPtr<Worker> Worker::create(const String& url, ScriptExecutionContext* con
     if (scriptURL.isEmpty())
         return 0;
 
-    worker->m_scriptLoader = adoptPtr(new WorkerScriptLoader(ResourceRequestBase::TargetIsWorker));
-    worker->m_scriptLoader->loadAsynchronously(context, scriptURL, DenyCrossOriginRequests, worker.get());
-
     // The worker context does not exist while loading, so we must ensure that the worker object is not collected, nor are its event listeners.
     worker->setPendingActivity(worker.get());
 
+    worker->m_scriptLoader = WorkerScriptLoader::create(ResourceRequestBase::TargetIsWorker);
+    worker->m_scriptLoader->loadAsynchronously(context, scriptURL, DenyCrossOriginRequests, worker.get());
+
     InspectorInstrumentation::didCreateWorker(context, worker->asID(), scriptURL.string(), false);
 
     return worker.release();
index 0e673ac..8e4ec3a 100644 (file)
@@ -37,9 +37,7 @@
 #include "MessagePort.h"
 #include "WorkerScriptLoaderClient.h"
 #include <wtf/Forward.h>
-#include <wtf/OwnPtr.h>
 #include <wtf/PassRefPtr.h>
-#include <wtf/RefCounted.h>
 #include <wtf/RefPtr.h>
 #include <wtf/text/AtomicStringHash.h>
 
@@ -79,7 +77,7 @@ namespace WebCore {
         virtual void refEventTarget() { ref(); }
         virtual void derefEventTarget() { deref(); }
 
-        OwnPtr<WorkerScriptLoader> m_scriptLoader;
+        RefPtr<WorkerScriptLoader> m_scriptLoader;
         WorkerContextProxy* m_contextProxy; // The proxy outlives the worker to perform thread shutdown.
     };
 
index e3189a3..58eb854 100644 (file)
@@ -245,19 +245,19 @@ void WorkerContext::importScripts(const Vector<String>& urls, ExceptionCode& ec)
     Vector<KURL>::const_iterator end = completedURLs.end();
 
     for (Vector<KURL>::const_iterator it = completedURLs.begin(); it != end; ++it) {
-        WorkerScriptLoader scriptLoader(ResourceRequestBase::TargetIsScript);
-        scriptLoader.loadSynchronously(scriptExecutionContext(), *it, AllowCrossOriginRequests);
+        RefPtr<WorkerScriptLoader> scriptLoader(WorkerScriptLoader::create(ResourceRequestBase::TargetIsScript));
+        scriptLoader->loadSynchronously(scriptExecutionContext(), *it, AllowCrossOriginRequests);
 
         // If the fetching attempt failed, throw a NETWORK_ERR exception and abort all these steps.
-        if (scriptLoader.failed()) {
+        if (scriptLoader->failed()) {
             ec = XMLHttpRequestException::NETWORK_ERR;
             return;
         }
 
-       InspectorInstrumentation::scriptImported(scriptExecutionContext(), scriptLoader.identifier(), scriptLoader.script());
+        InspectorInstrumentation::scriptImported(scriptExecutionContext(), scriptLoader->identifier(), scriptLoader->script());
 
         ScriptValue exception;
-        m_script->evaluate(ScriptSourceCode(scriptLoader.script(), scriptLoader.responseURL()), &exception);
+        m_script->evaluate(ScriptSourceCode(scriptLoader->script(), scriptLoader->responseURL()), &exception);
         if (!exception.hasNoValue()) {
             m_script->setException(exception);
             return;
index ed11159..e41548a 100644 (file)
 
 #include "CrossThreadTask.h"
 #include "ResourceRequest.h"
+#include "ResourceResponse.h"
 #include "ScriptExecutionContext.h"
 #include "SecurityOrigin.h"
+#include "TextResourceDecoder.h"
 #include "WorkerContext.h"
 #include "WorkerScriptLoaderClient.h"
 #include "WorkerThreadableLoader.h"
+
 #include <wtf/OwnPtr.h>
+#include <wtf/RefPtr.h>
 #include <wtf/UnusedParam.h>
 
 namespace WebCore {
@@ -51,6 +55,10 @@ WorkerScriptLoader::WorkerScriptLoader(ResourceRequestBase::TargetType targetTyp
 {
 }
 
+WorkerScriptLoader::~WorkerScriptLoader()
+{
+}
+
 void WorkerScriptLoader::loadSynchronously(ScriptExecutionContext* scriptExecutionContext, const KURL& url, CrossOriginRequestPolicy crossOriginRequestPolicy)
 {
     m_url = url;
@@ -84,6 +92,8 @@ void WorkerScriptLoader::loadAsynchronously(ScriptExecutionContext* scriptExecut
     options.crossOriginRequestPolicy = crossOriginRequestPolicy;
     options.sendLoadCallbacks = true;
 
+    // During create, callbacks may happen which remove the last reference to this object.
+    RefPtr<WorkerScriptLoader> protect(this);
     m_threadableLoader = ThreadableLoader::create(scriptExecutionContext, this, *request, options);
 }
 
index 67f113a..dd84313 100644 (file)
 #if ENABLE(WORKERS)
 
 #include "KURL.h"
-#include "ResourceRequest.h"
-#include "ResourceResponse.h"
-#include "TextResourceDecoder.h"
+#include "ResourceRequestBase.h"
 #include "ThreadableLoader.h"
 #include "ThreadableLoaderClient.h"
 
+#include <wtf/FastAllocBase.h>
+#include <wtf/PassRefPtr.h>
+#include <wtf/RefCounted.h>
+
 namespace WebCore {
 
+    class ResourceRequest;
+    class ResourceResponse;
     class ScriptExecutionContext;
+    class TextResourceDecoder;
     class WorkerScriptLoaderClient;
 
-    class WorkerScriptLoader : public ThreadableLoaderClient {
+    class WorkerScriptLoader : public RefCounted<WorkerScriptLoader>, public ThreadableLoaderClient {
+        WTF_MAKE_FAST_ALLOCATED;
     public:
-        explicit WorkerScriptLoader(ResourceRequestBase::TargetType);
+        static PassRefPtr<WorkerScriptLoader> create(ResourceRequestBase::TargetType targetType)
+        {
+            return adoptRef(new WorkerScriptLoader(targetType));
+        }
 
         void loadSynchronously(ScriptExecutionContext*, const KURL&, CrossOriginRequestPolicy);
         void loadAsynchronously(ScriptExecutionContext*, const KURL&, CrossOriginRequestPolicy, WorkerScriptLoaderClient*);
@@ -65,6 +74,11 @@ namespace WebCore {
         virtual void didReceiveAuthenticationCancellation(const ResourceResponse&);
 
     private:
+        friend class WTF::RefCounted<WorkerScriptLoader>;
+
+        explicit WorkerScriptLoader(ResourceRequestBase::TargetType);
+        ~WorkerScriptLoader();
+
         PassOwnPtr<ResourceRequest> createResourceRequest();
         void notifyFinished();
 
index f6e2a95..3170a79 100755 (executable)
@@ -1,3 +1,21 @@
+2011-06-10  David Levin  <levin@chromium.org>
+
+        Reviewed by Dmitry Titov.
+
+        Fetching a Worker with url that isn't allowed from a file based test causes DRT to crash.
+        https://bugs.webkit.org/show_bug.cgi?id=62469
+
+        Test: fast/workers/worker-crash-with-invalid-location.html
+
+        * src/SharedWorkerRepository.cpp:
+        (WebCore::SharedWorkerScriptLoader::SharedWorkerScriptLoader): Changed to using the RefCounted version
+        of WorkerScriptLoader.
+        (WebCore::SharedWorkerScriptLoader::load): Rearranged calls as done in similar places,
+        which allows for SharedWorkerScriptLoader to be deleted during the laodAsynchronously call
+        and for unsetPendingActivity to be called.
+        (WebCore::SharedWorkerScriptLoader::notifyFinished): Changed to using the RefCounted version
+        of WorkerScriptLoader.
+
 2011-06-10  Ryosuke Niwa  <rniwa@webkit.org>
 
         Rolled DEPS.
index e9c96fd..e2421ae 100644 (file)
@@ -70,7 +70,7 @@ public:
         , m_name(name)
         , m_webWorker(webWorker)
         , m_port(port)
-        , m_scriptLoader(ResourceRequestBase::TargetIsSharedWorker)
+        , m_scriptLoader(WorkerScriptLoader::create(ResourceRequestBase::TargetIsSharedWorker))
         , m_loading(false)
         , m_responseAppCacheID(0)
     {
@@ -96,7 +96,7 @@ private:
     String m_name;
     OwnPtr<WebSharedWorker> m_webWorker;
     OwnPtr<MessagePortChannel> m_port;
-    WorkerScriptLoader m_scriptLoader;
+    RefPtr<WorkerScriptLoader> m_scriptLoader;
     bool m_loading;
     long long m_responseAppCacheID;
 };
@@ -134,10 +134,11 @@ void SharedWorkerScriptLoader::load()
     if (m_webWorker->isStarted())
         sendConnect();
     else {
-        m_scriptLoader.loadAsynchronously(m_worker->scriptExecutionContext(), m_url, DenyCrossOriginRequests, this);
         // Keep the worker + JS wrapper alive until the resource load is complete in case we need to dispatch an error event.
         m_worker->setPendingActivity(m_worker.get());
         m_loading = true;
+
+        m_scriptLoader->loadAsynchronously(m_worker->scriptExecutionContext(), m_url, DenyCrossOriginRequests, this);
     }
 }
 
@@ -158,13 +159,13 @@ void SharedWorkerScriptLoader::didReceiveResponse(const ResourceResponse& respon
 
 void SharedWorkerScriptLoader::notifyFinished()
 {
-    if (m_scriptLoader.failed()) {
+    if (m_scriptLoader->failed()) {
         m_worker->dispatchEvent(Event::create(eventNames().errorEvent, false, true));
         delete this;
     } else {
-        InspectorInstrumentation::scriptImported(m_worker->scriptExecutionContext(), m_scriptLoader.identifier(), m_scriptLoader.script());
+        InspectorInstrumentation::scriptImported(m_worker->scriptExecutionContext(), m_scriptLoader->identifier(), m_scriptLoader->script());
         // Pass the script off to the worker, then send a connect event.
-        m_webWorker->startWorkerContext(m_url, m_name, m_worker->scriptExecutionContext()->userAgent(m_url), m_scriptLoader.script(), m_responseAppCacheID);
+        m_webWorker->startWorkerContext(m_url, m_name, m_worker->scriptExecutionContext()->userAgent(m_url), m_scriptLoader->script(), m_responseAppCacheID);
         sendConnect();
     }
 }