Throw as per spec when modifying an Array with builtin methods

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index d48a30d8abc922dc4966c3d5e5413d7207f27f1c..27f5e3640cdf63d1dbbcbe9f7d3d4695ad09c325 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -13097,18 +13097,21 @@ bool JSArray::IsReadOnlyLengthDescriptor(Handle<Map> jsarray_map) {
 }
 
 
+bool JSArray::HasReadOnlyLength(Handle<JSArray> array) {

[adamk, 2014/11/13 23:43:29]

Should this just delegate to IsReadOnlyLengthDescriptor above? It's not clear to
me why the code in the two methods looks different...

[Michael Starzinger, 2014/11/14 17:00:22]

Acknowledged. I chatted with Toon about this, semantically these two methods
should do the same. But since the LookupIterator is the "better implementation",
we should keep this method and instead deprecate the other one. So I would just
keep it as it is for now (i.e. keep both methods) and maybe even add a TODO to
IsReadOnlyLengthDescriptor that it is deprecated.

[adamk, 2014/11/14 19:13:19]

Thanks for the background, added a TODO.

+  LookupIterator it(array, array->GetIsolate()->factory()->length_string(),
+                    LookupIterator::OWN_SKIP_INTERCEPTOR);
+  CHECK_NE(LookupIterator::ACCESS_CHECK, it.state());
+  CHECK(it.IsFound());
+  CHECK_EQ(LookupIterator::ACCESSOR, it.state());
+  return it.IsReadOnly();
+}
+
+
 bool JSArray::WouldChangeReadOnlyLength(Handle<JSArray> array,
                                         uint32_t index) {
   uint32_t length = 0;
   CHECK(array->length()->ToArrayIndex(&length));
-  if (length <= index) {
-    LookupIterator it(array, array->GetIsolate()->factory()->length_string(),
-                      LookupIterator::OWN_SKIP_INTERCEPTOR);
-    CHECK_NE(LookupIterator::ACCESS_CHECK, it.state());
-    CHECK(it.IsFound());
-    CHECK_EQ(LookupIterator::ACCESSOR, it.state());
-    return it.IsReadOnly();
-  }
+  if (length <= index) return HasReadOnlyLength(array);
   return false;
 }