[ic] Use slow stub when storing non-existent properties to global object

In strict mode stores to non-existent properties throw. We should not
install a handler with the property cell for such stores. These handlers
would expect that the value exists when they see a property cell. If
this property cell gets invalidated later, it appears as if it is a
valid property cell with undefined value. This leads to an incorrect
behaviour. This cl checks if we are in strict mode and uses a slow
stub in such cases.

Bug: chromium:1067757
Change-Id: I543c6a6931530bfb13cc9a33d1dabaa756489fd1
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2142255
Commit-Queue: Mythri Alle <mythria@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67151}

src/ic/ic.cc

@@ -1423,6 +1423,28 @@ bool StoreIC::LookupForWrite(LookupIterator* it, Handle<Object> value,
     }
   }
 
+  // If we are in StoreGlobal then check if we should throw on non-existent
+  // properties.
+  if (IsStoreGlobalIC() &&
+      (GetShouldThrow(it->isolate(), Nothing<ShouldThrow>()) ==
+       ShouldThrow::kThrowOnError)) {
+    // ICs typically does the store in two steps: prepare receiver for the
+    // transition followed by the actual store. For global objects we create a
+    // property cell when preparing for transition and install this cell in the
+    // handler. In strict mode, we throw and never initialize this property
+    // cell. The IC handler assumes that the property cell it is holding is for
+    // a property that is existing. This case violates this assumption. If we
+    // happen to invalidate this property cell later, it leads to incorrect
+    // behaviour. For now just use a slow stub and don't install the property
+    // cell for these cases. Hopefully these cases are not frequent enough to
+    // impact performance.
+    //
+    // TODO(mythria): If we find this to be happening often, we could install a
+    // new kind of handler for non-existent properties. These handlers can then
+    // miss to runtime if the value is not hole (i.e. cell got invalidated) and
+    // handle these stores correctly.
+    return false;
+  }
   receiver = it->GetStoreTarget<JSObject>();
   if (it->ExtendingNonExtensible(receiver)) return false;
   it->PrepareTransitionToDataProperty(receiver, value, NONE, store_origin);

test/mjsunit/regress/regress-crbug-1067757.js

+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --no-lazy-feedback-allocation
+
+"use strict";
+
+function foo() {
+  let count = 0;
+  try {
+    for (p of v) {
+      count += 1;
+    }
+  } catch (e) { }
+  assertEquals(count, 0);
+}
+
+var v = [ "0", {}];
+
+foo();
+Reflect.deleteProperty(v, '0');
+
+let count_loop = 0;
+try {
+ for (p of v) { count_loop += 1; }
+} catch (e) {}
+assertEquals(count_loop, 0);
+
+foo();