Fix interceptor state reset by LookupIterator changes in ObjectDefine…

…OwnProperty (#16)

https://chromium-review.googlesource.com/c/v8/v8/+/5205671

Reverts this part of the CL:

> 5. Clean up OrdinaryDefineOwnProperty to remove an unnecessary overload
    (which was just splitting the logic over two functions), to avoid an
    unnecessary loop looking for interceptors, and to avoid an
    unnecessary iterator restart when such an interceptor is not found.

patches/0003-Fix-Interceptor-being-reset-by-LookupIterator-change.patch

@@ -0,0 +1,82 @@
+From f498f86cecc69cf82cb316452f33505204c2df5a Mon Sep 17 00:00:00 2001
+From: Divy Srivastava <[email protected]>
+Date: Fri, 8 Mar 2024 06:16:46 +0000
+Subject: [PATCH] Fix Interceptor being reset by LookupIterator changes
+
+---
+ src/objects/js-objects.cc | 48 ++++++++++++---------------------------
+ 1 file changed, 14 insertions(+), 34 deletions(-)
+
+diff --git a/src/objects/js-objects.cc b/src/objects/js-objects.cc
+index af21b605e3..7f6513d925 100644
+--- a/src/objects/js-objects.cc
++++ b/src/objects/js-objects.cc
+@@ -1380,52 +1380,32 @@ Maybe<bool> JSReceiver::OrdinaryDefineOwnProperty(
+     PropertyDescriptor* desc, Maybe<ShouldThrow> should_throw) {
+   LookupIterator it(isolate, object, key, LookupIterator::OWN);
+
+-  // Deal with access checks first.
+-  if (it.state() == LookupIterator::ACCESS_CHECK) {
+-    if (!it.HasAccess()) {
+-      RETURN_ON_EXCEPTION_VALUE(
+-          isolate, isolate->ReportFailedAccessCheck(it.GetHolder<JSObject>()),
+-          Nothing<bool>());
+-      UNREACHABLE();
+-    }
+-    it.Next();
+-  }
+-
+   // 1. Let current be O.[[GetOwnProperty]](P).
+   // 2. ReturnIfAbrupt(current).
+   PropertyDescriptor current;
+   MAYBE_RETURN(GetOwnPropertyDescriptor(&it, &current), Nothing<bool>());
+
+-  // TODO(jkummerow/verwaest): It would be nice if we didn't have to reset
+-  // the iterator every time. Currently, the reasons why we need it are because
+-  // GetOwnPropertyDescriptor can have side effects, namely:
+-  // - Interceptors
+-  // - Accessors (which might change the holder's map)
+   it.Restart();
+-
+-  // Skip over the access check after restarting -- we've already checked it.
+-  if (it.state() == LookupIterator::ACCESS_CHECK) {
+-    DCHECK(it.HasAccess());
+-    it.Next();
+-  }
+-
+-  // Handle interceptor.
+-  if (it.state() == LookupIterator::INTERCEPTOR) {
+-    if (it.HolderIsReceiverOrHiddenPrototype()) {
+-      Maybe<bool> result = DefinePropertyWithInterceptorInternal(
+-          &it, it.GetInterceptor(), should_throw, desc);
+-      if (result.IsNothing() || result.FromJust()) {
+-        return result;
++  // Handle interceptor
++  for (; it.IsFound(); it.Next()) {
++    if (it.state() == LookupIterator::INTERCEPTOR) {
++      if (it.HolderIsReceiverOrHiddenPrototype()) {
++        Maybe<bool> result = DefinePropertyWithInterceptorInternal(
++            &it, it.GetInterceptor(), should_throw, desc);
++        if (result.IsNothing() || result.FromJust()) {
++          return result;
++        }
+       }
+-      // We need to restart the lookup in case the accessor ran with side
+-      // effects.
+-      it.Restart();
+     }
+   }
+
++  // TODO(jkummerow/verwaest): It would be nice if we didn't have to reset
++  // the iterator every time. Currently, the reasons why we need it are:
++  // - handle interceptors correctly
++  // - handle accessors correctly (which might change the holder's map)
++  it.Restart();
+   // 3. Let extensible be the value of the [[Extensible]] internal slot of O.
+   bool extensible = JSObject::IsExtensible(isolate, object);
+-
+   return ValidateAndApplyPropertyDescriptor(
+       isolate, &it, extensible, desc, &current, should_throw, Handle<Name>());
+ }
+--
+2.34.1