Improve CancellableTaskFactory handling and Oilpan usage.

If a CancellableTaskFactory is used by an Oilpan heap object, the
closure that the factory works with/over, cannot embed a reference
back to that object by way of an off-heap Persistent<> (WTF::Closure
is not on the heap.) If it does, such a reference will keep the heap
object alive, without it ever being released. Memory leaks are very likely.

This is too easy a slip-up to make with the current CancellableTaskFactory
constructor, so rephrase the constructor so as to make leaks no longer (easily)
possible. For Oilpan heap objects baked into the closure, the persistent reference
now held will be weak.

At the same time, take the opportunity to have this object no longer be a part object,
but a separate (off-heap) allocation. This lets us drop the ad-hoc ASan unpoisoning
support that was previously needed if CancellableTaskFactory was a part object
of an Oilpan heap object. Less magic.

R=haraken
BUG=

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

git-svn-id: svn://svn.chromium.org/blink/trunk@201787 bbb929c8-8fbe-4397-9dbb-9b2b20218538

third_party/WebKit/Source/core/dom/Document.cpp

@@ -203,6 +203,7 @@
 #include "platform/TraceEvent.h"
 #include "platform/network/ContentSecurityPolicyParsers.h"
 #include "platform/network/HTTPParsers.h"
+#include "platform/scheduler/CancellableTaskFactory.h"
 #include "platform/scroll/ScrollbarTheme.h"
 #include "platform/text/PlatformLocale.h"
 #include "platform/text/SegmentedString.h"
@@ -366,25 +367,6 @@ private:
     }
 };
 
-} // namespace blink
-
-namespace WTF {
-
-#if ENABLE(OILPAN)
-// NOTE this is to prevent Document::m_executeScriptsWaitingForResourcesTask from leaking.
-template<>
-struct PointerParamStorageTraits<blink::Document*, true> {
-    using StorageType = blink::CrossThreadWeakPersistent<blink::Document>;
-
-    static StorageType wrap(blink::Document* value) { return value; }
-    static blink::Document* unwrap(const StorageType& value) { return value.get(); }
-};
-#endif
-
-} // namespace WTF
-
-namespace blink {
-
 Document::Document(const DocumentInit& initializer, DocumentClassFlags documentClasses)
     : ContainerNode(0, CreateDocument)
     , TreeScope(*this)
@@ -401,7 +383,7 @@ Document::Document(const DocumentInit& initializer, DocumentClassFlags documentC
     , m_paginatedForScreen(false)
     , m_compatibilityMode(NoQuirksMode)
     , m_compatibilityModeLocked(false)
-    , m_executeScriptsWaitingForResourcesTask(WTF::bind(&Document::executeScriptsWaitingForResources, this))
+    , m_executeScriptsWaitingForResourcesTask(CancellableTaskFactory::create(this, &Document::executeScriptsWaitingForResources))
     , m_hasAutofocused(false)
     , m_clearFocusedElementTimer(this, &Document::clearFocusedElementTimerFired)
     , m_domTreeVersion(++s_globalTreeVersion)
@@ -2982,7 +2964,7 @@ void Document::didRemoveAllPendingStylesheet()
 void Document::didLoadAllScriptBlockingResources()
 {
     Platform::current()->currentThread()->scheduler()->loadingTaskRunner()->postTask(
-        FROM_HERE, m_executeScriptsWaitingForResourcesTask.cancelAndCreate());
+        FROM_HERE, m_executeScriptsWaitingForResourcesTask->cancelAndCreate());
 
     if (frame())
         frame()->loader().client()->didRemoveAllPendingStylesheet();

third_party/WebKit/Source/core/dom/Document.h

@@ -57,7 +57,6 @@
 #include "platform/Length.h"
 #include "platform/Timer.h"
 #include "platform/heap/Handle.h"
-#include "platform/scheduler/CancellableTaskFactory.h"
 #include "platform/weborigin/KURL.h"
 #include "platform/weborigin/ReferrerPolicy.h"
 #include "public/platform/WebFocusType.h"
@@ -75,6 +74,7 @@ class Attr;
 class CDATASection;
 class CSSStyleDeclaration;
 class CSSStyleSheet;
+class CancellableTaskFactory;
 class CanvasFontCache;
 class CanvasRenderingContext2D;
 class CanvasRenderingContext2DOrWebGLRenderingContext;
@@ -1200,7 +1200,7 @@ private:
     CompatibilityMode m_compatibilityMode;
     bool m_compatibilityModeLocked; // This is cheaper than making setCompatibilityMode virtual.
 
-    CancellableTaskFactory m_executeScriptsWaitingForResourcesTask;
+    OwnPtr<CancellableTaskFactory> m_executeScriptsWaitingForResourcesTask;
 
     bool m_hasAutofocused;
     Timer<Document> m_clearFocusedElementTimer;

third_party/WebKit/Source/core/dom/ScriptRunner.cpp

@@ -30,29 +30,19 @@
 #include "core/dom/Element.h"
 #include "core/dom/ScriptLoader.h"
 #include "platform/heap/Handle.h"
+#include "platform/scheduler/CancellableTaskFactory.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebScheduler.h"
 #include "public/platform/WebThread.h"
-#include "wtf/Functional.h"
-
-// This bit of magic is needed by oilpan to prevent the ScriptRunner from leaking.
-namespace WTF {
-template<>
-struct ParamStorageTraits<blink::ScriptRunner*> : public PointerParamStorageTraits<blink::ScriptRunner*, false> {
-};
-}
 
 namespace blink {
 
 
 ScriptRunner::ScriptRunner(Document* document)
     : m_document(document)
-    , m_executeScriptsTaskFactory(WTF::bind(&ScriptRunner::executeScripts, this))
+    , m_executeScriptsTaskFactory(CancellableTaskFactory::create(this, &ScriptRunner::executeScripts))
 {
     ASSERT(document);
-#if ENABLE(LAZY_SWEEPING) && defined(ADDRESS_SANITIZER)
-    m_executeScriptsTaskFactory.setUnpoisonBeforeUpdate();
-#endif
 }
 
 ScriptRunner::~ScriptRunner()
@@ -92,7 +82,7 @@ void ScriptRunner::queueScriptForExecution(ScriptLoader* scriptLoader, Execution
 
 void ScriptRunner::suspend()
 {
-    m_executeScriptsTaskFactory.cancel();
+    m_executeScriptsTaskFactory->cancel();
 }
 
 void ScriptRunner::resume()
@@ -224,10 +214,10 @@ bool ScriptRunner::yieldForHighPriorityWork()
 
 void ScriptRunner::postTaskIfOneIsNotAlreadyInFlight()
 {
-    if (m_executeScriptsTaskFactory.isPending())
+    if (m_executeScriptsTaskFactory->isPending())
         return;
 
-    Platform::current()->currentThread()->scheduler()->loadingTaskRunner()->postTask(FROM_HERE, m_executeScriptsTaskFactory.cancelAndCreate());
+    Platform::current()->currentThread()->scheduler()->loadingTaskRunner()->postTask(FROM_HERE, m_executeScriptsTaskFactory->cancelAndCreate());
 }
 
 DEFINE_TRACE(ScriptRunner)
@@ -240,4 +230,4 @@ DEFINE_TRACE(ScriptRunner)
 #endif
 }
 
-}
+} // namespace blink

third_party/WebKit/Source/core/dom/ScriptRunner.h

@@ -29,7 +29,6 @@
 #include "core/CoreExport.h"
 #include "core/fetch/ResourcePtr.h"
 #include "platform/heap/Handle.h"
-#include "platform/scheduler/CancellableTaskFactory.h"
 #include "wtf/Deque.h"
 #include "wtf/HashMap.h"
 #include "wtf/Noncopyable.h"
@@ -37,6 +36,7 @@
 
 namespace blink {
 
+class CancellableTaskFactory;
 class Document;
 class ScriptLoader;
 
@@ -79,7 +79,7 @@ private:
     // http://www.whatwg.org/specs/web-apps/current-work/#set-of-scripts-that-will-execute-as-soon-as-possible
     WillBeHeapDeque<RawPtrWillBeMember<ScriptLoader>> m_scriptsToExecuteSoon;
     WillBeHeapHashSet<RawPtrWillBeMember<ScriptLoader>> m_pendingAsyncScripts;
-    CancellableTaskFactory m_executeScriptsTaskFactory;
+    OwnPtr<CancellableTaskFactory> m_executeScriptsTaskFactory;
 };
 
 }

third_party/WebKit/Source/core/dom/ScriptRunnerTest.cpp

@@ -8,6 +8,7 @@
 #include "core/dom/Document.h"
 #include "core/dom/Element.h"
 #include "core/dom/ScriptLoader.h"
+#include "platform/scheduler/CancellableTaskFactory.h"
 #include "public/platform/Platform.h"
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>

third_party/WebKit/Source/core/html/parser/HTMLParserScheduler.cpp

@@ -85,7 +85,7 @@ void SpeculationsPumpSession::addedElementTokens(size_t count)
 HTMLParserScheduler::HTMLParserScheduler(HTMLDocumentParser* parser)
     : m_parser(parser)
     , m_loadingTaskRunner(Platform::current()->currentThread()->scheduler()->loadingTaskRunner())
-    , m_cancellableContinueParse(WTF::bind(&HTMLParserScheduler::continueParsing, this))
+    , m_cancellableContinueParse(CancellableTaskFactory::create(this, &HTMLParserScheduler::continueParsing))
     , m_isSuspendedWithActiveTimer(false)
 {
 }
@@ -97,31 +97,31 @@ HTMLParserScheduler::~HTMLParserScheduler()
 void HTMLParserScheduler::scheduleForResume()
 {
     ASSERT(!m_isSuspendedWithActiveTimer);
-    m_loadingTaskRunner->postTask(FROM_HERE, m_cancellableContinueParse.cancelAndCreate());
+    m_loadingTaskRunner->postTask(FROM_HERE, m_cancellableContinueParse->cancelAndCreate());
 }
 
 void HTMLParserScheduler::suspend()
 {
     ASSERT(!m_isSuspendedWithActiveTimer);
-    if (!m_cancellableContinueParse.isPending())
+    if (!m_cancellableContinueParse->isPending())
         return;
     m_isSuspendedWithActiveTimer = true;
-    m_cancellableContinueParse.cancel();
+    m_cancellableContinueParse->cancel();
 }
 
 void HTMLParserScheduler::resume()
 {
-    ASSERT(!m_cancellableContinueParse.isPending());
+    ASSERT(!m_cancellableContinueParse->isPending());
     if (!m_isSuspendedWithActiveTimer)
         return;
     m_isSuspendedWithActiveTimer = false;
 
-    m_loadingTaskRunner->postTask(FROM_HERE, m_cancellableContinueParse.cancelAndCreate());
+    m_loadingTaskRunner->postTask(FROM_HERE, m_cancellableContinueParse->cancelAndCreate());
 }
 
 void HTMLParserScheduler::detach()
 {
-    m_cancellableContinueParse.cancel();
+    m_cancellableContinueParse->cancel();
     m_isSuspendedWithActiveTimer = false;
 }
 
@@ -161,7 +161,7 @@ bool HTMLParserScheduler::yieldIfNeeded(const SpeculationsPumpSession& session,
 
 void HTMLParserScheduler::forceResumeAfterYield()
 {
-    ASSERT(!m_cancellableContinueParse.isPending());
+    ASSERT(!m_cancellableContinueParse->isPending());
     m_isSuspendedWithActiveTimer = true;
 }
 

third_party/WebKit/Source/core/html/parser/HTMLParserScheduler.h

@@ -78,7 +78,7 @@ public:
     }
     ~HTMLParserScheduler();
 
-    bool isScheduledForResume() const { return m_isSuspendedWithActiveTimer || m_cancellableContinueParse.isPending(); }
+    bool isScheduledForResume() const { return m_isSuspendedWithActiveTimer || m_cancellableContinueParse->isPending(); }
 
     void scheduleForResume();
     bool yieldIfNeeded(const SpeculationsPumpSession&, bool startingScript);
@@ -106,7 +106,7 @@ private:
     HTMLDocumentParser* m_parser;
     WebTaskRunner* m_loadingTaskRunner; // NOT OWNED
 
-    CancellableTaskFactory m_cancellableContinueParse;
+    OwnPtr<CancellableTaskFactory> m_cancellableContinueParse;
     bool m_isSuspendedWithActiveTimer;
 };
 

third_party/WebKit/Source/platform/heap/Handle.h

@@ -989,6 +989,16 @@ private:
     OwnPtr<Persistent<Self>> m_keepAlive;
 };
 
+template<typename T>
+class AllowCrossThreadWeakPersistent {
+    STACK_ALLOCATED();
+public:
+    explicit AllowCrossThreadWeakPersistent(T* value) : m_value(value) { }
+    CrossThreadWeakPersistent<T> value() const { return m_value; }
+private:
+    CrossThreadWeakPersistent<T> m_value;
+};
+
 } // namespace blink
 
 namespace WTF {
@@ -1179,6 +1189,18 @@ struct ParamStorageTraits<RawPtr<T>> : public PointerParamStorageTraits<T*, blin
     static_assert(sizeof(T), "T must be fully defined");
 };
 
+template<typename T>
+struct ParamStorageTraits<blink::AllowCrossThreadWeakPersistent<T>> {
+    static_assert(sizeof(T), "T must be fully defined");
+    using StorageType = blink::CrossThreadWeakPersistent<T>;
+
+    static StorageType wrap(const blink::AllowCrossThreadWeakPersistent<T>& value) { return value.value(); }
+
+    // Currently assume that the call sites of this unwrap() account for cleared weak references also.
+    // TODO(sof): extend WTF::FunctionWrapper call overloading to also handle (CrossThread)WeakPersistent.
+    static T* unwrap(const StorageType& value) { return value.get(); }
+};
+
 template<typename T>
 PassRefPtr<T> adoptRef(blink::RefCountedGarbageCollected<T>*) = delete;
 

third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.cpp

@@ -21,14 +21,9 @@ WebTaskRunner::Task* CancellableTaskFactory::cancelAndCreate()
     return new CancellableTask(m_weakPtrFactory.createWeakPtr());
 }
 
-NO_LAZY_SWEEP_SANITIZE_ADDRESS
 void CancellableTaskFactory::CancellableTask::run()
 {
     if (CancellableTaskFactory* taskFactory = m_weakPtr.get()) {
-#if defined(ADDRESS_SANITIZER)
-        if (taskFactory->m_unpoisonBeforeUpdate)
-            ASAN_UNPOISON_MEMORY_REGION(reinterpret_cast<unsigned char*>(taskFactory), sizeof(CancellableTaskFactory));
-#endif
         Closure* closure = taskFactory->m_closure.get();
         taskFactory->m_weakPtrFactory.revokeAll();
         (*closure)();

third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.h

@@ -6,27 +6,51 @@
 #define CancellableTaskFactory_h
 
 #include "platform/PlatformExport.h"
+#include "platform/heap/Handle.h"
 #include "public/platform/WebScheduler.h"
-#include "wtf/AddressSanitizer.h"
 #include "wtf/Functional.h"
 #include "wtf/Noncopyable.h"
 #include "wtf/OwnPtr.h"
 #include "wtf/PassOwnPtr.h"
 #include "wtf/RefCounted.h"
+#include "wtf/TypeTraits.h"
 #include "wtf/WeakPtr.h"
 
 namespace blink {
+
 class TraceLocation;
 
 class PLATFORM_EXPORT CancellableTaskFactory {
     WTF_MAKE_NONCOPYABLE(CancellableTaskFactory);
-
+    WTF_MAKE_FAST_ALLOCATED(CancellableTaskFactory);
 public:
+    // A pair of mutually exclusive factory methods are provided for constructing
+    // a CancellableTaskFactory, one for when a Oilpan heap object owns a
+    // CancellableTaskFactory, and one when that owning object isn't controlled
+    // by Oilpan.
+    //
+    // In the Oilpan case, as WTF::Closure objects are off-heap, we have to construct the
+    // closure in such a manner that it doesn't end up referring back to the owning heap
+    // object with a strong Persistent<> GC root reference. If we do, this will create
+    // a heap <-> off-heap cycle and leak, the owning object can never be GCed.
+    // Instead, the closure will keep an off-heap persistent reference of the weak
+    // variety, which will refer back to the owner heap object safely (but weakly.)
+    //
+    template<typename T>
+    static PassOwnPtr<CancellableTaskFactory> create(T* thisObject, void (T::*method)(), typename WTF::EnableIf<IsGarbageCollectedType<T>::value>::Type* = nullptr)
+    {
+        return adoptPtr(new CancellableTaskFactory(WTF::bind(method, AllowCrossThreadWeakPersistent<T>(thisObject))));
+    }
+
+    template<typename T>
+    static PassOwnPtr<CancellableTaskFactory> create(T* thisObject, void (T::*method)(), typename WTF::EnableIf<!IsGarbageCollectedType<T>::value>::Type* = nullptr)
+    {
+        return adoptPtr(new CancellableTaskFactory(WTF::bind(method, thisObject)));
+    }
+
+    // Only intended used by unit tests. Please use leak safe create() factory method, if possible.
     explicit CancellableTaskFactory(PassOwnPtr<Closure> closure)
         : m_closure(closure)
-#if defined(ADDRESS_SANITIZER)
-        , m_unpoisonBeforeUpdate(false)
-#endif
         , m_weakPtrFactory(this)
     {
     }
@@ -42,15 +66,6 @@ public:
     // ownership of the task.  Creating a new task cancels any previous ones.
     WebTaskRunner::Task* cancelAndCreate();
 
-#if defined(ADDRESS_SANITIZER)
-    // The CancellableTaskFactory part object might be within a poisoned heap
-    // object, hence CancellableTask::run() will access poisoned memory
-    // when reaching into the factory object to update its state.
-    // We will allow such access iff the task factory is marked as requiring
-    // unpoisoning first.
-    void setUnpoisonBeforeUpdate() { m_unpoisonBeforeUpdate = true; }
-#endif
-
 private:
     class CancellableTask : public WebTaskRunner::Task {
         WTF_MAKE_NONCOPYABLE(CancellableTask);
@@ -68,9 +83,6 @@ private:
     };
 
     OwnPtr<Closure> m_closure;
-#if defined(ADDRESS_SANITIZER)
-    bool m_unpoisonBeforeUpdate;
-#endif
     WeakPtrFactory<CancellableTaskFactory> m_weakPtrFactory;
 };