heap: Don't mutate NodeData during Trace

TraceAfterDispatch of NodeRareData was  node_lists_ in case it was
empty. This optimization could cause null dereferences when using
the return value of EnsureNodeLists().

Drive-by: MainThreadGCForbiddenScope is no longer needed once Trace
doesn't modify node_lists_ so removing that as well.

Bug: 1055194
Change-Id: I49e2419ac3aff3ff3e13042220a31fade9f4f06a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2078433
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Anton Bikineev <bikineev@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745403}

third_party/blink/renderer/core/dom/node.cc

@@ -375,7 +375,6 @@ ContainerNode* Node::parentNode() const {
 }
 
 NodeList* Node::childNodes() {
-  ThreadState::MainThreadGCForbiddenScope gc_forbidden;
   auto* this_node = DynamicTo<ContainerNode>(this);
   if (this_node)
     return EnsureRareData().EnsureNodeLists().EnsureChildNodeList(*this_node);

third_party/blink/renderer/core/dom/node_lists_node_data.h

@@ -43,7 +43,6 @@ class NodeListsNodeData final : public GarbageCollected<NodeListsNodeData> {
   }
 
   ChildNodeList* EnsureChildNodeList(ContainerNode& node) {
-    DCHECK(ThreadState::Current()->IsGCForbidden());
     if (child_node_list_)
       return To<ChildNodeList>(child_node_list_.Get());
     auto* list = MakeGarbageCollected<ChildNodeList>(node);
@@ -52,7 +51,6 @@ class NodeListsNodeData final : public GarbageCollected<NodeListsNodeData> {
   }
 
   EmptyNodeList* EnsureEmptyChildNodeList(Node& node) {
-    DCHECK(ThreadState::Current()->IsGCForbidden());
     if (child_node_list_)
       return To<EmptyNodeList>(child_node_list_.Get());
     auto* list = MakeGarbageCollected<EmptyNodeList>(node);
@@ -88,7 +86,6 @@ class NodeListsNodeData final : public GarbageCollected<NodeListsNodeData> {
   T* AddCache(ContainerNode& node,
               CollectionType collection_type,
               const AtomicString& name) {
-    DCHECK(ThreadState::Current()->IsGCForbidden());
     NodeListAtomicNameCacheMap::AddResult result = atomic_name_caches_.insert(
         std::make_pair(collection_type, name), nullptr);
     if (!result.is_new_entry) {
@@ -102,7 +99,6 @@ class NodeListsNodeData final : public GarbageCollected<NodeListsNodeData> {
 
   template <typename T>
   T* AddCache(ContainerNode& node, CollectionType collection_type) {
-    DCHECK(ThreadState::Current()->IsGCForbidden());
     NodeListAtomicNameCacheMap::AddResult result = atomic_name_caches_.insert(
         NamedNodeListKey(collection_type, CSSSelector::UniversalSelectorAtom()),
         nullptr);
@@ -124,7 +120,6 @@ class NodeListsNodeData final : public GarbageCollected<NodeListsNodeData> {
   TagCollectionNS* AddCache(ContainerNode& node,
                             const AtomicString& namespace_uri,
                             const AtomicString& local_name) {
-    DCHECK(ThreadState::Current()->IsGCForbidden());
     QualifiedName name(g_null_atom, local_name, namespace_uri);
     TagCollectionNSCache::AddResult result =
         tag_collection_ns_caches_.insert(name, nullptr);
@@ -183,7 +178,6 @@ class NodeListsNodeData final : public GarbageCollected<NodeListsNodeData> {
 
 template <typename Collection>
 inline Collection* ContainerNode::EnsureCachedCollection(CollectionType type) {
-  ThreadState::MainThreadGCForbiddenScope gc_forbidden;
   return EnsureNodeLists().AddCache<Collection>(*this, type);
 }
 
@@ -191,7 +185,6 @@ template <typename Collection>
 inline Collection* ContainerNode::EnsureCachedCollection(
     CollectionType type,
     const AtomicString& name) {
-  ThreadState::MainThreadGCForbiddenScope gc_forbidden;
   return EnsureNodeLists().AddCache<Collection>(*this, type, name);
 }
 
@@ -201,7 +194,6 @@ inline Collection* ContainerNode::EnsureCachedCollection(
     const AtomicString& namespace_uri,
     const AtomicString& local_name) {
   DCHECK_EQ(type, kTagCollectionNSType);
-  ThreadState::MainThreadGCForbiddenScope gc_forbidden;
   return EnsureNodeLists().AddCache(*this, namespace_uri, local_name);
 }
 

third_party/blink/renderer/core/dom/node_rare_data.cc

@@ -112,11 +112,7 @@ void NodeRareData::TraceAfterDispatch(blink::Visitor* visitor) {
   visitor->Trace(mutation_observer_data_);
   visitor->Trace(flat_tree_node_data_);
   visitor->Trace(node_layout_data_);
-  // Do not keep empty NodeListsNodeData objects around.
-  if (node_lists_ && node_lists_->IsEmpty())
-    node_lists_.Clear();
-  else
-    visitor->Trace(node_lists_);
+  visitor->Trace(node_lists_);
   NodeData::TraceAfterDispatch(visitor);
 }
 

third_party/blink/renderer/core/dom/node_rare_data.h

@@ -146,7 +146,6 @@ class GC_PLUGIN_IGNORE("Manual dispatch implemented in NodeData.") NodeRareData
   // wrapped with a ThreadState::GCForbiddenScope in order to avoid an
   // initialized node_lists_ is cleared by NodeRareData::TraceAfterDispatch().
   NodeListsNodeData& EnsureNodeLists() {
-    DCHECK(ThreadState::Current()->IsGCForbidden());
     if (!node_lists_)
       return CreateNodeLists();
     return *node_lists_;

third_party/blink/renderer/platform/heap/thread_state.h

@@ -182,7 +182,6 @@ class PLATFORM_EXPORT ThreadState final {
   class AtomicPauseScope;
   class GCForbiddenScope;
   class LsanDisabledScope;
-  class MainThreadGCForbiddenScope;
   class NoAllocationScope;
   class StatisticsCollector;
   struct Statistics;

third_party/blink/renderer/platform/heap/thread_state_scopes.h

@@ -47,19 +47,6 @@ class ThreadState::SweepForbiddenScope final {
   ThreadState* const state_;
 };
 
-class ThreadState::MainThreadGCForbiddenScope final {
-  STACK_ALLOCATED();
-
- public:
-  MainThreadGCForbiddenScope() : thread_state_(ThreadState::MainThreadState()) {
-    thread_state_->EnterGCForbiddenScope();
-  }
-  ~MainThreadGCForbiddenScope() { thread_state_->LeaveGCForbiddenScope(); }
-
- private:
-  ThreadState* const thread_state_;
-};
-
 class ThreadState::GCForbiddenScope final {
   STACK_ALLOCATED();