Replace StringCycleChecker with scope guard

Summary:
The `StringCycleChecker` is a little weird to use because it cannot
return an error from its constructor. Instead, we have to check the
result of the constructor using `foundCycle`, and have to ensure that
the stack in the runtime is correctly set up so that the destructor can
run safely. This is also inefficient, but that probably doesn't matter.

Given that there are only two places it is currently used, a scope
guard seems simpler and clearer.

Reviewed By: kodafb

Differential Revision: D29954709

fbshipit-source-id: 0eb814f4568f315453f7b0d3c63b363802157d7b

include/hermes/VM/Runtime.h

@@ -728,7 +728,8 @@ class Runtime : public HandleRootOwner,
   ExecutionStatus stepFunction(InterpreterState &state);
 #endif
 
-  /// Inserts an object into the string cycle checking stack.
+  /// Inserts an object into the string cycle checking stack if it does not
+  /// already exist.
   /// \return true if a cycle was found
   bool insertVisitedObject(JSObject *obj);
 

lib/VM/JSLib/Array.cpp

@@ -18,6 +18,8 @@
 #include "hermes/VM/StringRefUtils.h"
 #include "hermes/VM/StringView.h"
 
+#include "llvh/ADT/ScopeExit.h"
+
 namespace hermes {
 namespace vm {
 
@@ -359,35 +361,6 @@ arrayIsArray(void *, Runtime *runtime, NativeArgs args) {
   return HermesValue::encodeBoolValue(*res);
 }
 
-namespace {
-/// Used to detect cyclic string conversions, and should be allocated on the
-/// stack. On construction, inserts an object into the runtime string cycle
-/// check stack, and removes it when destroyed.
-/// Use the foundCycle method to know if the object has already been visited.
-class StringCycleChecker {
- public:
-  /// FIXME: Handle error on inserting the visited object.
-  StringCycleChecker(Runtime *runtime, Handle<JSObject> obj)
-      : runtime_(runtime),
-        obj_(obj),
-        foundCycle_(runtime->insertVisitedObject(*obj)) {}
-
-  ~StringCycleChecker() {
-    runtime_->removeVisitedObject(*obj_);
-  }
-
-  bool foundCycle() const {
-    return foundCycle_;
-  }
-
- private:
-  Runtime *runtime_;
-  Handle<JSObject> obj_;
-
-  bool foundCycle_;
-};
-} // namespace
-
 /// ES5.1 15.4.4.5.
 CallResult<HermesValue>
 arrayPrototypeToString(void *, Runtime *runtime, NativeArgs args) {
@@ -425,10 +398,10 @@ arrayPrototypeToLocaleString(void *, Runtime *runtime, NativeArgs args) {
   auto emptyString =
       runtime->getPredefinedStringHandle(Predefined::emptyString);
 
-  StringCycleChecker checker{runtime, array};
-  if (checker.foundCycle()) {
+  if (runtime->insertVisitedObject(*array))
     return emptyString.getHermesValue();
-  }
+  auto cycleScope =
+      llvh::make_scope_exit([&] { runtime->removeVisitedObject(*array); });
 
   auto propRes = JSObject::getNamed_RJS(
       array, runtime, Predefined::getSymbolID(Predefined::length));
@@ -752,10 +725,10 @@ arrayPrototypeJoin(void *, Runtime *runtime, NativeArgs args) {
   auto emptyString =
       runtime->getPredefinedStringHandle(Predefined::emptyString);
 
-  StringCycleChecker checker{runtime, O};
-  if (checker.foundCycle()) {
+  if (runtime->insertVisitedObject(*O))
     return emptyString.getHermesValue();
-  }
+  auto cycleScope =
+      llvh::make_scope_exit([&] { runtime->removeVisitedObject(*O); });
 
   auto propRes = JSObject::getNamed_RJS(
       O, runtime, Predefined::getSymbolID(Predefined::length));

lib/VM/Runtime.cpp

@@ -1328,10 +1328,11 @@ ExecutionStatus Runtime::raiseEvalUnsupported(llvh::StringRef code) {
 }
 
 bool Runtime::insertVisitedObject(JSObject *obj) {
-  const bool foundCycle = llvh::find(stringCycleCheckVisited_, obj) !=
-      stringCycleCheckVisited_.end();
+  if (llvh::find(stringCycleCheckVisited_, obj) !=
+      stringCycleCheckVisited_.end())
+    return true;
   stringCycleCheckVisited_.push_back(obj);
-  return foundCycle;
+  return false;
 }
 
 void Runtime::removeVisitedObject(JSObject *obj) {