Currently builtins are using push and shift directly, which can be modified by user scripts.
Created attachment 265971 [details] Patch
Created attachment 266355 [details] Adding tests
Comment on attachment 266355 [details] Adding tests View in context: https://bugs.webkit.org/attachment.cgi?id=266355&action=review > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:100 > - JSC_NATIVE_INTRINSIC_FUNCTION("push", arrayProtoFuncPush, DontEnum, 1, ArrayPushIntrinsic); > + JSC_NATIVE_INTRINSIC_FUNCTION(vm.propertyNames->builtinNames().pushPublicName(), arrayProtoFuncPush, DontEnum, 1, ArrayPushIntrinsic); > + JSC_NATIVE_INTRINSIC_FUNCTION(vm.propertyNames->builtinNames().pushPrivateName(), arrayProtoFuncPush, DontEnum | DontDelete | ReadOnly, 1, ArrayPushIntrinsic); This makes me think even harder that we need to find a proper solution for this, as adding internal slots with the original function seems like a workaround and I am almost sure we are forgetting to fix something. > LayoutTests/streams/streams-promises.html:160 > + const ArrayPushBackup = Array.prototype.push; > + const ArrayShiftBackup = Array.prototype.shift; > + > + function cleanTest() { > + Array.prototype.push = ArrayPushBackup; > + Array.prototype.shift = ArrayShiftBackup; > + } I guess at some point it would be interesting to implement a setup and tear down for the tests.
Comment on attachment 266355 [details] Adding tests View in context: https://bugs.webkit.org/attachment.cgi?id=266355&action=review > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:103 > + JSC_NATIVE_FUNCTION(vm.propertyNames->builtinNames().shiftPublicName(), arrayProtoFuncShift, DontEnum, 0); > + JSC_NATIVE_FUNCTION(vm.propertyNames->builtinNames().shiftPrivateName(), arrayProtoFuncShift, DontEnum | DontDelete | ReadOnly, 0); Wrong function here.
Comment on attachment 266355 [details] Adding tests View in context: https://bugs.webkit.org/attachment.cgi?id=266355&action=review >> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:100 >> + JSC_NATIVE_INTRINSIC_FUNCTION(vm.propertyNames->builtinNames().pushPrivateName(), arrayProtoFuncPush, DontEnum | DontDelete | ReadOnly, 1, ArrayPushIntrinsic); > > This makes me think even harder that we need to find a proper solution for this, as adding internal slots with the original function seems like a workaround and I am almost sure we are forgetting to fix something. The latter is indeed worrying. >> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:103 >> + JSC_NATIVE_FUNCTION(vm.propertyNames->builtinNames().shiftPrivateName(), arrayProtoFuncShift, DontEnum | DontDelete | ReadOnly, 0); > > Wrong function here. Can you be more explicit about what is wrong here?
Comment on attachment 266355 [details] Adding tests View in context: https://bugs.webkit.org/attachment.cgi?id=266355&action=review >>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:103 >>> + JSC_NATIVE_FUNCTION(vm.propertyNames->builtinNames().shiftPrivateName(), arrayProtoFuncShift, DontEnum | DontDelete | ReadOnly, 0); >> >> Wrong function here. > > Can you be more explicit about what is wrong here? Oops. I take it back.
Created attachment 266516 [details] Improving test
> > LayoutTests/streams/streams-promises.html:160 > > + const ArrayPushBackup = Array.prototype.push; > > + const ArrayShiftBackup = Array.prototype.shift; > > + > > + function cleanTest() { > > + Array.prototype.push = ArrayPushBackup; > > + Array.prototype.shift = ArrayShiftBackup; > > + } > > I guess at some point it would be interesting to implement a setup and tear > down for the tests. Right. I improved a bit the test as modifying Array.prototype.push is making testharness.js unhappy when cleanTest is not called or not called early enough. Current version will mark test as failing if an assertion does not pass, which is better than timing out.
Comment on attachment 266516 [details] Improving test Clearing flags on attachment: 266516 Committed r193899: <http://trac.webkit.org/changeset/193899>
All reviewed patches have been landed. Closing bug.