From bb2fc6efcebc306450ff11684b1aafa7e9dd6354 Mon Sep 17 00:00:00 2001
From: Bill McCloskey <billm@mozilla.com>
Date: Sun, 30 Apr 2017 20:23:59 -0700
Subject: [PATCH] Bug 1360372 - Avoid rooting hazard when entering atoms
 compartment (r=sfink)

This patch avoids a rooting hazard involving the atoms compartment. There is
code in jsatom.cpp that enters the atoms compartment while unrooted pointers
are on the stack. Now that enterZoneGroup can potentially yield, this leads to
a rooting hazard. This patch avoids the hazard by using a totally separate path
that avoids enterZoneGroup when entering the atoms compartment.

MozReview-Commit-ID: ExG1ofCbN8C
---
 js/src/jscntxt.h              | 12 ++++++++----
 js/src/jscntxtinlines.h       | 24 +++++++++++++++++-------
 js/src/jscompartment.h        |  5 ++++-
 js/src/jscompartmentinlines.h | 23 ++++++++++++++++++-----
 4 files changed, 47 insertions(+), 17 deletions(-)

diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h
index 62a9b3340216..faae8dab7fc2 100644
--- a/js/src/jscntxt.h
+++ b/js/src/jscntxt.h
@@ -201,10 +201,14 @@ struct JSContext : public JS::RootingContext,
 #endif
 
   private:
-    // If |c| or |oldCompartment| is the atoms compartment, the
-    // |exclusiveAccessLock| must be held.
-    inline void enterCompartment(JSCompartment* c,
-                                 const js::AutoLockForExclusiveAccess* maybeLock = nullptr);
+    // We distinguish between entering the atoms compartment and all other
+    // compartments. Entering the atoms compartment requires a lock. Also, we
+    // don't call enterZoneGroup when entering the atoms compartment since that
+    // can induce GC hazards.
+    inline void enterNonAtomsCompartment(JSCompartment* c);
+    inline void enterAtomsCompartment(JSCompartment* c,
+                                      const js::AutoLockForExclusiveAccess& lock);
+
     friend class js::AutoCompartment;
 
   public:
diff --git a/js/src/jscntxtinlines.h b/js/src/jscntxtinlines.h
index 85fa87936861..4dad00a57c6f 100644
--- a/js/src/jscntxtinlines.h
+++ b/js/src/jscntxtinlines.h
@@ -451,17 +451,27 @@ JSContext::runningWithTrustedPrincipals()
 }
 
 inline void
-JSContext::enterCompartment(
-    JSCompartment* c,
-    const js::AutoLockForExclusiveAccess* maybeLock /* = nullptr */)
+JSContext::enterNonAtomsCompartment(JSCompartment* c)
 {
     enterCompartmentDepth_++;
 
-    if (!c->zone()->isAtomsZone())
-        enterZoneGroup(c->zone()->group());
+    MOZ_ASSERT(!c->zone()->isAtomsZone());
+    enterZoneGroup(c->zone()->group());
 
     c->enter();
-    setCompartment(c, maybeLock);
+    setCompartment(c, nullptr);
+}
+
+inline void
+JSContext::enterAtomsCompartment(JSCompartment* c,
+                                 const js::AutoLockForExclusiveAccess& lock)
+{
+    enterCompartmentDepth_++;
+
+    MOZ_ASSERT(c->zone()->isAtomsZone());
+
+    c->enter();
+    setCompartment(c, &lock);
 }
 
 template <typename T>
@@ -469,7 +479,7 @@ inline void
 JSContext::enterCompartmentOf(const T& target)
 {
     MOZ_ASSERT(JS::CellIsNotGray(target));
-    enterCompartment(target->compartment(), nullptr);
+    enterNonAtomsCompartment(target->compartment());
 }
 
 inline void
diff --git a/js/src/jscompartment.h b/js/src/jscompartment.h
index 3cf989c5d408..375b5cd69731 100644
--- a/js/src/jscompartment.h
+++ b/js/src/jscompartment.h
@@ -1053,8 +1053,11 @@ class AutoCompartment
     JSCompartment* origin() const { return origin_; }
 
   protected:
+    inline AutoCompartment(JSContext* cx, JSCompartment* target);
+
+    // Used only for entering the atoms compartment.
     inline AutoCompartment(JSContext* cx, JSCompartment* target,
-                           AutoLockForExclusiveAccess* maybeLock = nullptr);
+                           AutoLockForExclusiveAccess& lock);
 
   private:
     AutoCompartment(const AutoCompartment&) = delete;
diff --git a/js/src/jscompartmentinlines.h b/js/src/jscompartmentinlines.h
index 54c90f02e415..47280975127a 100644
--- a/js/src/jscompartmentinlines.h
+++ b/js/src/jscompartmentinlines.h
@@ -44,14 +44,27 @@ js::AutoCompartment::AutoCompartment(JSContext* cx, const T& target)
     cx_->enterCompartmentOf(target);
 }
 
-// Protected constructor that bypasses assertions in enterCompartmentOf.
+// Protected constructor that bypasses assertions in enterCompartmentOf. Used
+// only for entering the atoms compartment.
 js::AutoCompartment::AutoCompartment(JSContext* cx, JSCompartment* target,
-                                     js::AutoLockForExclusiveAccess* maybeLock /* = nullptr */)
+                                     js::AutoLockForExclusiveAccess& lock)
   : cx_(cx),
     origin_(cx->compartment()),
-    maybeLock_(maybeLock)
+    maybeLock_(&lock)
 {
-    cx_->enterCompartment(target, maybeLock);
+    MOZ_ASSERT(target->isAtomsCompartment());
+    cx_->enterAtomsCompartment(target, lock);
+}
+
+// Protected constructor that bypasses assertions in enterCompartmentOf. Should
+// not be used to enter the atoms compartment.
+js::AutoCompartment::AutoCompartment(JSContext* cx, JSCompartment* target)
+  : cx_(cx),
+    origin_(cx->compartment()),
+    maybeLock_(nullptr)
+{
+    MOZ_ASSERT(!target->isAtomsCompartment());
+    cx_->enterNonAtomsCompartment(target);
 }
 
 js::AutoCompartment::~AutoCompartment()
@@ -61,7 +74,7 @@ js::AutoCompartment::~AutoCompartment()
 
 js::AutoAtomsCompartment::AutoAtomsCompartment(JSContext* cx,
                                                js::AutoLockForExclusiveAccess& lock)
-  : AutoCompartment(cx, cx->atomsCompartment(lock), &lock)
+  : AutoCompartment(cx, cx->atomsCompartment(lock), lock)
 {}
 
 js::AutoCompartmentUnchecked::AutoCompartmentUnchecked(JSContext* cx, JSCompartment* target)