create handles in the current scope's parent

Summary:
Returning a Handle from a function can be really inconvenient if that
function has a GCScope - the handle is created in the current scope,
which is promptly destroyed, leaving us with an invalid handle.

Add Runtime methods to create handles in the current scope's parent for
convenience.

Reviewed By: mhorowitz

Differential Revision: D23643302

fbshipit-source-id: 24e7c7f69fdd0f727f75ab010c5619e99446e831

include/hermes/VM/Handle-inline.h

@@ -32,15 +32,19 @@ inline PseudoHandle<T> PseudoHandle<T>::dyn_vmcast(PseudoHandle<U> &&other) {
   return result;
 }
 
-/// Allocate a new handle in the current GCScope
-inline HandleBase::HandleBase(HandleRootOwner *runtime, HermesValue value)
-    : handle_(runtime->newHandle(value)) {
+/// Allocate a new handle in the specified GCScope
+inline HandleBase::HandleBase(GCScope *inScope, HermesValue value)
+    : handle_(HandleRootOwner::newPinnedHermesValue(inScope, value)) {
 #ifndef NDEBUG
-  gcScope_ = runtime->getTopGCScope();
+  gcScope_ = inScope;
   ++gcScope_->numActiveHandles_;
 #endif
 }
 
+/// Allocate a new handle in the current GCScope
+inline HandleBase::HandleBase(HandleRootOwner *runtime, HermesValue value)
+    : HandleBase(runtime->getTopGCScope(), value) {}
+
 #ifndef NDEBUG
 inline HandleBase::HandleBase(const HandleBase &sc) : handle_(sc.handle_) {
   gcScope_ = sc.gcScope_;

include/hermes/VM/Handle.h

@@ -201,6 +201,11 @@ class HandleBase {
   }
 
  public:
+  /// Allocate a new handle in the specified GCScope.
+  explicit HandleBase(
+      GCScope *inScope,
+      HermesValue value = HermesValue::encodeUndefinedValue());
+
   /// Allocate a new handle in the current GCScope
   explicit HandleBase(
       HandleRootOwner *runtime,
@@ -315,6 +320,10 @@ class Handle : public HandleBase {
       HandleRootOwner *runtime,
       value_type value = HermesValueTraits<T>::defaultValue())
       : HandleBase(runtime, HermesValueTraits<T>::encode(value)){};
+  explicit Handle(
+      GCScope *inScope,
+      value_type value = HermesValueTraits<T>::defaultValue())
+      : HandleBase(inScope, HermesValueTraits<T>::encode(value)){};
 
   /// Create a Handle aliasing a non-movable HermesValue without
   /// allocating a handle.

include/hermes/VM/HandleRootOwner-inline.h

@@ -15,6 +15,9 @@ namespace hermes {
 namespace vm {
 
 inline Handle<HermesValue> HandleRootOwner::makeHandle(HermesValue value) {
+  // This is the same as Handle<HermesValue>(getTopGCScope(), value), but it's
+  // this way for historical reasons.  The source could be updated for clarity
+  // if the generated code doesn't change.
   return Handle<HermesValue>(this, value);
 }
 template <class T>
@@ -28,6 +31,50 @@ inline Handle<T> HandleRootOwner::makeHandle(HermesValue value) {
 inline Handle<SymbolID> HandleRootOwner::makeHandle(SymbolID value) {
   return Handle<SymbolID>(this, value);
 }
+template <class T>
+inline Handle<T> HandleRootOwner::makeHandle(PseudoHandle<T> &&pseudo) {
+  Handle<T> res{this, pseudo.get()};
+  pseudo.invalidate();
+  return res;
+}
+template <class T>
+inline Handle<T> HandleRootOwner::makeHandle(
+    PseudoHandle<HermesValue> &&pseudo) {
+  Handle<T> res{this, PseudoHandle<T>::vmcast(std::move(pseudo)).get()};
+  return res;
+}
+
+inline Handle<HermesValue> HandleRootOwner::makeHandleInParentScope(
+    HermesValue value) {
+  return Handle<HermesValue>(getTopGCScopesParent(), value);
+}
+template <class T>
+inline Handle<T> HandleRootOwner::makeHandleInParentScope(T *p) {
+  return Handle<T>(getTopGCScopesParent(), p);
+}
+template <class T>
+inline Handle<T> HandleRootOwner::makeHandleInParentScope(HermesValue value) {
+  return Handle<T>(getTopGCScopesParent(), vmcast<T>(value));
+}
+inline Handle<SymbolID> HandleRootOwner::makeHandleInParentScope(
+    SymbolID value) {
+  return Handle<SymbolID>(getTopGCScopesParent(), value);
+}
+template <class T>
+inline Handle<T> HandleRootOwner::makeHandleInParentScope(
+    PseudoHandle<T> &&pseudo) {
+  Handle<T> res{getTopGCScopesParent(), pseudo.get()};
+  pseudo.invalidate();
+  return res;
+}
+template <class T>
+inline Handle<T> HandleRootOwner::makeHandleInParentScope(
+    PseudoHandle<HermesValue> &&pseudo) {
+  Handle<T> res{getTopGCScopesParent(),
+                PseudoHandle<T>::vmcast(std::move(pseudo)).get()};
+  return res;
+}
+
 inline MutableHandle<HermesValue> HandleRootOwner::makeMutableHandle(
     HermesValue value) {
   return MutableHandle<HermesValue>(this, value);
@@ -58,9 +105,25 @@ inline Handle<HermesValue> HandleRootOwner::getBoolValue(bool b) {
   return Handle<HermesValue>(b ? &trueValue_ : &falseValue_);
 }
 
-inline PinnedHermesValue *HandleRootOwner::newHandle(HermesValue value) {
+inline GCScope *HandleRootOwner::getTopGCScope() {
+  return topGCScope_;
+}
+inline GCScope *HandleRootOwner::getTopGCScopesParent() {
+  assert(topGCScope_ && "trying to obtain the parent of NULL scope");
+  return topGCScope_->getParentScope();
+}
+
+inline PinnedHermesValue *HandleRootOwner::newPinnedHermesValue(
+    GCScope *inScope,
+    HermesValue value) {
+  assert(inScope && "Target GCScope can't be null");
+  return inScope->newPinnedHermesValue(value);
+}
+
+inline PinnedHermesValue *HandleRootOwner::newPinnedHermesValue(
+    HermesValue value) {
   assert(topGCScope_ && "no active GCScope");
-  return topGCScope_->newHandle(value);
+  return newPinnedHermesValue(topGCScope_, value);
 }
 
 } // namespace vm

include/hermes/VM/HandleRootOwner.h

@@ -57,8 +57,9 @@ class HandleRootOwner {
 
   /// Convenience function to create a Handle.
   Handle<HermesValue> makeHandle(HermesValue value);
-  template <class T>
+
   /// Convenience function to create a Handle from a pointer.
+  template <class T>
   Handle<T> makeHandle(T *p);
 
   /// Convenience function to create typed Handle given a HermesValue.
@@ -70,19 +71,31 @@ class HandleRootOwner {
 
   /// Create a Handle from a valid PseudoHandle and invalidate the latter.
   template <class T>
-  Handle<T> makeHandle(PseudoHandle<T> &&pseudo) {
-    Handle<T> res{this, pseudo.get()};
-    pseudo.invalidate();
-    return res;
-  }
+  Handle<T> makeHandle(PseudoHandle<T> &&pseudo);
 
   /// Create a Handle from a valid PseudoHandle by vmcasting from HermesValue
   /// and invalidate the PseudoHandle<HermesValue>.
   template <class T>
-  Handle<T> makeHandle(PseudoHandle<HermesValue> &&pseudo) {
-    Handle<T> res{this, PseudoHandle<T>::vmcast(std::move(pseudo)).get()};
-    return res;
-  }
+  Handle<T> makeHandle(PseudoHandle<HermesValue> &&pseudo);
+
+  /// @name Creating handles in the parent scope
+  /// @{
+  Handle<HermesValue> makeHandleInParentScope(HermesValue value);
+
+  template <class T>
+  Handle<T> makeHandleInParentScope(T *p);
+
+  template <class T>
+  Handle<T> makeHandleInParentScope(HermesValue value);
+
+  Handle<SymbolID> makeHandleInParentScope(SymbolID value);
+
+  template <class T>
+  Handle<T> makeHandleInParentScope(PseudoHandle<T> &&pseudo);
+
+  template <class T>
+  Handle<T> makeHandleInParentScope(PseudoHandle<HermesValue> &&pseudo);
+  /// @}
 
   /// Convenience function to create a MutableHandle.
   MutableHandle<HermesValue> makeMutableHandle(HermesValue value);
@@ -108,9 +121,10 @@ class HandleRootOwner {
   static Handle<HermesValue> getBoolValue(bool b);
 
   /// Return the top-most \c GCScope.
-  GCScope *getTopGCScope() {
-    return topGCScope_;
-  }
+  GCScope *getTopGCScope();
+
+  /// Return the parent of the top-most \c GCScope.
+  GCScope *getTopGCScopesParent();
 
  protected:
   /// Used for efficient construction of Handle<>(..., nullptr).
@@ -137,9 +151,15 @@ class HandleRootOwner {
   /// The top-most GC scope.
   GCScope *topGCScope_{};
 
-  /// Allocate a new handle in the top-most GCScope and initialize with
-  /// \p value.
-  PinnedHermesValue *newHandle(HermesValue value);
+  /// Allocate storage for a new PinnedHermesValue in the specified GCScope and
+  /// initialize it with \p value.
+  static PinnedHermesValue *newPinnedHermesValue(
+      GCScope *inScope,
+      HermesValue value);
+
+  /// Allocate storage for a new PinnedHermesValye in the top-most GCScope and
+  /// initialize it with \p value.
+  PinnedHermesValue *newPinnedHermesValue(HermesValue value);
 };
 
 /// This class exists only for debugging purposes. We need to make
@@ -284,6 +304,11 @@ class GCScope : public GCScopeDebugBase {
 
   ~GCScope();
 
+  /// \return the parent scope of this scope.
+  GCScope *getParentScope() {
+    return prevScope_;
+  }
+
   /// \return the increment in which pages of handles are allocated. To be
   /// used for debugging and asserts.
   static constexpr size_t getChunkSize() {
@@ -407,18 +432,12 @@ class GCScope : public GCScopeDebugBase {
   }
 #endif
 
-  /// Allocate a new handle. The garbage collector knows about it and will be
-  /// able to mark it.
-  PinnedHermesValue *newHandle(HermesValue value) {
+  /// Allocate storage for a new PinnedHermesValue. The garbage collector knows
+  /// about it and will be able to mark it.
+  PinnedHermesValue *newPinnedHermesValue(HermesValue value) {
     assert(
         getHandleCountDbg() < handlesLimit_ &&
         "Too many handles allocated in GCScope");
-    // We currently only allocate handles in the top scope, although there is no
-    // current design constraint why we must. This assert serves to detect bugs
-    // early, and can be removed if we ever want to violate this invariant.
-    assert(
-        runtime_->getTopGCScope() == this &&
-        "Expect allocation only in top scope");
 
     setHandleCountDbg(getHandleCountDbg() + 1);
 #ifdef HERMESVM_DEBUG_TRACK_GCSCOPE_HANDLES
@@ -433,12 +452,12 @@ class GCScope : public GCScopeDebugBase {
     }
 
     /// Slow path: allocate a new chunk.
-    return _newChunkAndHandle(value);
+    return _newChunkAndPHV(value);
   }
 
-  /// The current chunk is full, so allocate a new chunk and allocate a handle
-  /// inside it. This is the allocation slow path.
-  PinnedHermesValue *_newChunkAndHandle(HermesValue value);
+  /// The current chunk is full, so allocate a new chunk and allocate a
+  /// PinnedHermesValue inside it. This is the allocation slow path.
+  PinnedHermesValue *_newChunkAndPHV(HermesValue value);
 
   /// Mark all handles in this scope.
   void mark(SlotAcceptor &acceptor);

lib/VM/HandleRootOwner.cpp

@@ -57,7 +57,7 @@ GCScope::~GCScope() {
 #endif
 }
 
-PinnedHermesValue *GCScope::_newChunkAndHandle(HermesValue value) {
+PinnedHermesValue *GCScope::_newChunkAndPHV(HermesValue value) {
   assert(next_ == curChunkEnd_ && "current chunk is not exhaused");
 
   // Move to the next chunk.

unittests/VMRuntime/HandleTest.cpp

@@ -154,6 +154,32 @@ TEST_F(HandleTest, ScopedPointerConstructorTest) {
   ASSERT_EQ(function.get(), obj.get());
 }
 
+/// Make sure that we can create handles one scope up in the scope chain.
+TEST_F(HandleTest, CreateInParentScope) {
+  GCScope theParent{runtime};
+  auto h1 = runtime->makeHandle(HermesValue::encodeBoolValue(true));
+  (void)h1;
+#ifndef NDEBUG
+  ASSERT_EQ(1, theParent.getHandleCountDbg());
+#endif
+
+  auto func = [this, &theParent]() -> Handle<> {
+    GCScope theChild{runtime};
+    EXPECT_EQ(&theParent, runtime->getTopGCScopesParent());
+    auto h2 = runtime->makeHandle(HermesValue::encodeBoolValue(true));
+    (void)h2;
+    auto h3 =
+        runtime->makeHandleInParentScope(HermesValue::encodeBoolValue(true));
+    return h3;
+  };
+
+  auto h3 = func();
+  (void)h3;
+#ifndef NDEBUG
+  ASSERT_EQ(2, theParent.getHandleCountDbg());
+#endif
+}
+
 #ifdef HERMES_SLOW_DEBUG
 TEST_F(HandleDeathTest, UseFlushedHandle) {
   auto marker = gcScope.createMarker();