Disallow heap objects containing unsafe on-heap iterators.

Do not allow BlinkGC managed objects to include unsafe iterators of
other heap objects; that is, do not allow them to keep iterator
part objects as fields.

These iterators contain untraced references, which is in general
unsafe practice and breaks the general rule that all heap references
must be known to the GC infrastructure, and be marked and traced
through.

This applies to all heap collection iterators but HeapListHashSet<>'s,
which can be safely traced. It is also the only collection iterator
which is kept as a field of an on-heap object (CSSSegmentedFontFace.)

R=haraken
BUG=672030

Review-Url: https://codereview.chromium.org/2588943002
Cr-Commit-Position: refs/heads/master@{#439784}

tools/clang/blink_gc_plugin/CheckFieldsVisitor.cpp

@@ -47,6 +47,14 @@ void CheckFieldsVisitor::AtMember(Member* edge) {
   invalid_fields_.push_back(std::make_pair(current_, kMemberInUnmanaged));
 }
 
+void CheckFieldsVisitor::AtIterator(Iterator* edge) {
+  if (!managed_host_)
+    return;
+
+  if (edge->IsUnsafe())
+    invalid_fields_.push_back(std::make_pair(current_, kIteratorToGCManaged));
+}
+
 void CheckFieldsVisitor::AtValue(Value* edge) {
   // TODO: what should we do to check unions?
   if (edge->value()->record()->isUnion())

tools/clang/blink_gc_plugin/CheckFieldsVisitor.h

@@ -28,7 +28,8 @@ class CheckFieldsVisitor : public RecursiveEdgeVisitor {
     kMemberToGCUnmanaged,
     kMemberInUnmanaged,
     kPtrFromHeapToStack,
-    kGCDerivedPartObject
+    kGCDerivedPartObject,
+    kIteratorToGCManaged,
   };
 
   using Errors = std::vector<std::pair<FieldPoint*, Error>>;
@@ -42,6 +43,7 @@ class CheckFieldsVisitor : public RecursiveEdgeVisitor {
   void AtMember(Member* edge) override;
   void AtValue(Value* edge) override;
   void AtCollection(Collection* edge) override;
+  void AtIterator(Iterator*) override;
 
  private:
   Error InvalidSmartPtr(Edge* ptr);

tools/clang/blink_gc_plugin/Config.h

@@ -32,6 +32,10 @@ const char kVisitorVarName[] = "visitor";
 const char kAdjustAndMarkName[] = "adjustAndMark";
 const char kIsHeapObjectAliveName[] = "isHeapObjectAlive";
 const char kIsEagerlyFinalizedName[] = "IsEagerlyFinalizedMarker";
+const char kConstIteratorName[] = "const_iterator";
+const char kIteratorName[] = "iterator";
+const char kConstReverseIteratorName[] = "const_reverse_iterator";
+const char kReverseIteratorName[] = "reverse_iterator";
 
 class Config {
  public:
@@ -101,6 +105,16 @@ class Config {
            name == "PersistentHeapHashMap";
   }
 
+  static bool IsGCCollectionWithUnsafeIterator(const std::string& name) {
+    if (!IsGCCollection(name))
+      return false;
+    // The list hash set iterators refer to the set, not the
+    // backing store and are consequently safe.
+    if (name == "HeapListHashSet" || name == "PersistentHeapListHashSet")
+      return false;
+    return true;
+  }
+
   static bool IsHashMap(const std::string& name) {
     return name == "HashMap" ||
            name == "HeapHashMap" ||
@@ -131,6 +145,11 @@ class Config {
            IsGCMixinBase(name);
   }
 
+  static bool IsIterator(const std::string& name) {
+    return name == kIteratorName || name == kConstIteratorName ||
+           name == kReverseIteratorName || name == kConstReverseIteratorName;
+  }
+
   // Returns true of the base classes that do not need a vtable entry for trace
   // because they cannot possibly initiate a GC during construction.
   static bool IsSafePolymorphicBase(const std::string& name) {

tools/clang/blink_gc_plugin/DiagnosticsReporter.cpp

@@ -147,6 +147,9 @@ const char kBaseClassMustDeclareVirtualTrace[] =
     "[blink-gc] Left-most base class %0 of derived class %1"
     " must define a virtual trace method.";
 
+const char kIteratorToGCManagedCollectionNote[] =
+    "[blink-gc] Iterator field %0 to a GC managed collection declared here:";
+
 } // namespace
 
 DiagnosticBuilder DiagnosticsReporter::ReportDiagnostic(
@@ -253,6 +256,8 @@ DiagnosticsReporter::DiagnosticsReporter(
       DiagnosticsEngine::Note, kOverriddenNonVirtualTraceNote);
   diag_manual_dispatch_method_note_ = diagnostic_.getCustomDiagID(
       DiagnosticsEngine::Note, kManualDispatchMethodNote);
+  diag_iterator_to_gc_managed_collection_note_ = diagnostic_.getCustomDiagID(
+      DiagnosticsEngine::Note, kIteratorToGCManagedCollectionNote);
 }
 
 bool DiagnosticsReporter::hasErrorOccurred() const
@@ -343,6 +348,8 @@ void DiagnosticsReporter::ClassContainsInvalidFields(
       note = diag_stack_allocated_field_note_;
     } else if (error.second == CheckFieldsVisitor::kGCDerivedPartObject) {
       note = diag_part_object_to_gc_derived_class_note_;
+    } else if (error.second == CheckFieldsVisitor::kIteratorToGCManaged) {
+      note = diag_iterator_to_gc_managed_collection_note_;
     } else {
       assert(false && "Unknown field error");
     }

tools/clang/blink_gc_plugin/DiagnosticsReporter.h

@@ -135,7 +135,7 @@ class DiagnosticsReporter {
   unsigned diag_field_requires_finalization_note_;
   unsigned diag_overridden_non_virtual_trace_note_;
   unsigned diag_manual_dispatch_method_note_;
-
+  unsigned diag_iterator_to_gc_managed_collection_note_;
 };
 
 #endif // TOOLS_BLINK_GC_PLUGIN_DIAGNOSTICS_REPORTER_H_

tools/clang/blink_gc_plugin/Edge.cpp

@@ -23,6 +23,7 @@ void RecursiveEdgeVisitor::AtWeakMember(WeakMember*) {}
 void RecursiveEdgeVisitor::AtPersistent(Persistent*) {}
 void RecursiveEdgeVisitor::AtCrossThreadPersistent(CrossThreadPersistent*) {}
 void RecursiveEdgeVisitor::AtCollection(Collection*) {}
+void RecursiveEdgeVisitor::AtIterator(Iterator*) {}
 
 void RecursiveEdgeVisitor::VisitValue(Value* e) {
   AtValue(e);
@@ -90,3 +91,7 @@ void RecursiveEdgeVisitor::VisitCollection(Collection* e) {
   e->AcceptMembers(this);
   Leave();
 }
+
+void RecursiveEdgeVisitor::VisitIterator(Iterator* e) {
+  AtIterator(e);
+}

tools/clang/blink_gc_plugin/Edge.h

@@ -16,6 +16,7 @@ class RecordInfo;
 class Edge;
 class Collection;
 class CrossThreadPersistent;
+class Iterator;
 class Member;
 class OwnPtr;
 class Persistent;
@@ -39,6 +40,7 @@ class EdgeVisitor {
   virtual void VisitPersistent(Persistent*) {}
   virtual void VisitCrossThreadPersistent(CrossThreadPersistent*) {}
   virtual void VisitCollection(Collection*) {}
+  virtual void VisitIterator(Iterator*) {}
 };
 
 // Recursive edge visitor. The traversed path is accessible in context.
@@ -55,6 +57,7 @@ class RecursiveEdgeVisitor : public EdgeVisitor {
   void VisitPersistent(Persistent*) override;
   void VisitCrossThreadPersistent(CrossThreadPersistent*) override;
   void VisitCollection(Collection*) override;
+  void VisitIterator(Iterator*) override;
 
  protected:
   typedef std::deque<Edge*> Context;
@@ -74,6 +77,7 @@ class RecursiveEdgeVisitor : public EdgeVisitor {
   virtual void AtPersistent(Persistent*);
   virtual void AtCrossThreadPersistent(CrossThreadPersistent*);
   virtual void AtCollection(Collection*);
+  virtual void AtIterator(Iterator*);
 
  private:
   Context context_;
@@ -281,4 +285,30 @@ class Collection : public Edge {
   bool is_root_;
 };
 
+// An iterator edge is a direct edge to some iterator type.
+class Iterator : public Edge {
+ public:
+  Iterator(RecordInfo* info, bool on_heap, bool is_unsafe)
+      : info_(info), on_heap_(on_heap), is_unsafe_(is_unsafe) {}
+  ~Iterator() {}
+
+  void Accept(EdgeVisitor* visitor) { visitor->VisitIterator(this); }
+  LivenessKind Kind() override { return kStrong; }
+  bool NeedsFinalization() { return false; }
+  TracingStatus NeedsTracing(NeedsTracingOption) {
+    if (on_heap_)
+      return TracingStatus::Needed();
+    return TracingStatus::Unneeded();
+  }
+
+  RecordInfo* info() const { return info_; }
+
+  bool IsUnsafe() const { return is_unsafe_; }
+
+ private:
+  RecordInfo* info_;
+  bool on_heap_;
+  bool is_unsafe_;
+};
+
 #endif  // TOOLS_BLINK_GC_PLUGIN_EDGE_H_

tools/clang/blink_gc_plugin/RecordInfo.cpp

@@ -113,10 +113,10 @@ void RecordInfo::walkBases() {
   // have a "GC base name", so are to be included and considered.
   SmallVector<const CXXRecordDecl*, 8> queue;
 
-  const CXXRecordDecl *base_record = record();
+  const CXXRecordDecl* base_record = record();
   while (true) {
     for (const auto& it : base_record->bases()) {
-      const RecordType *type = it.getType()->getAs<RecordType>();
+      const RecordType* type = it.getType()->getAs<RecordType>();
       CXXRecordDecl* base;
       if (!type)
         base = GetDependentTemplatedDecl(*it.getType());
@@ -171,17 +171,19 @@ bool RecordInfo::IsGCAllocated() {
 }
 
 bool RecordInfo::IsEagerlyFinalized() {
-  if (is_eagerly_finalized_ == kNotComputed) {
-    is_eagerly_finalized_ = kFalse;
-    if (IsGCFinalized()) {
-      for (Decl* decl : record_->decls()) {
-        if (TypedefDecl* typedef_decl = dyn_cast<TypedefDecl>(decl)) {
-          if (typedef_decl->getNameAsString() == kIsEagerlyFinalizedName) {
-            is_eagerly_finalized_ = kTrue;
-            break;
-          }
-        }
-      }
+  if (is_eagerly_finalized_ != kNotComputed)
+    return is_eagerly_finalized_;
+
+  is_eagerly_finalized_ = kFalse;
+  if (!IsGCFinalized())
+    return is_eagerly_finalized_;
+
+  for (Decl* decl : record_->decls()) {
+    if (TypedefDecl* typedef_decl = dyn_cast<TypedefDecl>(decl)) {
+      if (typedef_decl->getNameAsString() != kIsEagerlyFinalizedName)
+        continue;
+      is_eagerly_finalized_ = kTrue;
+      break;
     }
   }
   return is_eagerly_finalized_;
@@ -414,7 +416,13 @@ RecordInfo::Fields* RecordInfo::CollectFields() {
     // Ignore fields annotated with the GC_PLUGIN_IGNORE macro.
     if (Config::IsIgnoreAnnotated(field))
       continue;
-    if (Edge* edge = CreateEdge(field->getType().getTypePtrOrNull())) {
+    // Check if the unexpanded type should be recorded; needed
+    // to track iterator aliases only
+    const Type* unexpandedType = field->getType().getSplitUnqualifiedType().Ty;
+    Edge* edge = CreateEdgeFromOriginalType(unexpandedType);
+    if (!edge)
+      edge = CreateEdge(field->getType().getTypePtrOrNull());
+    if (edge) {
       fields_status = fields_status.LUB(edge->NeedsTracing(Edge::kRecursive));
       fields->insert(std::make_pair(field, FieldPoint(field, edge)));
     }
@@ -567,6 +575,36 @@ static bool isInStdNamespace(clang::Sema& sema, NamespaceDecl* ns)
   return false;
 }
 
+Edge* RecordInfo::CreateEdgeFromOriginalType(const Type* type) {
+  if (!type)
+    return nullptr;
+
+  // look for "typedef ... iterator;"
+  if (!isa<ElaboratedType>(type))
+    return nullptr;
+  const ElaboratedType* elaboratedType = cast<ElaboratedType>(type);
+  if (!isa<TypedefType>(elaboratedType->getNamedType()))
+    return nullptr;
+  const TypedefType* typedefType =
+      cast<TypedefType>(elaboratedType->getNamedType());
+  std::string typeName = typedefType->getDecl()->getNameAsString();
+  if (!Config::IsIterator(typeName))
+    return nullptr;
+  RecordInfo* info =
+      cache_->Lookup(elaboratedType->getQualifier()->getAsType());
+
+  bool on_heap = false;
+  bool is_unsafe = false;
+  // Silently handle unknown types; the on-heap collection types will
+  // have to be in scope for the declaration to compile, though.
+  if (info) {
+    is_unsafe = Config::IsGCCollectionWithUnsafeIterator(info->name());
+    // Don't mark iterator as being on the heap if it is not supported.
+    on_heap = !is_unsafe && Config::IsGCCollection(info->name());
+  }
+  return new Iterator(info, on_heap, is_unsafe);
+}
+
 Edge* RecordInfo::CreateEdge(const Type* type) {
   if (!type) {
     return 0;

tools/clang/blink_gc_plugin/RecordInfo.h

@@ -129,6 +129,7 @@ class RecordInfo {
   bool InheritsTrace();
 
   Edge* CreateEdge(const clang::Type* type);
+  Edge* CreateEdgeFromOriginalType(const clang::Type* type);
 
   RecordCache* cache_;
   clang::CXXRecordDecl* record_;

tools/clang/blink_gc_plugin/tests/fields_illegal_tracing.h

@@ -38,6 +38,9 @@ private:
     RefPtr<HeapObject> m_obj2;
     bar::unique_ptr<HeapObject> m_obj3;
     std::unique_ptr<HeapObject> m_obj4;
+    Vector<int>::iterator m_iterator1;
+    HeapVector<Member<HeapObject>>::iterator m_iterator2;
+    HeapHashSet<PartObject>::const_iterator m_iterator3;
 };
 
 class HeapObject : public GarbageCollectedFinalized<HeapObject> {
@@ -49,6 +52,10 @@ private:
     RefPtr<HeapObject> m_obj2;
     bar::unique_ptr<HeapObject> m_obj3;
     std::unique_ptr<HeapObject> m_obj4;
+    HeapHashMap<int, Member<HeapObject>>::reverse_iterator m_iterator3;
+    HeapDeque<Member<HeapObject>>::const_reverse_iterator m_iterator4;
+    HeapListHashSet<Member<HeapObject>>::const_iterator m_iterator5;
+    HeapLinkedHashSet<Member<HeapObject>>::const_iterator m_iterator6;
 };
 
 }

tools/clang/blink_gc_plugin/tests/fields_illegal_tracing.txt

@@ -11,18 +11,33 @@ class PartObject {
 ./fields_illegal_tracing.h:40:5: note: [blink-gc] std::unique_ptr field 'm_obj4' to a GC managed class declared here:
     std::unique_ptr<HeapObject> m_obj4;
     ^
-./fields_illegal_tracing.h:43:1: warning: [blink-gc] Class 'HeapObject' contains invalid fields.
+./fields_illegal_tracing.h:42:5: note: [blink-gc] Iterator field 'm_iterator2' to a GC managed collection declared here:
+    HeapVector<Member<HeapObject>>::iterator m_iterator2;
+    ^
+./fields_illegal_tracing.h:43:5: note: [blink-gc] Iterator field 'm_iterator3' to a GC managed collection declared here:
+    HeapHashSet<PartObject>::const_iterator m_iterator3;
+    ^
+./fields_illegal_tracing.h:46:1: warning: [blink-gc] Class 'HeapObject' contains invalid fields.
 class HeapObject : public GarbageCollectedFinalized<HeapObject> {
 ^
-./fields_illegal_tracing.h:48:5: note: [blink-gc] OwnPtr field 'm_obj1' to a GC managed class declared here:
+./fields_illegal_tracing.h:51:5: note: [blink-gc] OwnPtr field 'm_obj1' to a GC managed class declared here:
     OwnPtr<HeapObject> m_obj1;
     ^
-./fields_illegal_tracing.h:49:5: note: [blink-gc] RefPtr field 'm_obj2' to a GC managed class declared here:
+./fields_illegal_tracing.h:52:5: note: [blink-gc] RefPtr field 'm_obj2' to a GC managed class declared here:
     RefPtr<HeapObject> m_obj2;
     ^
-./fields_illegal_tracing.h:51:5: note: [blink-gc] std::unique_ptr field 'm_obj4' to a GC managed class declared here:
+./fields_illegal_tracing.h:54:5: note: [blink-gc] std::unique_ptr field 'm_obj4' to a GC managed class declared here:
     std::unique_ptr<HeapObject> m_obj4;
     ^
+./fields_illegal_tracing.h:55:5: note: [blink-gc] Iterator field 'm_iterator3' to a GC managed collection declared here:
+    HeapHashMap<int, Member<HeapObject>>::reverse_iterator m_iterator3;
+    ^
+./fields_illegal_tracing.h:56:5: note: [blink-gc] Iterator field 'm_iterator4' to a GC managed collection declared here:
+    HeapDeque<Member<HeapObject>>::const_reverse_iterator m_iterator4;
+    ^
+./fields_illegal_tracing.h:58:5: note: [blink-gc] Iterator field 'm_iterator6' to a GC managed collection declared here:
+    HeapLinkedHashSet<Member<HeapObject>>::const_iterator m_iterator6;
+    ^
 fields_illegal_tracing.cpp:9:1: warning: [blink-gc] Class 'PartObject' has untraced or not traceable fields.
 void PartObject::trace(Visitor* visitor) {
 ^
@@ -38,13 +53,16 @@ void PartObject::trace(Visitor* visitor) {
 fields_illegal_tracing.cpp:16:1: warning: [blink-gc] Class 'HeapObject' has untraced or not traceable fields.
 void HeapObject::trace(Visitor* visitor) {
 ^
-./fields_illegal_tracing.h:48:5: note: [blink-gc] Untraceable field 'm_obj1' declared here:
+./fields_illegal_tracing.h:51:5: note: [blink-gc] Untraceable field 'm_obj1' declared here:
     OwnPtr<HeapObject> m_obj1;
     ^
-./fields_illegal_tracing.h:49:5: note: [blink-gc] Untraceable field 'm_obj2' declared here:
+./fields_illegal_tracing.h:52:5: note: [blink-gc] Untraceable field 'm_obj2' declared here:
     RefPtr<HeapObject> m_obj2;
     ^
-./fields_illegal_tracing.h:51:5: note: [blink-gc] Untraceable field 'm_obj4' declared here:
+./fields_illegal_tracing.h:54:5: note: [blink-gc] Untraceable field 'm_obj4' declared here:
     std::unique_ptr<HeapObject> m_obj4;
     ^
+./fields_illegal_tracing.h:57:5: note: [blink-gc] Untraced field 'm_iterator5' declared here:
+    HeapListHashSet<Member<HeapObject>>::const_iterator m_iterator5;
+    ^
 4 warnings generated.

tools/clang/blink_gc_plugin/tests/heap/stubs.h

@@ -68,37 +68,62 @@ template<
 class Vector : public VectorDestructorBase<inlineCapacity,
                                            Allocator::isGarbageCollected,
                                            VectorTraits<T>::needsDestruction> {
-public:
-    size_t size();
-    T& operator[](size_t);
+ public:
+  using iterator = T*;
+  using const_iterator = const T*;
+  using reverse_iterator = T*;
+  using const_reverse_iterator = const T*;
+
+  size_t size();
+  T& operator[](size_t);
 };
 
-template<
-    typename T,
-    size_t inlineCapacity = 0,
-    typename Allocator = DefaultAllocator>
-class Deque {};
+template <typename T,
+          size_t inlineCapacity = 0,
+          typename Allocator = DefaultAllocator>
+class Deque {
+ public:
+  using iterator = T*;
+  using const_iterator = const T*;
+  using reverse_iterator = T*;
+  using const_reverse_iterator = const T*;
+};
 
-template<
-    typename ValueArg,
-    typename HashArg = void,
-    typename TraitsArg = void,
-    typename Allocator = DefaultAllocator>
-class HashSet {};
+template <typename ValueArg,
+          typename HashArg = void,
+          typename TraitsArg = void,
+          typename Allocator = DefaultAllocator>
+class HashSet {
+ public:
+  typedef ValueArg* iterator;
+  typedef const ValueArg* const_iterator;
+  typedef ValueArg* reverse_iterator;
+  typedef const ValueArg* const_reverse_iterator;
+};
 
-template<
-    typename ValueArg,
-    typename HashArg = void,
-    typename TraitsArg = void,
-    typename Allocator = DefaultAllocator>
-class ListHashSet {};
+template <typename ValueArg,
+          typename HashArg = void,
+          typename TraitsArg = void,
+          typename Allocator = DefaultAllocator>
+class ListHashSet {
+ public:
+  typedef ValueArg* iterator;
+  typedef const ValueArg* const_iterator;
+  typedef ValueArg* reverse_iterator;
+  typedef const ValueArg* const_reverse_iterator;
+};
 
-template<
-    typename ValueArg,
-    typename HashArg = void,
-    typename TraitsArg = void,
-    typename Allocator = DefaultAllocator>
-class LinkedHashSet {};
+template <typename ValueArg,
+          typename HashArg = void,
+          typename TraitsArg = void,
+          typename Allocator = DefaultAllocator>
+class LinkedHashSet {
+ public:
+  typedef ValueArg* iterator;
+  typedef const ValueArg* const_iterator;
+  typedef ValueArg* reverse_iterator;
+  typedef const ValueArg* const_reverse_iterator;
+};
 
 template<
     typename ValueArg,
@@ -107,15 +132,19 @@ template<
     typename Allocator = DefaultAllocator>
 class HashCountedSet {};
 
-template<
-    typename KeyArg,
-    typename MappedArg,
-    typename HashArg = void,
-    typename KeyTraitsArg = void,
-    typename MappedTraitsArg = void,
-    typename Allocator = DefaultAllocator>
-class HashMap {};
-
+template <typename KeyArg,
+          typename MappedArg,
+          typename HashArg = void,
+          typename KeyTraitsArg = void,
+          typename MappedTraitsArg = void,
+          typename Allocator = DefaultAllocator>
+class HashMap {
+ public:
+  typedef MappedArg* iterator;
+  typedef const MappedArg* const_iterator;
+  typedef MappedArg* reverse_iterator;
+  typedef const MappedArg* const_reverse_iterator;
+};
 }
 
 // Empty namespace declaration to exercise internal

tools/clang/blink_gc_plugin/tests/stack_allocated.txt

@@ -20,7 +20,7 @@ class DerivedHeapObject2 : public HeapObject {
 ./stack_allocated.h:39:3: warning: [blink-gc] Garbage collected class 'DerivedHeapObject2' is not permitted to override its new operator.
   STACK_ALLOCATED();
   ^
-./heap/stubs.h:149:5: note: expanded from macro 'STACK_ALLOCATED'
+./heap/stubs.h:184:5: note: expanded from macro 'STACK_ALLOCATED'
     __attribute__((annotate("blink_stack_allocated")))      \
     ^
 stack_allocated.cpp:12:1: warning: [blink-gc] Class 'AnonStackObject' contains invalid fields.