Trace ScriptWrappableVisitor.m_markingDeque by oilpan gc

third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp

Index: third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp b/third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp
index b65fb47785e5911a914da324b23f2d3edd4b4f06..b72bdde05e9a4b05f69a633b33e26cb8a6e1dcfb 100644
--- a/third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp
@@ -38,20 +38,33 @@ void ScriptWrappableVisitor::TraceEpilogue()
     m_tracingInProgress = false;
 }
 
+void ScriptWrappableVisitor::RegisterV8Reference(const std::pair<void*, void*>& internalFields)
+{
+    if (!m_tracingInProgress) {
+        return;

[haraken, 2016/06/09 06:28:31]

Shall we add DCHECK(m_markingDeque.isEmpty())?

[Marcel Hlopko, 2016/06/09 09:00:50]

As far as I meant it, no. This method will be called multiple times, everytime
v8 discovers new wrappers, and everytime blink associates new wrappers. Calls to
AdvanceTracing happen independently to RegisterV8Reference.

[haraken, 2016/06/09 11:29:03]

Ah, makes sense.

+    }
+
+    WrapperTypeInfo* wrapperTypeInfo = reinterpret_cast<WrapperTypeInfo*>(internalFields.first);
+    if (wrapperTypeInfo->ginEmbedder != gin::GinEmbedder::kEmbedderBlink) {
+        return;
+    }
+    DCHECK(wrapperTypeInfo->wrapperClassId == WrapperTypeInfo::NodeClassId
+        || wrapperTypeInfo->wrapperClassId == WrapperTypeInfo::ObjectClassId);
+
+    ScriptWrappable* scriptWrappable = reinterpret_cast<ScriptWrappable*>(internalFields.second);
+    if (wrapperTypeInfo->getHeapObjectHeader(scriptWrappable)->isWrapperHeaderMarked()) {
+        return;
+    }
+
+    m_markingDeque.append(scriptWrappable);
+}
+
 void ScriptWrappableVisitor::RegisterV8References(const std::vector<std::pair<void*, void*>>& internalFieldsOfPotentialWrappers)
 {
     // TODO(hlopko): Visit the vector in the V8 instead of passing it over if
     // there is no performance impact
-    for (auto pair : internalFieldsOfPotentialWrappers) {
-        WrapperTypeInfo* wrapperTypeInfo = reinterpret_cast<WrapperTypeInfo*>(pair.first);
-        if (wrapperTypeInfo->ginEmbedder != gin::GinEmbedder::kEmbedderBlink)
-            continue;
-
-        ScriptWrappable* scriptWrappable = reinterpret_cast<ScriptWrappable*>(pair.second);
-        DCHECK(wrapperTypeInfo->wrapperClassId == WrapperTypeInfo::NodeClassId
-            || wrapperTypeInfo->wrapperClassId == WrapperTypeInfo::ObjectClassId);
-
-        m_markingDeque.append(scriptWrappable);
+    for (auto& pair : internalFieldsOfPotentialWrappers) {
+        RegisterV8Reference(pair);
     }
 }
 
@@ -65,8 +78,11 @@ bool ScriptWrappableVisitor::AdvanceTracing(double deadlineInMs, v8::EmbedderHea
         }
 
         const ScriptWrappable* scriptWrappable = m_markingDeque.takeFirst();
-        markWrapperHeader(scriptWrappable);
-        scriptWrappable->traceWrappers(this);
+        // there might be nullptrs in the deque after oilpan gcs
+        if (scriptWrappable) {
+            markWrapperHeader(scriptWrappable);
+            scriptWrappable->traceWrappers(this);
+        }
     }
     return true;
 }
@@ -124,4 +140,24 @@ WRAPPER_VISITOR_SPECIAL_CLASSES(DEFINE_DISPATCH_TRACE_WRAPPERS);
 
 #undef DEFINE_DISPATCH_TRACE_WRAPPERS
 
+void ScriptWrappableVisitor::invalidateDeadObjectsInMarkingDeque()
+{
+    for (auto it = m_markingDeque.begin(); it != m_markingDeque.end(); ++it) {

[haraken, 2016/06/09 06:28:31]

Just to confirm: m_markingDeque contains only ScriptWrappables created on this
thread, right? If it's not guaranteed, it will break assumptions in Oilpan's
per-thread heap.

[Marcel Hlopko, 2016/06/09 09:00:50]

Yes, as long as an isolate corresponds to a thread.

+        const ScriptWrappable* scriptWrappable = *it;
+        if (!scriptWrappable->wrapperTypeInfo()->
+            getHeapObjectHeader(const_cast<ScriptWrappable*>(scriptWrappable))->
+            isMarked()) {
+            *it = nullptr;
+        }
+    }
+}
+
+void ScriptWrappableVisitor::invalidateDeadObjectsInMarkingDeque(v8::Isolate* isolate)
+{
+    ScriptWrappableVisitor* scriptWrappableVisitor = V8PerIsolateData::from(isolate)->scriptWrappableVisitor();
+    if (scriptWrappableVisitor) {
+        scriptWrappableVisitor->invalidateDeadObjectsInMarkingDeque();
+    }
+}
+
 } // namespace blink