Oilpan: fix PromiseTracker's handling of its persistent promise wrappers.

The handling of the PromiseDataWrapper in r181662 wasn't complete, as it
failed to keep an Oilpan-reachable persistent to the GCed wrapper object,
risking premature finalization. For Oilpan also, this wrapper object should 
only keep its references weakly alive.

To make the tracing of these relationships work out, PromiseTracker is
no longer a part object, but heap allocated.

R=haraken
BUG=340522

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

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

third_party/WebKit/Source/core/inspector/InspectorDebuggerAgent.cpp

@@ -130,6 +130,7 @@ InspectorDebuggerAgent::InspectorDebuggerAgent(InjectedScriptManager* injectedSc
     , m_skipAllPauses(false)
     , m_skipContentScripts(false)
     , m_asyncCallStackTracker(adoptPtrWillBeNoop(new AsyncCallStackTracker()))
+    , m_promiseTracker(PromiseTracker::create())
 {
 }
 
@@ -232,7 +233,7 @@ void InspectorDebuggerAgent::restore()
             m_state->setBoolean(DebuggerAgentState::skipAllPauses, false);
         }
         asyncCallStackTracker().setAsyncCallStackDepth(m_state->getLong(DebuggerAgentState::asyncCallStackDepth));
-        m_promiseTracker.setEnabled(m_state->getBoolean(DebuggerAgentState::promiseTrackerEnabled));
+        promiseTracker().setEnabled(m_state->getBoolean(DebuggerAgentState::promiseTrackerEnabled));
     }
 }
 
@@ -922,9 +923,9 @@ void InspectorDebuggerAgent::didReceiveV8AsyncTaskEvent(ExecutionContext* contex
 
 void InspectorDebuggerAgent::didReceiveV8PromiseEvent(ScriptState* scriptState, v8::Handle<v8::Object> promise, v8::Handle<v8::Value> parentPromise, int status)
 {
-    if (!m_promiseTracker.isEnabled())
+    if (!promiseTracker().isEnabled())
         return;
-    m_promiseTracker.didReceiveV8PromiseEvent(scriptState, promise, parentPromise, status);
+    promiseTracker().didReceiveV8PromiseEvent(scriptState, promise, parentPromise, status);
 }
 
 void InspectorDebuggerAgent::pause(ErrorString*)
@@ -1173,20 +1174,20 @@ void InspectorDebuggerAgent::setAsyncCallStackDepth(ErrorString*, int depth)
 void InspectorDebuggerAgent::enablePromiseTracker(ErrorString*)
 {
     m_state->setBoolean(DebuggerAgentState::promiseTrackerEnabled, true);
-    m_promiseTracker.setEnabled(true);
+    promiseTracker().setEnabled(true);
 }
 
 void InspectorDebuggerAgent::disablePromiseTracker(ErrorString*)
 {
     m_state->setBoolean(DebuggerAgentState::promiseTrackerEnabled, false);
-    m_promiseTracker.setEnabled(false);
+    promiseTracker().setEnabled(false);
 }
 
 void InspectorDebuggerAgent::getPromises(ErrorString*, RefPtr<Array<PromiseDetails> >& promises)
 {
-    if (!m_promiseTracker.isEnabled())
+    if (!promiseTracker().isEnabled())
         return;
-    promises = m_promiseTracker.promises();
+    promises = promiseTracker().promises();
 }
 
 void InspectorDebuggerAgent::scriptExecutionBlockedByCSP(const String& directiveText)
@@ -1451,7 +1452,7 @@ void InspectorDebuggerAgent::clear()
     m_scripts.clear();
     m_breakpointIdToDebugServerBreakpointIds.clear();
     asyncCallStackTracker().clear();
-    m_promiseTracker.clear();
+    promiseTracker().clear();
     m_continueToLocationBreakpointId = String();
     clearBreakDetails();
     m_javaScriptPauseScheduled = false;
@@ -1494,7 +1495,7 @@ void InspectorDebuggerAgent::reset()
     m_scripts.clear();
     m_breakpointIdToDebugServerBreakpointIds.clear();
     asyncCallStackTracker().clear();
-    m_promiseTracker.clear();
+    promiseTracker().clear();
     if (m_frontend)
         m_frontend->globalObjectCleared();
 }

third_party/WebKit/Source/core/inspector/InspectorDebuggerAgent.h

@@ -244,7 +244,8 @@ private:
     String sourceMapURLForScript(const Script&, CompileResult);
 
     PassRefPtrWillBeRawPtr<JavaScriptCallFrame> topCallFrameSkipUnknownSources(String* scriptURL, bool* isBlackboxed);
-    AsyncCallStackTracker& asyncCallStackTracker() { return *m_asyncCallStackTracker; };
+    AsyncCallStackTracker& asyncCallStackTracker() const { return *m_asyncCallStackTracker; };
+    PromiseTracker& promiseTracker() const { return *m_promiseTracker; }
 
     typedef HashMap<String, Script> ScriptsMap;
     typedef HashMap<String, Vector<String> > BreakpointIdToDebugServerBreakpointIdsMap;
@@ -272,7 +273,7 @@ private:
     bool m_skipContentScripts;
     OwnPtr<ScriptRegexp> m_cachedSkipStackRegExp;
     OwnPtrWillBeMember<AsyncCallStackTracker> m_asyncCallStackTracker;
-    PromiseTracker m_promiseTracker;
+    OwnPtrWillBeMember<PromiseTracker> m_promiseTracker;
 };
 
 } // namespace blink

third_party/WebKit/Source/core/inspector/PromiseTracker.cpp

@@ -27,18 +27,16 @@ public:
     int promiseHash() const { return m_promiseHash; }
     ScopedPersistent<v8::Object>& promise() { return m_promise; }
 
-    void trace(Visitor* visitor)
+#if !ENABLE(OILPAN)
+    WeakPtr<PromiseData> createWeakPtr()
     {
-        visitor->trace(m_callStack);
+        return m_weakPtrFactory.createWeakPtr();
     }
+#endif
 
-    WeakPtrWillBeRawPtr<PromiseData> createWeakPtr()
+    void trace(Visitor* visitor)
     {
-#if ENABLE(OILPAN)
-        return this;
-#else
-        return m_weakPtrFactory.createWeakPtr();
-#endif
+        visitor->trace(m_callStack);
     }
 
 private:
@@ -81,57 +79,68 @@ namespace {
 
 class PromiseDataWrapper FINAL : public NoBaseWillBeGarbageCollected<PromiseDataWrapper> {
 public:
-    static PassOwnPtrWillBeRawPtr<PromiseDataWrapper> create(PromiseTracker::PromiseData* data, PromiseTracker::PromiseDataMap* map)
+    static PassOwnPtrWillBeRawPtr<PromiseDataWrapper> create(PromiseTracker::PromiseData* data, PromiseTracker* tracker)
     {
-        return adoptPtrWillBeNoop(new PromiseDataWrapper(data->createWeakPtr(), map));
+#if ENABLE(OILPAN)
+        return new PromiseDataWrapper(data, tracker);
+#else
+        return adoptPtr(new PromiseDataWrapper(data->createWeakPtr(), tracker));
+#endif
     }
 
+#if ENABLE(OILPAN)
+    static void didRemovePromise(const v8::WeakCallbackData<v8::Object, Persistent<PromiseDataWrapper> >& data)
+#else
     static void didRemovePromise(const v8::WeakCallbackData<v8::Object, PromiseDataWrapper>& data)
+#endif
     {
-        OwnPtrWillBeRawPtr<PromiseDataWrapper> wrapper = adoptPtrWillBeNoop(data.GetParameter());
+#if ENABLE(OILPAN)
+        OwnPtr<Persistent<PromiseDataWrapper> > persistentWrapper = adoptPtr(data.GetParameter());
+        RawPtr<PromiseDataWrapper> wrapper = *persistentWrapper;
+#else
+        OwnPtr<PromiseDataWrapper> wrapper = adoptPtr(data.GetParameter());
+#endif
         WeakPtrWillBeRawPtr<PromiseTracker::PromiseData> promiseData = wrapper->m_data;
-        if (!promiseData)
+        if (!promiseData || !wrapper->m_tracker)
             return;
-        PromiseTracker::PromiseDataMap* map = wrapper->m_promiseDataMap;
+        PromiseTracker::PromiseDataMap& map = wrapper->m_tracker->promiseDataMap();
         int promiseHash = promiseData->promiseHash();
-        PromiseTracker::PromiseDataVector* vector = &map->find(promiseHash)->value;
+        PromiseTracker::PromiseDataVector* vector = &map.find(promiseHash)->value;
         int index = indexOf(vector, promiseData->promise());
         ASSERT(index >= 0);
         vector->remove(index);
-        if (vector->size() == 0)
-            map->remove(promiseHash);
+        if (vector->isEmpty())
+            map.remove(promiseHash);
     }
 
     void trace(Visitor* visitor)
     {
 #if ENABLE(OILPAN)
         visitor->trace(m_data);
-        visitor->trace(m_promiseDataMap);
+        visitor->trace(m_tracker);
 #endif
     }
 
 private:
-    PromiseDataWrapper(WeakPtrWillBeRawPtr<PromiseTracker::PromiseData> data, PromiseTracker::PromiseDataMap* map)
+    PromiseDataWrapper(WeakPtrWillBeRawPtr<PromiseTracker::PromiseData> data, PromiseTracker* tracker)
         : m_data(data)
-        , m_promiseDataMap(map)
+        , m_tracker(tracker)
     {
     }
 
-    WeakPtrWillBeMember<PromiseTracker::PromiseData> m_data;
-    RawPtrWillBeMember<PromiseTracker::PromiseDataMap> m_promiseDataMap;
+    WeakPtrWillBeWeakMember<PromiseTracker::PromiseData> m_data;
+    RawPtrWillBeWeakMember<PromiseTracker> m_tracker;
 };
 
 }
 
 PromiseTracker::PromiseTracker()
-    : m_isEnabled(false)
-    , m_circularSequentialId(0)
+    : m_circularSequentialId(0)
+    , m_isEnabled(false)
 {
 }
 
-PromiseTracker::~PromiseTracker()
-{
-}
+DEFINE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(PromiseTracker);
 
 void PromiseTracker::trace(Visitor* visitor)
 {
@@ -170,16 +179,22 @@ PassRefPtrWillBeRawPtr<PromiseTracker::PromiseData> PromiseTracker::createPromis
     else
         vector = &m_promiseDataMap.add(promiseHash, PromiseDataVector()).storedValue->value;
 
-    RefPtrWillBeRawPtr<PromiseData> data = nullptr;
     int index = indexOf(vector, ScopedPersistent<v8::Object>(isolate, promise));
-    if (index == -1) {
-        data = PromiseData::create(isolate, promiseHash, circularSequentialId(), promise);
-        OwnPtrWillBeRawPtr<PromiseDataWrapper> wrapper = PromiseDataWrapper::create(data.get(), &m_promiseDataMap);
-        data->m_promise.setWeak(wrapper.leakPtr(), &PromiseDataWrapper::didRemovePromise);
-        vector->append(data);
-    } else {
-        data = vector->at(index);
-    }
+    if (index != -1)
+        return vector->at(index);
+
+    // FIXME: Consider using the ScriptState's DOMWrapperWorld instead
+    // to handle the lifetime of PromiseDataWrapper, avoiding all this
+    // manual labor to achieve the same, with and without Oilpan.
+    RefPtrWillBeRawPtr<PromiseData> data = PromiseData::create(isolate, promiseHash, circularSequentialId(), promise);
+    OwnPtrWillBeRawPtr<PromiseDataWrapper> dataWrapper = PromiseDataWrapper::create(data.get(), this);
+#if ENABLE(OILPAN)
+    OwnPtr<Persistent<PromiseDataWrapper> > wrapper = adoptPtr(new Persistent<PromiseDataWrapper>(dataWrapper));
+#else
+    OwnPtr<PromiseDataWrapper> wrapper = dataWrapper.release();
+#endif
+    data->m_promise.setWeak(wrapper.leakPtr(), &PromiseDataWrapper::didRemovePromise);
+    vector->append(data);
 
     return data.release();
 }

third_party/WebKit/Source/core/inspector/PromiseTracker.h

@@ -17,12 +17,14 @@ namespace blink {
 
 class ScriptState;
 
-class PromiseTracker FINAL {
+class PromiseTracker FINAL : public NoBaseWillBeGarbageCollected<PromiseTracker> {
     WTF_MAKE_NONCOPYABLE(PromiseTracker);
-    DISALLOW_ALLOCATION();
+    DECLARE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(PromiseTracker);
 public:
-    PromiseTracker();
-    ~PromiseTracker();
+    static PassOwnPtrWillBeRawPtr<PromiseTracker> create()
+    {
+        return adoptPtrWillBeNoop(new PromiseTracker());
+    }
 
     bool isEnabled() const { return m_isEnabled; }
     void setEnabled(bool);
@@ -40,13 +42,17 @@ public:
 
     void trace(Visitor*);
 
+    PromiseDataMap& promiseDataMap() { return m_promiseDataMap; }
+
 private:
+    PromiseTracker();
+
     int circularSequentialId();
     PassRefPtrWillBeRawPtr<PromiseData> createPromiseDataIfNeeded(v8::Isolate*, v8::Handle<v8::Object> promise);
 
-    bool m_isEnabled;
     int m_circularSequentialId;
     PromiseDataMap m_promiseDataMap;
+    bool m_isEnabled;
 };
 
 } // namespace blink