From 9bebd23d5c349a9ed5c374ee73efe18e0a64303b Mon Sep 17 00:00:00 2001 From: "adamk@chromium.org" Date: Fri, 22 Mar 2013 18:04:32 +0000 Subject: [PATCH] Fix %GetArrayKeys to not skip non-enumerable indices This is one step in the direction of fixing a range of small bugs in the array methods when dealing with non-standard element attributes. Added tests exercising this behavior for shift and unshift. For Proxies and Interceptors, the behavior of %GetArrayKeys is now to just return an interval, rather than trying to list all their indexed properties. In the Proxy case, this seems like the only way to avoid an observable difference between smart and non-smart array methods. For Interceptors, the usual case (in WebKit, anyway) is for them to have all indices in [0, length), so enumerating them won't be any better than simply iterating over that range. Review URL: https://codereview.chromium.org/12653010 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@14057 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/runtime.cc | 56 ++++++++++++++++++++++++------------------- test/mjsunit/array-shift.js | 14 +++++++++++ test/mjsunit/array-unshift.js | 15 ++++++++++++ 3 files changed, 60 insertions(+), 25 deletions(-) diff --git a/src/runtime.cc b/src/runtime.cc index 2626f9a..4aa334b 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -9558,6 +9558,15 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_EstimateNumberOfElements) { } +static Handle NewSingleInterval(Isolate* isolate, uint32_t length) { + Handle single_interval = isolate->factory()->NewFixedArray(2); + // -1 means start of array. + single_interval->set(0, Smi::FromInt(-1)); + single_interval->set(1, *isolate->factory()->NewNumberFromUint(length)); + return isolate->factory()->NewJSArrayWithElements(single_interval); +} + + // Returns an array that tells you where in the [0, length) interval an array // might have elements. Can either return keys (positive integers) or // intervals (pair of a negative integer (-start-1) followed by a @@ -9569,37 +9578,34 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_GetArrayKeys) { CONVERT_ARG_HANDLE_CHECKED(JSObject, array, 0); CONVERT_NUMBER_CHECKED(uint32_t, length, Uint32, args[1]); if (array->elements()->IsDictionary()) { - // Create an array and get all the keys into it, then remove all the - // keys that are not integers in the range 0 to length-1. - bool threw = false; - Handle keys = - GetKeysInFixedArrayFor(array, INCLUDE_PROTOS, &threw); - if (threw) return Failure::Exception(); - - int keys_length = keys->length(); - for (int i = 0; i < keys_length; i++) { - Object* key = keys->get(i); - uint32_t index = 0; - if (!key->ToArrayIndex(&index) || index >= length) { - // Zap invalid keys. - keys->set_undefined(i); + Handle keys = isolate->factory()->empty_fixed_array(); + for (Handle p = array; + !p->IsNull(); + p = Handle(p->GetPrototype(isolate), isolate)) { + if (p->IsJSProxy() || JSObject::cast(*p)->HasIndexedInterceptor()) { + // Bail out if we find a proxy or interceptor, likely not worth + // collecting keys in that case. + return *NewSingleInterval(isolate, length); } + Handle current = Handle::cast(p); + Handle current_keys = + isolate->factory()->NewFixedArray( + current->NumberOfLocalElements(NONE)); + current->GetLocalElementKeys(*current_keys, NONE); + keys = UnionOfKeys(keys, current_keys); + } + // Erase any keys >= length. + // TODO(adamk): Remove this step when the contract of %GetArrayKeys + // is changed to let this happen on the JS side. + for (int i = 0; i < keys->length(); i++) { + if (NumberToUint32(keys->get(i)) >= length) keys->set_undefined(i); } return *isolate->factory()->NewJSArrayWithElements(keys); } else { ASSERT(array->HasFastSmiOrObjectElements() || array->HasFastDoubleElements()); - Handle single_interval = isolate->factory()->NewFixedArray(2); - // -1 means start of array. - single_interval->set(0, Smi::FromInt(-1)); - FixedArrayBase* elements = FixedArrayBase::cast(array->elements()); - uint32_t actual_length = - static_cast(elements->length()); - uint32_t min_length = actual_length < length ? actual_length : length; - Handle length_object = - isolate->factory()->NewNumber(static_cast(min_length)); - single_interval->set(1, *length_object); - return *isolate->factory()->NewJSArrayWithElements(single_interval); + uint32_t actual_length = static_cast(array->elements()->length()); + return *NewSingleInterval(isolate, Min(actual_length, length)); } } diff --git a/test/mjsunit/array-shift.js b/test/mjsunit/array-shift.js index 3601cbb..ad742e1 100644 --- a/test/mjsunit/array-shift.js +++ b/test/mjsunit/array-shift.js @@ -106,3 +106,17 @@ assertEquals(array[7], array_proto[7]); assertFalse(array.hasOwnProperty(7)); })(); + +// Check that non-enumerable elements are treated appropriately +(function() { + var array = [1, 2, 3]; + Object.defineProperty(array, '1', {enumerable: false}); + assertEquals(1, array.shift()); + assertEquals([2, 3], array); + + array = [1,,3]; + array.__proto__[1] = 2; + Object.defineProperty(array.__proto__, '1', {enumerable: false}); + assertEquals(1, array.shift()); + assertEquals([2, 3], array); +})(); diff --git a/test/mjsunit/array-unshift.js b/test/mjsunit/array-unshift.js index 4010601..0eb299a 100644 --- a/test/mjsunit/array-unshift.js +++ b/test/mjsunit/array-unshift.js @@ -213,3 +213,18 @@ assertEquals([1, 2, 3, 4, 5, 6, 7, 8, 9], a); } })(); + +// Check that non-enumerable elements are treated appropriately +(function() { + var array = [2, 3]; + Object.defineProperty(array, '1', {enumerable: false}); + array.unshift(1) + assertEquals([1, 2, 3], array); + + array = [2]; + array.length = 2; + array.__proto__[1] = 3; + Object.defineProperty(array.__proto__, '1', {enumerable: false}); + array.unshift(1); + assertEquals([1, 2, 3], array); +})(); -- 2.7.4