Commit b3f041dd authored by kinuko@chromium.org's avatar kinuko@chromium.org

Make WorkerScriptLoader back to RefCounted class

This was made non-refcounted class in https://crrev.com/1190133002,
but looks like this could cause crashes in some corner cases.

To be more specific:
FrameLoaderClientImpl::dispatchDidFinishDocumentLoad() calls
WebFrameClient callback first and then WebView callback next, but
if the loader is deleted in the first callback an attempt to call the
second callback on a member variable would crash.

We could probably avoid this crash without making the loader class
ref-counted again, but for now I would like to fix the crash by simply
reverting some of the changes that're causing the crash.

BUG=524694

Review URL: https://codereview.chromium.org/1322523002

git-svn-id: svn://svn.chromium.org/blink/trunk@201346 bbb929c8-8fbe-4397-9dbb-9b2b20218538
parent c11bf2e8
...@@ -51,7 +51,7 @@ bool InProcessWorkerBase::initialize(ExecutionContext* context, const String& ur ...@@ -51,7 +51,7 @@ bool InProcessWorkerBase::initialize(ExecutionContext* context, const String& ur
if (scriptURL.isEmpty()) if (scriptURL.isEmpty())
return false; return false;
m_scriptLoader = adoptPtr(new WorkerScriptLoader()); m_scriptLoader = WorkerScriptLoader::create();
m_scriptLoader->loadAsynchronously( m_scriptLoader->loadAsynchronously(
*context, *context,
scriptURL, scriptURL,
......
...@@ -56,7 +56,7 @@ private: ...@@ -56,7 +56,7 @@ private:
void onResponse(); void onResponse();
void onFinished(); void onFinished();
OwnPtr<WorkerScriptLoader> m_scriptLoader; RefPtr<WorkerScriptLoader> m_scriptLoader;
RefPtrWillBeMember<ContentSecurityPolicy> m_contentSecurityPolicy; RefPtrWillBeMember<ContentSecurityPolicy> m_contentSecurityPolicy;
WorkerGlobalScopeProxy* m_contextProxy; // The proxy outlives the worker to perform thread shutdown. WorkerGlobalScopeProxy* m_contextProxy; // The proxy outlives the worker to perform thread shutdown.
}; };
......
...@@ -248,23 +248,23 @@ void WorkerGlobalScope::importScripts(const Vector<String>& urls, ExceptionState ...@@ -248,23 +248,23 @@ void WorkerGlobalScope::importScripts(const Vector<String>& urls, ExceptionState
} }
for (const KURL& completeURL : completedURLs) { for (const KURL& completeURL : completedURLs) {
WorkerScriptLoader scriptLoader; RefPtr<WorkerScriptLoader> scriptLoader(WorkerScriptLoader::create());
scriptLoader.setRequestContext(WebURLRequest::RequestContextScript); scriptLoader->setRequestContext(WebURLRequest::RequestContextScript);
scriptLoader.loadSynchronously(executionContext, completeURL, AllowCrossOriginRequests); scriptLoader->loadSynchronously(executionContext, completeURL, AllowCrossOriginRequests);
// If the fetching attempt failed, throw a NetworkError exception and abort all these steps. // If the fetching attempt failed, throw a NetworkError exception and abort all these steps.
if (scriptLoader.failed()) { if (scriptLoader->failed()) {
exceptionState.throwDOMException(NetworkError, "The script at '" + completeURL.elidedString() + "' failed to load."); exceptionState.throwDOMException(NetworkError, "The script at '" + completeURL.elidedString() + "' failed to load.");
return; return;
} }
InspectorInstrumentation::scriptImported(&executionContext, scriptLoader.identifier(), scriptLoader.script()); InspectorInstrumentation::scriptImported(&executionContext, scriptLoader->identifier(), scriptLoader->script());
scriptLoaded(scriptLoader.script().length(), scriptLoader.cachedMetadata() ? scriptLoader.cachedMetadata()->size() : 0); scriptLoaded(scriptLoader->script().length(), scriptLoader->cachedMetadata() ? scriptLoader->cachedMetadata()->size() : 0);
RefPtrWillBeRawPtr<ErrorEvent> errorEvent = nullptr; RefPtrWillBeRawPtr<ErrorEvent> errorEvent = nullptr;
OwnPtr<Vector<char>> cachedMetaData(scriptLoader.releaseCachedMetadata()); OwnPtr<Vector<char>> cachedMetaData(scriptLoader->releaseCachedMetadata());
OwnPtrWillBeRawPtr<CachedMetadataHandler> handler(createWorkerScriptCachedMetadataHandler(completeURL, cachedMetaData.get())); OwnPtrWillBeRawPtr<CachedMetadataHandler> handler(createWorkerScriptCachedMetadataHandler(completeURL, cachedMetaData.get()));
m_script->evaluate(ScriptSourceCode(scriptLoader.script(), scriptLoader.responseURL()), &errorEvent, handler.get(), m_v8CacheOptions); m_script->evaluate(ScriptSourceCode(scriptLoader->script(), scriptLoader->responseURL()), &errorEvent, handler.get(), m_v8CacheOptions);
if (errorEvent) { if (errorEvent) {
m_script->rethrowExceptionFromImportedScript(errorEvent.release(), exceptionState); m_script->rethrowExceptionFromImportedScript(errorEvent.release(), exceptionState);
return; return;
......
...@@ -100,11 +100,15 @@ void WorkerScriptLoader::loadAsynchronously(ExecutionContext& executionContext, ...@@ -100,11 +100,15 @@ void WorkerScriptLoader::loadAsynchronously(ExecutionContext& executionContext,
ResourceLoaderOptions resourceLoaderOptions; ResourceLoaderOptions resourceLoaderOptions;
resourceLoaderOptions.allowCredentials = AllowStoredCredentials; resourceLoaderOptions.allowCredentials = AllowStoredCredentials;
// During create, callbacks may happen which could remove the last reference
// to this object, while some of the callchain assumes that the client and
// loader wouldn't be deleted within callbacks.
// (E.g. see crbug.com/524694 for why we can't easily remove this protect)
RefPtr<WorkerScriptLoader> protect(this);
m_needToCancel = true; m_needToCancel = true;
m_threadableLoader = ThreadableLoader::create(executionContext, this, *request, options, resourceLoaderOptions); m_threadableLoader = ThreadableLoader::create(executionContext, this, *request, options, resourceLoaderOptions);
if (m_failed) if (m_failed)
notifyFinished(); notifyFinished();
// Do nothing here since notifyFinished() could delete |this|.
} }
const KURL& WorkerScriptLoader::responseURL() const const KURL& WorkerScriptLoader::responseURL() const
......
...@@ -37,6 +37,7 @@ ...@@ -37,6 +37,7 @@
#include "wtf/FastAllocBase.h" #include "wtf/FastAllocBase.h"
#include "wtf/Functional.h" #include "wtf/Functional.h"
#include "wtf/PassRefPtr.h" #include "wtf/PassRefPtr.h"
#include "wtf/RefCounted.h"
#include "wtf/text/StringBuilder.h" #include "wtf/text/StringBuilder.h"
namespace blink { namespace blink {
...@@ -47,11 +48,13 @@ class ResourceResponse; ...@@ -47,11 +48,13 @@ class ResourceResponse;
class ExecutionContext; class ExecutionContext;
class TextResourceDecoder; class TextResourceDecoder;
class CORE_EXPORT WorkerScriptLoader final : public ThreadableLoaderClient { class CORE_EXPORT WorkerScriptLoader final : public RefCounted<WorkerScriptLoader>, public ThreadableLoaderClient {
WTF_MAKE_FAST_ALLOCATED(WorkerScriptLoader); WTF_MAKE_FAST_ALLOCATED(WorkerScriptLoader);
public: public:
WorkerScriptLoader(); static PassRefPtr<WorkerScriptLoader> create()
~WorkerScriptLoader() override; {
return adoptRef(new WorkerScriptLoader());
}
void loadSynchronously(ExecutionContext&, const KURL&, CrossOriginRequestPolicy); void loadSynchronously(ExecutionContext&, const KURL&, CrossOriginRequestPolicy);
// TODO: |finishedCallback| is not currently guaranteed to be invoked if // TODO: |finishedCallback| is not currently guaranteed to be invoked if
...@@ -88,6 +91,11 @@ public: ...@@ -88,6 +91,11 @@ public:
void setRequestContext(WebURLRequest::RequestContext requestContext) { m_requestContext = requestContext; } void setRequestContext(WebURLRequest::RequestContext requestContext) { m_requestContext = requestContext; }
private: private:
friend class WTF::RefCounted<WorkerScriptLoader>;
WorkerScriptLoader();
~WorkerScriptLoader() override;
PassOwnPtr<ResourceRequest> createResourceRequest(); PassOwnPtr<ResourceRequest> createResourceRequest();
void notifyError(); void notifyError();
void notifyFinished(); void notifyFinished();
......
...@@ -261,7 +261,7 @@ void WebEmbeddedWorkerImpl::didFinishDocumentLoad(WebLocalFrame* frame, bool) ...@@ -261,7 +261,7 @@ void WebEmbeddedWorkerImpl::didFinishDocumentLoad(WebLocalFrame* frame, bool)
ASSERT(!m_askedToTerminate); ASSERT(!m_askedToTerminate);
m_loadingShadowPage = false; m_loadingShadowPage = false;
m_networkProvider = adoptPtr(m_workerContextClient->createServiceWorkerNetworkProvider(frame->dataSource())); m_networkProvider = adoptPtr(m_workerContextClient->createServiceWorkerNetworkProvider(frame->dataSource()));
m_mainScriptLoader = adoptPtr(new WorkerScriptLoader()); m_mainScriptLoader = WorkerScriptLoader::create();
m_mainScriptLoader->setRequestContext(WebURLRequest::RequestContextServiceWorker); m_mainScriptLoader->setRequestContext(WebURLRequest::RequestContextServiceWorker);
m_mainScriptLoader->loadAsynchronously( m_mainScriptLoader->loadAsynchronously(
*m_mainFrame->frame()->document(), *m_mainFrame->frame()->document(),
......
...@@ -103,7 +103,7 @@ private: ...@@ -103,7 +103,7 @@ private:
OwnPtr<WebServiceWorkerNetworkProvider> m_networkProvider; OwnPtr<WebServiceWorkerNetworkProvider> m_networkProvider;
// Kept around only while main script loading is ongoing. // Kept around only while main script loading is ongoing.
OwnPtr<WorkerScriptLoader> m_mainScriptLoader; RefPtr<WorkerScriptLoader> m_mainScriptLoader;
RefPtr<WorkerThread> m_workerThread; RefPtr<WorkerThread> m_workerThread;
RefPtr<WorkerLoaderProxy> m_loaderProxy; RefPtr<WorkerLoaderProxy> m_loaderProxy;
......
...@@ -172,7 +172,7 @@ void WebSharedWorkerImpl::didFinishDocumentLoad(WebLocalFrame* frame, bool) ...@@ -172,7 +172,7 @@ void WebSharedWorkerImpl::didFinishDocumentLoad(WebLocalFrame* frame, bool)
ASSERT(!m_loadingDocument); ASSERT(!m_loadingDocument);
ASSERT(!m_mainScriptLoader); ASSERT(!m_mainScriptLoader);
m_networkProvider = adoptPtr(m_client->createServiceWorkerNetworkProvider(frame->dataSource())); m_networkProvider = adoptPtr(m_client->createServiceWorkerNetworkProvider(frame->dataSource()));
m_mainScriptLoader = adoptPtr(new WorkerScriptLoader()); m_mainScriptLoader = WorkerScriptLoader::create();
m_mainScriptLoader->setRequestContext(WebURLRequest::RequestContextSharedWorker); m_mainScriptLoader->setRequestContext(WebURLRequest::RequestContextSharedWorker);
m_loadingDocument = toWebLocalFrameImpl(frame)->frame()->document(); m_loadingDocument = toWebLocalFrameImpl(frame)->frame()->document();
m_mainScriptLoader->loadAsynchronously( m_mainScriptLoader->loadAsynchronously(
......
...@@ -153,7 +153,7 @@ private: ...@@ -153,7 +153,7 @@ private:
bool m_isPausedOnStart; bool m_isPausedOnStart;
// Kept around only while main script loading is ongoing. // Kept around only while main script loading is ongoing.
OwnPtr<WorkerScriptLoader> m_mainScriptLoader; RefPtr<WorkerScriptLoader> m_mainScriptLoader;
RefPtr<WorkerLoaderProxy> m_loaderProxy; RefPtr<WorkerLoaderProxy> m_loaderProxy;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment