diff --git a/js/src/tests/non262/statements/for-in-with-gc-during-iterator-init.js b/js/src/tests/non262/statements/for-in-with-gc-during-iterator-init.js new file mode 100644 index 000000000000..d36322878f3b --- /dev/null +++ b/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"); diff --git a/js/src/vm/Iteration.cpp b/js/src/vm/Iteration.cpp index 3fd1282c4de0..35a174418278 100644 --- a/js/src/vm/Iteration.cpp +++ b/js/src/vm/Iteration.cpp @@ -56,6 +56,9 @@ typedef Rooted 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 pr propertyCursor_(reinterpret_cast(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 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(guardsEnd_) == propertyCursor_); markInitialized(); diff --git a/js/src/vm/Iteration.h b/js/src/vm/Iteration.h index 5275425038f8..8e40caada1b4 100644 --- a/js/src/vm/Iteration.h +++ b/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; }