[accessors] handle `writable` changing during ArrayLengthSetter

src/accessors.cc

 // Copyright 2012 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.
 
 #include "src/accessors.h"
 
 #include "src/api.h"
 #include "src/contexts.h"
 #include "src/deoptimizer.h"
 #include "src/execution.h"
@@ skipping 153 matching lines @@
 void Accessors::ArrayLengthSetter(
     v8::Local<v8::Name> name, v8::Local<v8::Value> val,
     const v8::PropertyCallbackInfo<v8::Boolean>& info) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(info.GetIsolate());
   HandleScope scope(isolate);
 
   Handle<JSReceiver> object = Utils::OpenHandle(*info.Holder());
   Handle<JSArray> array = Handle<JSArray>::cast(object);
   Handle<Object> length_obj = Utils::OpenHandle(*val);
 
+  bool was_readonly;
+  {
+    LookupIterator it(object, Utils::OpenHandle(*name), LookupIterator::OWN);
+    DCHECK(it.IsFound());
+    was_readonly = it.property_details().IsReadOnly();
+  }
+
   uint32_t length = 0;
   if (!JSArray::AnythingToArrayLength(isolate, length_obj, &length)) {
     isolate->OptionalRescheduleException(false);
     return;
   }
 
+  // AnythingToArrayLength() may have called setter re-entrantly and modified
+  // its property descriptor. Don't perform this check if "length" was
+  // previously readonly, as this may have been called during
+  // DefineOwnPropertyIgnoreAttributes().
+  LookupIterator it(object, Utils::OpenHandle(*name), LookupIterator::OWN);

[Camillo Bruni, 2016/11/30 12:08:27]

I think you want to delay the LookupIterator creation as far as possible here.
if (!was_readonly && length != array->length()->Number()) {
  LookupIterator it(...);
  if (V8_UNLIKELY(...)) {...

[caitp, 2016/11/30 13:17:58]

Done, but I think with that condition, it gets created in the vast majority of
cases, so it probably doesn't add much in terms of savings.

Maybe PropertyCallbackInfo should include a representation of property details
to at least avoid the first lookup iterator on line 174?

+  DCHECK(it.IsFound());
+  if (!was_readonly && V8_UNLIKELY(it.property_details().IsReadOnly()) &&
+      length != array->length()->Number()) {
+    if (info.ShouldThrowOnError()) {
+      Factory* factory = isolate->factory();
+      isolate->Throw(*factory->NewTypeError(
+          MessageTemplate::kStrictReadOnlyProperty, Utils::OpenHandle(*name),
+          i::Object::TypeOf(isolate, object), object));
+      isolate->OptionalRescheduleException(false);
+    } else {
+      info.GetReturnValue().Set(false);
+    }
+    return;
+  }
+
   JSArray::SetLength(array, length);
 
   uint32_t actual_new_len = 0;
   CHECK(array->length()->ToArrayLength(&actual_new_len));
   // Fail if there were non-deletable elements.
   if (actual_new_len != length) {
     if (info.ShouldThrowOnError()) {
       Factory* factory = isolate->factory();
       isolate->Throw(*factory->NewTypeError(
           MessageTemplate::kStrictDeleteProperty,
@@ skipping 1034 matching lines @@
 Handle<AccessorInfo> Accessors::ErrorStackInfo(Isolate* isolate,
                                                PropertyAttributes attributes) {
   Handle<AccessorInfo> info =
       MakeAccessor(isolate, isolate->factory()->stack_string(),
                    &ErrorStackGetter, &ErrorStackSetter, attributes);
   return info;
 }
 
 }  // namespace internal
 }  // namespace v8