Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
555173002:
Array.prototype.sort: Unchecked calls to hasOwnProperty and push and sort (Closed)
|
Created:
6 years, 3 months ago by Diego Pino Modified:
6 years, 3 months ago CC:
aperez, vjaquez, gemont_igalia.com, v8-dev Project:
v8 Visibility:
Public. |
DescriptionArray.prototype.sort: Unchecked calls to hasOwnProperty and push and sort
BUG=v8:3537
LOG=
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fixed issues #Patch Set 3 : Added test #
Total comments: 5
Patch Set 4 : Fixed nits #
Total comments: 8
Patch Set 5 : Fixed last issues #
Total comments: 5
Patch Set 6 : Fixed 80-column line limit #
Messages
Total messages: 22 (6 generated)
dpino@igalia.com changed reviewers: + aperez@igalia.com, ceyusa@gmail.com, gemont@igalia.com, wingo@igalia.com
On 2014/09/09 16:22:47, Diego Pino wrote: > mailto:dpino@igalia.com changed reviewers: > + mailto:aperez@igalia.com, mailto:ceyusa@gmail.com, mailto:gemont@igalia.com, mailto:wingo@igalia.com Looks good to me, I think we can add now some owners for the final review.
dpino@igalia.com changed reviewers: + svenpanne@google.com
dpino@igalia.com changed reviewers: + mstarzinger@chromium.org
arv@chromium.org changed reviewers: + arv@chromium.org
https://codereview.chromium.org/555173002/diff/1/src/array.js File src/array.js (right): https://codereview.chromium.org/555173002/diff/1/src/array.js#newcode867 src/array.js:867: ArrayPush.call(t_array, [i, a[i]]); %_CallFunction(t_array, [i, a[i]], ArrayPush) But why are we using push here? Can't we just do? t_array[nextIndex] = [i, a[i]] https://codereview.chromium.org/555173002/diff/1/src/array.js#newcode869 src/array.js:869: ArraySort.call(t_array, function(a, b) { %_CallFunction https://codereview.chromium.org/555173002/diff/1/src/array.js#newcode966 src/array.js:966: var hasOwnProperty = $Object.prototype.hasOwnProperty; Either use %HasOwnProperty or ObjectHasOwnProperty. The benefit of the second one is that it will also work with Proxies but I'm not sure we need to handle those here? https://codereview.chromium.org/555173002/diff/1/src/array.js#newcode974 src/array.js:974: if (!hasOwnProperty.call(obj, i) && hasOwnProperty.call(proto, i)) { call could have been replaced too :'( Use %_CallFunction(obj, i, ObjectHasOwnProperty)
Missing a test; please add one. Thanks.
dpino@igalia.com changed reviewers: - aperez@igalia.com, ceyusa@gmail.com, gemont@igalia.com, mstarzinger@chromium.org, svenpanne@google.com
On 2014/09/15 12:36:51, wingo wrote: > Missing a test; please add one. Thanks. I updated array-sort.js and added a test for the issues the patch solves (Array.prototype.sort doesn't depend on Array.prototype.push, Array.prototype.sort and Object.prototype.hasOwnProperty). On running successfully the tests do not throw an Exception.
LGTM Just a few nits. https://codereview.chromium.org/555173002/diff/40001/test/mjsunit/array-sort.js File test/mjsunit/array-sort.js (right): https://codereview.chromium.org/555173002/diff/40001/test/mjsunit/array-sort.... test/mjsunit/array-sort.js:408: // Test sort doesn't depend on Object.prototype.hasOwnProperty. Otherwise a You can remove this comment. The name and the code explains what it does. https://codereview.chromium.org/555173002/diff/40001/test/mjsunit/array-sort.... test/mjsunit/array-sort.js:411: { no newline before { https://codereview.chromium.org/555173002/diff/40001/test/mjsunit/array-sort.... test/mjsunit/array-sort.js:412: Array.prototype.sort.call({__proto__: { hasOwnProperty: null, 0: 1 }, length: 5}); long line https://codereview.chromium.org/555173002/diff/40001/test/mjsunit/array-sort.... test/mjsunit/array-sort.js:415: Object.defineProperty(arr, "0", { get: function() {}, set: function() {}, Why the strange 0 property? It does not seem relevant to the test. Can you get away with? arr[0] = undefined; https://codereview.chromium.org/555173002/diff/40001/test/mjsunit/array-sort.... test/mjsunit/array-sort.js:430: delete Array.prototype.push; or Array.prototype.push = function() { fail('Should not call push'); };
On 2014/09/15 15:32:48, arv wrote: Fixed nits. > https://codereview.chromium.org/555173002/diff/40001/test/mjsunit/array-sort.... > test/mjsunit/array-sort.js:415: Object.defineProperty(arr, "0", { get: > function() {}, set: function() {}, > Why the strange 0 property? It does not seem relevant to the test. Can you get > away with? > > arr[0] = undefined; > hasOwnProperty is only triggered if arr[0] is defined as a property. It's not triggered if arr[0] is defined as an index.
On 2014/09/15 17:52:42, Diego Pino wrote: > On 2014/09/15 15:32:48, arv wrote: > > Fixed nits. > > > > https://codereview.chromium.org/555173002/diff/40001/test/mjsunit/array-sort.... > > test/mjsunit/array-sort.js:415: Object.defineProperty(arr, "0", { get: > > function() {}, set: function() {}, > > Why the strange 0 property? It does not seem relevant to the test. Can you get > > away with? > > > > arr[0] = undefined; > > > > hasOwnProperty is only triggered if arr[0] is defined as a property. It's not > triggered if arr[0] is defined as an index. I see. It needs to be an accessor and not an indexed data property?
https://codereview.chromium.org/555173002/diff/60001/src/array.js File src/array.js (right): https://codereview.chromium.org/555173002/diff/60001/src/array.js#newcode973 src/array.js:973: if (!%_CallFunction(obj, i, ObjectHasOwnProperty) && Maybe use a helper or a macro for these? macro HAS_OWN_PROPERTY(obj, n) %_CallFunction(obj, n, ObjectHasOwnProperty) endmacro https://codereview.chromium.org/555173002/diff/60001/test/mjsunit/array-sort.js File test/mjsunit/array-sort.js (right): https://codereview.chromium.org/555173002/diff/60001/test/mjsunit/array-sort.... test/mjsunit/array-sort.js:409: Array.prototype.sort.call({__proto__: { hasOwnProperty: null, 0: 1 }, Array.prototype.sort.call({ __proto__: { hasOwnProperty: null, 0: 1 }, length: 5 }); https://codereview.chromium.org/555173002/diff/60001/test/mjsunit/array-sort.... test/mjsunit/array-sort.js:413: Object.defineProperty(arr, "0", {}); Now I'm confused again ;-) Don't we treat the '0' as 0 when we do defineProperty? https://codereview.chromium.org/555173002/diff/60001/test/mjsunit/array-sort.... test/mjsunit/array-sort.js:425: fail('Should not call push'); indent 2 spaces. https://codereview.chromium.org/555173002/diff/60001/test/mjsunit/array-sort.... test/mjsunit/array-sort.js:426: } ; https://codereview.chromium.org/555173002/diff/60001/test/mjsunit/array-sort.... test/mjsunit/array-sort.js:444: } ;
https://codereview.chromium.org/555173002/diff/60001/test/mjsunit/array-sort.js File test/mjsunit/array-sort.js (right): https://codereview.chromium.org/555173002/diff/60001/test/mjsunit/array-sort.... test/mjsunit/array-sort.js:420: function TestSortDoesntDepenOnArrayPrototypePush() { "Depend". Also better to write out DoesNot instead of Doesnt
https://codereview.chromium.org/555173002/diff/60001/test/mjsunit/array-sort.js File test/mjsunit/array-sort.js (right): https://codereview.chromium.org/555173002/diff/60001/test/mjsunit/array-sort.... test/mjsunit/array-sort.js:413: Object.defineProperty(arr, "0", {}); On 2014/09/15 18:05:28, arv wrote: > Now I'm confused again ;-) > > Don't we treat the '0' as 0 when we do defineProperty? Yes, it's the same to use '0' or 0 in this case. I made a mistake in this test, the array needs to have at least two elements and the accessor needs to have a getter and a setter, otherwise there's a TypeError (Cannot assign to read only property '0' of [object Array]). As the array had only one element, it didn't really sort anything.
LGTM with nits, please add an OWNER https://codereview.chromium.org/555173002/diff/80001/src/array.js File src/array.js (right): https://codereview.chromium.org/555173002/diff/80001/src/array.js#newcode982 src/array.js:982: && !HAS_OWN_PROPERTY(obj, index) && HAS_OWN_PROPERTY(proto, index)) { Nit: 80 columns. (Don't the presubmit checks flag this for you?) https://codereview.chromium.org/555173002/diff/80001/test/mjsunit/array-sort.js File test/mjsunit/array-sort.js (right): https://codereview.chromium.org/555173002/diff/80001/test/mjsunit/array-sort.... test/mjsunit/array-sort.js:419: I think the style is to join the invocation with the definition in tests, like function foo() { ... } foo(); i.e. no empty line
https://codereview.chromium.org/555173002/diff/80001/src/array.js File src/array.js (right): https://codereview.chromium.org/555173002/diff/80001/src/array.js#newcode982 src/array.js:982: && !HAS_OWN_PROPERTY(obj, index) && HAS_OWN_PROPERTY(proto, index)) { On 2014/09/16 12:11:53, wingo wrote: > Nit: 80 columns. (Don't the presubmit checks flag this for you?) It seems not, I got warnings about trailing whitespaces, for instance, but not about a line being longer than 80 columns :/ https://codereview.chromium.org/555173002/diff/80001/test/mjsunit/array-sort.js File test/mjsunit/array-sort.js (right): https://codereview.chromium.org/555173002/diff/80001/test/mjsunit/array-sort.... test/mjsunit/array-sort.js:419: On 2014/09/16 12:11:53, wingo wrote: > I think the style is to join the invocation with the definition in tests, like > > function foo() { > ... > } > foo(); > > i.e. no empty line OK, this is actually two tests that both have to do with HasOwnProperty, that's why I put an empty line. Should I separate each in its own test?
https://codereview.chromium.org/555173002/diff/80001/test/mjsunit/array-sort.js File test/mjsunit/array-sort.js (right): https://codereview.chromium.org/555173002/diff/80001/test/mjsunit/array-sort.... test/mjsunit/array-sort.js:419: On 2014/09/16 at 13:23:20, Diego Pino wrote: > On 2014/09/16 12:11:53, wingo wrote: > > I think the style is to join the invocation with the definition in tests, like > > > > function foo() { > > ... > > } > > foo(); > > > > i.e. no empty line > > OK, this is actually two tests that both have to do with HasOwnProperty, that's why I put an empty line. Should I separate each in its own test? It is fine to keep them in one function. I believe Andy was saying that the empty line before the call to `TestSortDoesNotDependOnObjectPrototypeHasOwnProperty()` should be removed. Since you're just following the style of this file I think either way is fine.
arv@chromium.org changed reviewers: + yangguo@chromium.org
LGTM I'm not an owner. Adding Yang.
On 2014/09/16 13:44:02, arv wrote: > LGTM > > I'm not an owner. Adding Yang. lgtm.
On 2014/09/16 14:31:50, Yang wrote: > On 2014/09/16 13:44:02, arv wrote: > > LGTM > > > > I'm not an owner. Adding Yang. > > lgtm. Yeeps! I went to land this but didn't set the appropriate patch-by line. Apologies :-(( Landed in http://code.google.com/p/v8/source/detail?r=24005. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
