VisitPersistentHandle should not call visitDOMWrapper

Source/bindings/core/v8/V8GCController.cpp

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 237 matching lines @@
         , m_liveRootGroupIdSet(false)
         , m_constructRetainedObjectInfos(constructRetainedObjectInfos)
     {
     }
 
     virtual void VisitPersistentHandle(v8::Persistent<v8::Value>* value, uint16_t classId) override
     {
         if (classId != WrapperTypeInfo::NodeClassId && classId != WrapperTypeInfo::ObjectClassId)
             return;
 
-        v8::Local<v8::Object> wrapper = v8::Local<v8::Object>::New(m_isolate, v8::Persistent<v8::Object>::Cast(*value));
-        ASSERT(V8DOMWrapper::hasInternalFieldsSet(wrapper));
-
         if (value->IsIndependent())
             return;
 
-        const WrapperTypeInfo* type = toWrapperTypeInfo(wrapper);
+        v8::Persistent<v8::Object>& persistentWrapper = v8::Persistent<v8::Object>::Cast(*value);
+        v8::Local<v8::Object> wrapper = v8::Local<v8::Object>::New(m_isolate, persistentWrapper);
+        ASSERT(V8DOMWrapper::hasInternalFieldsSet(wrapper));
 
-        ActiveDOMObject* activeDOMObject = type->toActiveDOMObject(wrapper);
+        const WrapperTypeInfo* wrapperTypeInfo = toWrapperTypeInfo(wrapper);
+
+        ActiveDOMObject* activeDOMObject = wrapperTypeInfo->toActiveDOMObject(wrapper);
         if (activeDOMObject && activeDOMObject->hasPendingActivity()) {
             m_isolate->SetObjectGroupId(*value, liveRootId());
             ++m_domObjectsWithPendingActivity;
         }
 
         if (classId == WrapperTypeInfo::NodeClassId) {
             ASSERT(V8Node::hasInstance(wrapper, m_isolate));
             Node* node = V8Node::toImpl(wrapper);
             if (node->hasEventListeners())
-                addReferencesForNodeWithEventListeners(m_isolate, node, v8::Persistent<v8::Object>::Cast(*value));
+                addReferencesForNodeWithEventListeners(m_isolate, node, persistentWrapper);
             Node* root = V8GCController::opaqueRootForGC(m_isolate, node);
             m_isolate->SetObjectGroupId(*value, v8::UniqueId(reinterpret_cast<intptr_t>(root)));
             if (m_constructRetainedObjectInfos)
                 m_groupsWhichNeedRetainerInfo.append(root);
         } else if (classId == WrapperTypeInfo::ObjectClassId) {
-            type->visitDOMWrapper(m_isolate, toScriptWrappable(wrapper), v8::Persistent<v8::Object>::Cast(*value));
+            m_wrappersToBeVisited.append(value);
         } else {
             ASSERT_NOT_REACHED();
         }
     }
 
     void notifyFinished()
     {
+        for (auto& value : m_wrappersToBeVisited) {
+            ASSERT(!value->IsIndependent());

[haraken, 2015/06/20 09:29:49]

This CL is not correct. The access to |value| causes stack-use-after-free,
because v8::Persistent<v8::Value>* is pointing to an on-stack pointer. How can I
avoid the issue?

One way to avoid this would be to copy the persistent handles into
m_wrappersToBeVisited. However persistent handles are not copyable...

Jochen: Any idea?

[jochen (gone - plz use gerrit), 2015/06/22 08:45:49]

what about unwrapping the handle and collecting ScriptWrappable* ?

+            v8::Persistent<v8::Object>& persistentWrapper = v8::Persistent<v8::Object>::Cast(*value);
+            v8::Local<v8::Object> wrapper = v8::Local<v8::Object>::New(m_isolate, persistentWrapper);
+            const WrapperTypeInfo* wrapperTypeInfo = toWrapperTypeInfo(wrapper);
+            wrapperTypeInfo->visitDOMWrapper(m_isolate, toScriptWrappable(wrapper), persistentWrapper);

[jochen (gone - plz use gerrit), 2015/06/22 08:45:49]

can this create new Persistent<> handles that need to be in the right object
group?

+        }
+
         if (!m_constructRetainedObjectInfos)
             return;
         std::sort(m_groupsWhichNeedRetainerInfo.begin(), m_groupsWhichNeedRetainerInfo.end());
         Node* alreadyAdded = 0;
         v8::HeapProfiler* profiler = m_isolate->GetHeapProfiler();
         for (size_t i = 0; i < m_groupsWhichNeedRetainerInfo.size(); ++i) {
             Node* root = m_groupsWhichNeedRetainerInfo[i];
             if (root != alreadyAdded) {
                 profiler->SetRetainedObjectInfo(v8::UniqueId(reinterpret_cast<intptr_t>(root)), new RetainedDOMInfo(root));
                 alreadyAdded = root;
@@ skipping 12 matching lines @@
         if (!m_liveRootGroupIdSet) {
             m_isolate->SetObjectGroupId(liveRoot, id);
             m_liveRootGroupIdSet = true;
             ++m_domObjectsWithPendingActivity;
         }
         return id;
     }
 
     v8::Isolate* m_isolate;
     WillBePersistentHeapVector<RawPtrWillBeMember<Node>> m_groupsWhichNeedRetainerInfo;
+    Vector<v8::Persistent<v8::Value>*> m_wrappersToBeVisited;
     int m_domObjectsWithPendingActivity;
     bool m_liveRootGroupIdSet;
     bool m_constructRetainedObjectInfos;
 };
 
 static unsigned long long usedHeapSize(v8::Isolate* isolate)
 {
     v8::HeapStatistics heapStatistics;
     isolate->GetHeapStatistics(&heapStatistics);
     return heapStatistics.used_heap_size();
@@ skipping 169 matching lines @@
     Visitor* m_visitor;
 };
 
 void V8GCController::traceDOMWrappers(v8::Isolate* isolate, Visitor* visitor)
 {
     DOMWrapperTracer tracer(visitor);
     v8::V8::VisitHandlesWithClassIds(isolate, &tracer);
 }
 
 } // namespace blink