Bug 1197097 - Don't use a context-wide cycle-detection mechanism for detecting cycles during JSON.stringify. This prevents nested (yet separate) JSON.stringify, and it causes that algorithm to be affected by specification-unrelated operations like toSource. r=jonco

--HG--
extra : rebase_source : 6a2f7174e79e4a52f77084e2a8d8172484422998

js/public/TraceableHashTable.h

@@ -153,6 +153,164 @@ class HandleBase<TraceableHashMap<A,B,C,D,E,F>>
   : public TraceableHashMapOperations<JS::Handle<TraceableHashMap<A,B,C,D,E,F>>, A,B,C,D,E,F>
 {};
 
+// A TraceableHashSet is a HashSet with an additional trace method that knows
+// how to visit all set element.  HashSets that contain GC pointers that must
+// be traced to be kept alive will generally want to use this TraceableHashSet
+// specializeation in lieu of HashSet.
+//
+// Most types of GC pointers can be traced with no extra infrastructure.  For
+// structs and non-gc-pointer members, ensure that there is a specialization of
+// DefaultTracer<T> with an appropriate trace method available to handle the
+// custom type.
+//
+// Note that although this HashSet's trace will deal correctly with moved
+// elements, it does not itself know when to barrier or trace elements.  To
+// function properly it must either be used with Rooted or barriered and traced
+// manually.
+template <typename T,
+          typename HashPolicy = DefaultHasher<T>,
+          typename AllocPolicy = TempAllocPolicy,
+          typename ElemTraceFunc = DefaultTracer<T>>
+class TraceableHashSet : public HashSet<T, HashPolicy, AllocPolicy>,
+                         public JS::Traceable
+{
+    using Base = HashSet<T, HashPolicy, AllocPolicy>;
+
+  public:
+    explicit TraceableHashSet(AllocPolicy a = AllocPolicy()) : Base(a)  {}
+
+    static void trace(TraceableHashSet* set, JSTracer* trc) { set->trace(trc); }
+    void trace(JSTracer* trc) {
+        if (!this->initialized())
+            return;
+        for (typename Base::Enum e(*this); !e.empty(); e.popFront()) {
+            T elem = e.front();
+            ElemTraceFunc::trace(trc, &elem, "hashset element");
+            if (elem != e.front())
+                e.rekeyFront(elem);
+        }
+    }
+
+    // TraceableHashSet is movable
+    TraceableHashSet(TraceableHashSet&& rhs) : Base(mozilla::Forward<TraceableHashSet>(rhs)) {}
+    void operator=(TraceableHashSet&& rhs) {
+        MOZ_ASSERT(this != &rhs, "self-move assignment is prohibited");
+        Base::operator=(mozilla::Forward<TraceableHashSet>(rhs));
+    }
+
+  private:
+    // TraceableHashSet is not copyable or assignable
+    TraceableHashSet(const TraceableHashSet& hs) = delete;
+    TraceableHashSet& operator=(const TraceableHashSet& hs) = delete;
+};
+
+template <typename Outer, typename... Args>
+class TraceableHashSetOperations
+{
+    using Set = TraceableHashSet<Args...>;
+    using Lookup = typename Set::Lookup;
+    using Ptr = typename Set::Ptr;
+    using AddPtr = typename Set::AddPtr;
+    using Range = typename Set::Range;
+    using Enum = typename Set::Enum;
+
+    const Set& set() const { return static_cast<const Outer*>(this)->extract(); }
+
+  public:
+    bool initialized() const                   { return set().initialized(); }
+    Ptr lookup(const Lookup& l) const          { return set().lookup(l); }
+    AddPtr lookupForAdd(const Lookup& l) const { return set().lookupForAdd(l); }
+    Range all() const                          { return set().all(); }
+    bool empty() const                         { return set().empty(); }
+    uint32_t count() const                     { return set().count(); }
+    size_t capacity() const                    { return set().capacity(); }
+    uint32_t generation() const                { return set().generation(); }
+    bool has(const Lookup& l) const            { return set().lookup(l).found(); }
+};
+
+template <typename Outer, typename... Args>
+class MutableTraceableHashSetOperations
+  : public TraceableHashSetOperations<Outer, Args...>
+{
+    using Set = TraceableHashSet<Args...>;
+    using Lookup = typename Set::Lookup;
+    using Ptr = typename Set::Ptr;
+    using AddPtr = typename Set::AddPtr;
+    using Range = typename Set::Range;
+    using Enum = typename Set::Enum;
+
+    Set& set() { return static_cast<Outer*>(this)->extract(); }
+
+  public:
+    bool init(uint32_t len = 16) { return set().init(len); }
+    void clear()                 { set().clear(); }
+    void finish()                { set().finish(); }
+    void remove(const Lookup& l) { set().remove(l); }
+
+    template<typename TInput>
+    bool add(AddPtr& p, TInput&& t) {
+        return set().add(p, mozilla::Forward<TInput>(t));
+    }
+
+    template<typename TInput>
+    bool relookupOrAdd(AddPtr& p, const Lookup& l, TInput&& t) {
+        return set().relookupOrAdd(p, l, mozilla::Forward<TInput>(t));
+    }
+
+    template<typename TInput>
+    bool put(TInput&& t) {
+        return set().put(mozilla::Forward<TInput>(t));
+    }
+
+    template<typename TInput>
+    bool putNew(TInput&& t) {
+        return set().putNew(mozilla::Forward<TInput>(t));
+    }
+
+    template<typename TInput>
+    bool putNew(const Lookup& l, TInput&& t) {
+        return set().putNew(l, mozilla::Forward<TInput>(t));
+    }
+};
+
+template <typename T, typename HP, typename AP, typename TF>
+class RootedBase<TraceableHashSet<T, HP, AP, TF>>
+  : public MutableTraceableHashSetOperations<JS::Rooted<TraceableHashSet<T, HP, AP, TF>>, T, HP, AP, TF>
+{
+    using Set = TraceableHashSet<T, HP, AP, TF>;
+
+    friend class TraceableHashSetOperations<JS::Rooted<Set>, T, HP, AP, TF>;
+    const Set& extract() const { return *static_cast<const JS::Rooted<Set>*>(this)->address(); }
+
+    friend class MutableTraceableHashSetOperations<JS::Rooted<Set>, T, HP, AP, TF>;
+    Set& extract() { return *static_cast<JS::Rooted<Set>*>(this)->address(); }
+};
+
+template <typename T, typename HP, typename AP, typename TF>
+class MutableHandleBase<TraceableHashSet<T, HP, AP, TF>>
+  : public MutableTraceableHashSetOperations<JS::MutableHandle<TraceableHashSet<T, HP, AP, TF>>,
+                                             T, HP, AP, TF>
+{
+    using Set = TraceableHashSet<T, HP, AP, TF>;
+
+    friend class TraceableHashSetOperations<JS::MutableHandle<Set>, T, HP, AP, TF>;
+    const Set& extract() const {
+        return *static_cast<const JS::MutableHandle<Set>*>(this)->address();
+    }
+
+    friend class MutableTraceableHashSetOperations<JS::MutableHandle<Set>, T, HP, AP, TF>;
+    Set& extract() { return *static_cast<JS::MutableHandle<Set>*>(this)->address(); }
+};
+
+template <typename T, typename HP, typename AP, typename TF>
+class HandleBase<TraceableHashSet<T, HP, AP, TF>>
+  : public TraceableHashSetOperations<JS::Handle<TraceableHashSet<T, HP, AP, TF>>, T, HP, AP, TF>
+{
+    using Set = TraceableHashSet<T, HP, AP, TF>;
+    friend class TraceableHashSetOperations<JS::Handle<Set>, T, HP, AP, TF>;
+    const Set& extract() const { return *static_cast<const JS::Handle<Set>*>(this)->address(); }
+};
+
 } /* namespace js */
 
 #endif /* gc_HashTable_h */

js/src/json.cpp

@@ -135,13 +135,19 @@ class StringifyContext
       : sb(sb),
         gap(gap),
         replacer(cx, replacer),
+        stack(cx, TraceableHashSet<JSObject*>(cx)),
         propertyList(propertyList),
         depth(0)
     {}
 
+    bool init() {
+        return stack.init(8);
+    }
+
     StringBuffer& sb;
     const StringBuffer& gap;
     RootedObject replacer;
+    Rooted<TraceableHashSet<JSObject*>> stack;
     const AutoIdVector& propertyList;
     uint32_t depth;
 };
@@ -290,6 +296,32 @@ IsFilteredValue(const Value& v)
     return v.isUndefined() || v.isSymbol() || IsCallable(v);
 }
 
+class CycleDetector
+{
+  public:
+    CycleDetector(StringifyContext* scx, HandleObject obj)
+      : stack(&scx->stack), obj_(obj) {
+    }
+
+    bool foundCycle(JSContext* cx) {
+        auto addPtr = stack.lookupForAdd(obj_);
+        if (addPtr) {
+            JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_JSON_CYCLIC_VALUE,
+                                 js_object_str);
+            return false;
+        }
+        return stack.add(addPtr, obj_);
+    }
+
+    ~CycleDetector() {
+        stack.remove(obj_);
+    }
+
+  private:
+    MutableHandle<TraceableHashSet<JSObject*>> stack;
+    HandleObject obj_;
+};
+
 /* ES5 15.12.3 JO. */
 static bool
 JO(JSContext* cx, HandleObject obj, StringifyContext* scx)
@@ -305,14 +337,9 @@ JO(JSContext* cx, HandleObject obj, StringifyContext* scx)
      */
 
     /* Steps 1-2, 11. */
-    AutoCycleDetector detect(cx, obj);
-    if (!detect.init())
+    CycleDetector detect(scx, obj);
+    if (!detect.foundCycle(cx))
         return false;
-    if (detect.foundCycle()) {
-        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_JSON_CYCLIC_VALUE,
-                             js_object_str);
-        return false;
-    }
 
     if (!scx->sb.append('{'))
         return false;
@@ -396,14 +423,9 @@ JA(JSContext* cx, HandleObject obj, StringifyContext* scx)
      */
 
     /* Steps 1-2, 11. */
-    AutoCycleDetector detect(cx, obj);
-    if (!detect.init())
+    CycleDetector detect(scx, obj);
+    if (!detect.foundCycle(cx))
         return false;
-    if (detect.foundCycle()) {
-        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_JSON_CYCLIC_VALUE,
-                             js_object_str);
-        return false;
-    }
 
     if (!scx->sb.append('['))
         return false;
@@ -670,6 +692,8 @@ js::Stringify(JSContext* cx, MutableHandleValue vp, JSObject* replacer_, Value s
 
     /* Step 11. */
     StringifyContext scx(cx, sb, gap, replacer, propertyList);
+    if (!scx.init())
+        return false;
     if (!PreprocessValue(cx, wrapper, HandleId(emptyId), vp, &scx))
         return false;
     if (IsFilteredValue(vp))

js/src/tests/ecma_5/JSON/cyclic-stringify-unrelated.js

@@ -0,0 +1,39 @@
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/licenses/publicdomain/
+
+//-----------------------------------------------------------------------------
+var BUGNUMBER = 1197097;
+var summary = "JSON.stringify shouldn't use context-wide cycle detection";
+
+print(BUGNUMBER + ": " + summary);
+
+/**************
+ * BEGIN TEST *
+ **************/
+
+var arr;
+
+// Nested yet separate JSON.stringify is okay.
+arr = [{}];
+assertEq(JSON.stringify(arr, function(k, v) {
+  assertEq(JSON.stringify(arr), "[{}]");
+  return v;
+}), "[{}]");
+
+// SpiderMonkey censors cycles in array-joining.  This mechanism must not
+// interfere with the cycle detection in JSON.stringify.
+arr = [{
+  toString: function() {
+    var s = JSON.stringify(arr);
+    assertEq(s, "[{}]");
+    return s;
+  }
+}];
+assertEq(arr.join(), "[{}]");
+
+/******************************************************************************/
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);
+
+print("Tests complete");