Don't check the for-in cache for non-cacheable objects.

mhorowitz · facebook-github-bot · commit 83dfb44548e2 · 2020-10-01T18:07:38.000-07:00
Summary:
Previously, if a Proxy shared a HiddenClass with an object in the ForIn cache, the code would try to walk the prototype chain on the Proxy, which triggers an assert.

Also split the slow test out of for_in.js.

Reviewed By: tmikov

Differential Revision: D24061305

fbshipit-source-id: aef2b3e4b00943a14a6cee85c351eef7788f9b9b
diff --git a/lib/VM/JSObject.cpp b/lib/VM/JSObject.cpp
@@ -3141,19 +3141,22 @@ CallResult<Handle<BigStorage>> getForInPropertyNames(
   Handle<HiddenClass> clazz(runtime, obj->getClass(runtime));
 
   // Fast case: Check the cache.
-  MutableHandle<BigStorage> arr(runtime, clazz->getForInCache(runtime));
-  if (arr) {
-    beginIndex = matchesProtoClasses(runtime, obj, arr);
-    if (beginIndex) {
-      // Cache is valid for this object, so use it.
-      endIndex = arr->size();
-      return arr;
-    }
-    // Invalid for this object. We choose to clear the cache since the
-    // changes to the prototype chain probably affect other objects too.
-    clazz->clearForInCache(runtime);
-    // Clear arr to slightly reduce risk of OOM from allocation below.
-    arr = nullptr;
+  MutableHandle<BigStorage> arr(runtime);
+  if (obj->shouldCacheForIn(runtime)) {
+    arr = clazz->getForInCache(runtime);
+    if (arr) {
+      beginIndex = matchesProtoClasses(runtime, obj, arr);
+      if (beginIndex) {
+        // Cache is valid for this object, so use it.
+        endIndex = arr->size();
+        return arr;
+      }
+      // Invalid for this object. We choose to clear the cache since the
+      // changes to the prototype chain probably affect other objects too.
+      clazz->clearForInCache(runtime);
+      // Clear arr to slightly reduce risk of OOM from allocation below.
+      arr = nullptr;
+    }
   }
 
   // Slow case: Build the array of properties.
diff --git a/test/hermes/for_in.js b/test/hermes/for_in.js
@@ -5,12 +5,8 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-// RUN: %hermes -O -gc-sanitize-handles=0 -target=HBC %s | %FileCheck --match-full-lines %s
-// RUN: %hermes -O -target=HBC -emit-binary -out %t.hbc %s && %hermes -gc-sanitize-handles=0 %t.hbc | %FileCheck --match-full-lines %s
-// REQUIRES: !slow_debug
-
-// This test took over 10 minutes with HandleSan, so -gc-sanitize-handles=0 is
-// passed to reduce the chances of tests timing out.
+// RUN: %hermes -O -target=HBC %s | %FileCheck --match-full-lines %s
+// RUN: %hermes -O -target=HBC -emit-binary -out %t.hbc %s && %hermes %t.hbc | %FileCheck --match-full-lines %s
 
 // Simple case
 var x = {};
@@ -113,14 +109,8 @@ for (var y in x) {
 //CHECK: s4 4
 //CHECK: s5 5
 
-// Large array (doesn't fit in single segment)
-var a = []
-for (var i = 0; i < 1000*1000; ++i) {
-  a[i] = i;
-}
-var t = 0;
-for (var p in a) {
-  t += 1;
-}
-print(t);
-//CHECK: 1000000
+// Don't probe parent chain of non-cacheable objects (proxy will assert)
+// 1. This creates a for-in cache entry for a particular hidden class
+for (var k in Number(0)) {}
+// 2. This Proxy uses the same hidden cache object internally.
+for (var k in new Proxy({},{})) {}
diff --git a/test/hermes/for_in_bigloop.js b/test/hermes/for_in_bigloop.js
@@ -0,0 +1,25 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+// RUN: %hermes -O -gc-sanitize-handles=0 -target=HBC %s | %FileCheck --match-full-lines %s
+// RUN: %hermes -O -target=HBC -emit-binary -out %t.hbc %s && %hermes -gc-sanitize-handles=0 %t.hbc | %FileCheck --match-full-lines %s
+// REQUIRES: !slow_debug
+
+// This test took over 10 minutes with HandleSan, so -gc-sanitize-handles=0 is
+// passed to reduce the chances of tests timing out.
+
+// Large array (doesn't fit in single segment)
+var a = []
+for (var i = 0; i < 1000*1000; ++i) {
+  a[i] = i;
+}
+var t = 0;
+for (var p in a) {
+  t += 1;
+}
+print(t);
+//CHECK: 1000000
