Let AssociatedURLLoader listen to destruction of the Document used for loading

third_party/WebKit/Source/web/AssociatedURLLoader.cpp

Index: third_party/WebKit/Source/web/AssociatedURLLoader.cpp
diff --git a/third_party/WebKit/Source/web/AssociatedURLLoader.cpp b/third_party/WebKit/Source/web/AssociatedURLLoader.cpp
index 6bff6875e29b821319c6bf36287770f53535d84b..7ed6a9ed071b047c7a5893cfd24c01b50110bfa4 100644
--- a/third_party/WebKit/Source/web/AssociatedURLLoader.cpp
+++ b/third_party/WebKit/Source/web/AssociatedURLLoader.cpp
@@ -30,6 +30,7 @@
 
 #include "web/AssociatedURLLoader.h"
 
+#include "core/dom/ContextLifecycleObserver.h"
 #include "core/fetch/CrossOriginAccessControl.h"
 #include "core/fetch/FetchUtils.h"
 #include "core/loader/DocumentThreadableLoader.h"
@@ -247,6 +248,8 @@ void AssociatedURLLoader::ClientAdapter::didFinishLoading(unsigned long identifi
     if (!m_client)
         return;
 
+    m_loader->disposeObserver();
+
     m_client->didFinishLoading(m_loader, finishTime, WebURLLoaderClient::kUnknownEncodedDataLength);
 }
 
@@ -255,6 +258,8 @@ void AssociatedURLLoader::ClientAdapter::didFail(const ResourceError& error)
     if (!m_client)
         return;
 
+    m_loader->disposeObserver();
+
     m_didFail = true;
     m_error = WebURLError(error);
     if (m_enableErrorNotifications)
@@ -266,11 +271,6 @@ void AssociatedURLLoader::ClientAdapter::didFailRedirectCheck()
     didFail(ResourceError());
 }
 
-void AssociatedURLLoader::ClientAdapter::setDelayedError(const ResourceError& error)
-{
-    didFail(error);
-}
-
 void AssociatedURLLoader::ClientAdapter::enableErrorNotifications()
 {
     m_enableErrorNotifications = true;
@@ -284,15 +284,45 @@ void AssociatedURLLoader::ClientAdapter::notifyError(Timer<ClientAdapter>* timer
 {
     ASSERT_UNUSED(timer, timer == &m_errorTimer);
 
+    if (!m_client)
+        return;
+
     m_client->didFail(m_loader, m_error);
 }
 
+class AssociatedURLLoader::Observer final : public GarbageCollectedFinalized<Observer>, public ContextLifecycleObserver {
+    USING_GARBAGE_COLLECTED_MIXIN(Observer);
+public:
+    Observer(AssociatedURLLoader* parent, Document* document)
+        : ContextLifecycleObserver(document)
+        , m_parent(parent)
+    {
+    }
+
+    void dispose()
+    {
+        m_parent = nullptr;
+        clearContext();
+    }
+
+    void contextDestroyed() override
+    {
+        if (m_parent)
+            m_parent->documentDestroyed();
+    }
+
+    DEFINE_INLINE_VIRTUAL_TRACE()
+    {
+        ContextLifecycleObserver::trace(visitor);
+    }
+
+    AssociatedURLLoader* m_parent;
+};
+
 AssociatedURLLoader::AssociatedURLLoader(RawPtr<WebLocalFrameImpl> frameImpl, const WebURLLoaderOptions& options)
-    : m_frameImpl(frameImpl)
-    , m_options(options)
-    , m_client(0)
+    : m_options(options)
+    , m_observer(new Observer(this, frameImpl->frame()->document()))
 {
-    DCHECK(m_frameImpl);
 }
 
 AssociatedURLLoader::~AssociatedURLLoader()
@@ -320,16 +350,15 @@ void AssociatedURLLoader::loadSynchronously(const WebURLRequest& request, WebURL
 void AssociatedURLLoader::loadAsynchronously(const WebURLRequest& request, WebURLLoaderClient* client)
 {
     DCHECK(!m_loader);
-    DCHECK(!m_client);
+    DCHECK(!m_clientAdapter);
 
-    m_client = client;
-    DCHECK(m_client);
+    DCHECK(client);
 
     bool allowLoad = true;
     WebURLRequest newRequest(request);
     if (m_options.untrustedHTTP) {
         WebString method = newRequest.httpMethod();
-        allowLoad = isValidHTTPToken(method) && FetchUtils::isUsefulMethod(method);
+        allowLoad = m_observer && isValidHTTPToken(method) && FetchUtils::isUsefulMethod(method);
         if (allowLoad) {
             newRequest.setHTTPMethod(FetchUtils::normalizeMethod(method));
             HTTPRequestHeaderValidator validator;
@@ -338,7 +367,7 @@ void AssociatedURLLoader::loadAsynchronously(const WebURLRequest& request, WebUR
         }
     }
 
-    m_clientAdapter = ClientAdapter::create(this, m_client, m_options);
+    m_clientAdapter = ClientAdapter::create(this, client, m_options);
 
     if (allowLoad) {
         ThreadableLoaderOptions options;
@@ -357,25 +386,34 @@ void AssociatedURLLoader::loadAsynchronously(const WebURLRequest& request, WebUR
             newRequest.setRequestContext(WebURLRequest::RequestContextInternal);
         }
 
-        Document* webcoreDocument = m_frameImpl->frame()->document();
-        DCHECK(webcoreDocument);
-        m_loader = DocumentThreadableLoader::create(*webcoreDocument, m_clientAdapter.get(), options, resourceLoaderOptions);
+        Document* document = toDocument(m_observer->lifecycleContext());
+        DCHECK(document);
+        m_loader = DocumentThreadableLoader::create(*document, m_clientAdapter.get(), options, resourceLoaderOptions);
         m_loader->start(webcoreRequest);
     }
 
     if (!m_loader) {
         // FIXME: return meaningful error codes.
-        m_clientAdapter->setDelayedError(ResourceError());
+        m_clientAdapter->didFail(ResourceError());
     }
     m_clientAdapter->enableErrorNotifications();
 }
 
 void AssociatedURLLoader::cancel()
 {
-    if (m_clientAdapter)
-        m_clientAdapter->clearClient();
-    if (m_loader)
+    disposeObserver();
+
+    if (!m_clientAdapter)
+        return;
+
+    // Prevent invocation of the WebURLLoaderClient methods.
+    m_clientAdapter->clearClient();
+
+    if (m_loader) {
         m_loader->cancel();
+        m_loader.clear();
+    }
+    m_clientAdapter.clear();
 }
 
 void AssociatedURLLoader::setDefersLoading(bool defersLoading)
@@ -389,4 +427,26 @@ void AssociatedURLLoader::setLoadingTaskRunner(blink::WebTaskRunner*)
     // TODO(alexclarke): Maybe support this one day if it proves worthwhile.
 }
 
+void AssociatedURLLoader::documentDestroyed()
+{
+    cancel();
+
+    m_client->didFail(this, ResourceError());
+    // |this| may be dead here.
+}
+
+void AssociatedURLLoader::disposeObserver()
+{
+    if (!m_observer)
+        return;
+
+    // The method of detecting Document destruction implemented here doesn't

[haraken, 2016/04/08 07:34:13]

Add TODO.

[tyoshino (SeeGerritForStatus), 2016/04/08 07:56:58]

Done.

+    // work for all kinds of Documents. In case we reached here after the
+    // Oilpan is destroyed, we just crash the renderer process to prevent UaF.

[haraken, 2016/04/08 07:34:13]

Sorry, how does this cause UaF?

[tyoshino (SeeGerritForStatus), 2016/04/08 07:56:58]

m_observer is on-heap. If the Oilpan is already shutdown, we shouldn't touch
m_observer.

We could consider the way that we just ignore the rest of the algorithm. But the
fact that we were not notified of destruction of the context means that we
haven't cleaned up the ThreadableLoader. ThreadableLoader involves some other
non-Blink instances e.g. WebURLLoader. It may be still facing the on-heap
objects, so we should kill the process for safety.

Improved the comment.

+    RELEASE_ASSERT(ThreadState::current());
+
+    m_observer->dispose();
+    m_observer = nullptr;
+}
+
 } // namespace blink