Bug 1464472 - Add some comments and a test. r=jandem

--HG--
extra : rebase_source : a7da12b5777672e3d8adc49427c4b202c71d5f6f

js/src/tests/non262/statements/for-in-with-gc-during-iterator-init.js

@@ -0,0 +1,30 @@
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/licenses/publicdomain/
+
+//-----------------------------------------------------------------------------
+var gTestfile = "for-in-with-gc-during-iterator-init.js";
+var BUGNUMBER = 1464472;
+var summary =
+  "Properly trace NativeIterator when a GC occurs during its initialization";
+
+print(BUGNUMBER + ": " + summary);
+
+/**************
+ * BEGIN TEST *
+ **************/
+
+gczeal(17, 1);
+for (var i = 0; i < 100; ++i)
+{
+  Object.prototype[1012] = "value";
+  imports = {};
+  for (dmod in imports)
+    continue; // gc occurs here converting 1012 to string
+}
+
+/******************************************************************************/
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);
+
+print("Tests complete");

js/src/vm/Iteration.cpp

@@ -56,6 +56,9 @@ typedef Rooted<PropertyIteratorObject*> RootedPropertyIteratorObject;
 
 static const gc::AllocKind ITERATOR_FINALIZE_KIND = gc::AllocKind::OBJECT2_BACKGROUND;
 
+// Beware!  This function may have to trace incompletely-initialized
+// |NativeIterator| allocations if the |IdToString| in that constructor recurs
+// into this code.
 void
 NativeIterator::trace(JSTracer* trc)
 {
@@ -66,11 +69,24 @@ NativeIterator::trace(JSTracer* trc)
     if (iterObj_)
         TraceManuallyBarrieredEdge(trc, &iterObj_, "iterObj");
 
+    // The limits below are correct at every instant of |NativeIterator|
+    // initialization, with the end-pointer incremented as each new guard is
+    // created, so they're safe to use here.
     std::for_each(guardsBegin(), guardsEnd(),
                   [trc](HeapReceiverGuard& guard) {
                       guard.trace(trc);
                   });
 
+    // But as properties must be created *before* guards, |propertiesBegin()|
+    // that depends on |guardsEnd()| having its final value can't safely be
+    // used.  Until this is fully initialized, use |propertyCursor_| instead,
+    // which points at the start of properties even in partially initialized
+    // |NativeIterator|s.  (|propertiesEnd()| is safe at all times with respect
+    // to the properly-chosen beginning.)
+    //
+    // Note that we must trace all properties (not just those not yet visited,
+    // or just visited, due to |NativeIterator::previousPropertyWas|) for
+    // |NativeIterator|s to be reusable.
     GCPtrFlatString* begin = MOZ_LIKELY(isInitialized()) ? propertiesBegin() : propertyCursor_;
     std::for_each(begin, propertiesEnd(),
                   [trc](GCPtrFlatString& prop) {
@@ -674,7 +690,7 @@ NativeIterator::NativeIterator(JSContext* cx, Handle<PropertyIteratorObject*> pr
     propertyCursor_(reinterpret_cast<GCPtrFlatString*>(guardsBegin() + numGuards)),
     propertiesEnd_(propertyCursor_),
     guardKey_(guardKey),
-    flags_(0)
+    flags_(0) // note: no Flags::Initialized
 {
     MOZ_ASSERT(!*hadError);
 
@@ -740,6 +756,8 @@ NativeIterator::NativeIterator(JSContext* cx, Handle<PropertyIteratorObject*> pr
         MOZ_ASSERT(i == numGuards);
     }
 
+    // |guardsEnd_| is now guaranteed to point at the start of properties, so
+    // we can mark this initialized.
     MOZ_ASSERT(static_cast<void*>(guardsEnd_) == propertyCursor_);
     markInitialized();
 

js/src/vm/Iteration.h

@@ -58,9 +58,33 @@ struct NativeIterator
     // active.  Not serialized by XDR.
     struct Flags
     {
+        // This flag is set when all guards and properties associated with this
+        // NativeIterator have been initialized, such that |guardsEnd_|, in
+        // addition to being the end of guards, is also the beginning of
+        // properties.
+        //
+        // This flag is only *not* set when a NativeIterator is in the process
+        // of being constructed.  At such time |guardsEnd_| accounts only for
+        // guards that have been initialized -- potentially none of them.
+        // Instead, |propertyCursor_| is initialized to the ultimate/actual
+        // start of properties and must be used instead of |propertiesBegin()|,
+        // which asserts that this flag is present to guard against misuse.
         static constexpr uint32_t Initialized = 0x1;
+
+        // This flag indicates that this NativeIterator is currently being used
+        // to enumerate an object's properties and has not yet been closed.
         static constexpr uint32_t Active = 0x2;
+
+        // This flag indicates that the object being enumerated by this
+        // |NativeIterator| had a property deleted from it before it was
+        // visited, forcing the properties array in this to be mutated to
+        // remove it.
         static constexpr uint32_t HasUnvisitedPropertyDeletion = 0x4;
+
+        // If any of these bits are set on a |NativeIterator|, it isn't
+        // currently reusable.  (An active |NativeIterator| can't be stolen
+        // *right now*; a |NativeIterator| that's had its properties mutated
+        // can never be reused, because it would give incorrect results.)
         static constexpr uint32_t NotReusable = Active | HasUnvisitedPropertyDeletion;
     };
 
@@ -248,6 +272,12 @@ struct NativeIterator
 
     bool isReusable() const {
         MOZ_ASSERT(isInitialized());
+
+        // Cached NativeIterators are reusable if they're not currently active
+        // and their properties array hasn't been mutated, i.e. if only
+        // |Flags::Initialized| is set.  Using |Flags::NotReusable| to test
+        // would also work, but this formulation is safer against memory
+        // corruption.
         return flags_ == Flags::Initialized;
     }