Array.prototype.unshift should check exception after thisObj->get(exec, 0) since it could raise exceptions.
Created attachment 249973 [details] Patch
Created attachment 249974 [details] Patch
Comment on attachment 249974 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249974&action=review I’d like us to find an efficient way to write a lot of tests like this one. I’m not so happy with the code change because it doesn’t fix anything at this time! But I’ll say r=me since I assume you are trying to get to the point where we can remove the callGetter check. > Source/JavaScriptCore/ChangeLog:9 > + Currently, it's not observable. I don’t understand why we would make the change if we can’t observe any benefit. I think it means we added more code for no reason. > Source/JavaScriptCore/ChangeLog:11 > + But I think it's not intended to become unobservable > + because this is guarded by callGetter's workaround check noted as FIXME. Seems that we could deal with this later then. The important part is to write and run the test. I don’t want us to add code that has no observable effect.
Comment on attachment 249974 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249974&action=review > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:616 > result = thisObj->get(exec, 0); > + if (exec->hadException()) > + return JSValue::encode(jsUndefined()); > shift<JSArray::ShiftCountForShift>(exec, thisObj, 0, 1, 0, length); Doesn't the call to shift have observable side effects?
Comment on attachment 249974 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249974&action=review >> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:616 >> shift<JSArray::ShiftCountForShift>(exec, thisObj, 0, 1, 0, length); > > Doesn't the call to shift have observable side effects? Before calling observable side effect like calling getters in the shift function, workaround exception checking in callGetter code works and it immediately returns.
In fact, when dropping the workaround check in callGetter, we can see the regression with the attached test cases in this issue.
I checked the bug ID in Webkit GitHub but it seems this r+ patch didn't landed. But this patch was trying to modify following portion of code +/- few lines: https://github.com/WebKit/WebKit/blob/3c9c31d0fd7c9a2c6579790319f90dc88f98f4f5/Source/JavaScriptCore/runtime/ArrayPrototype.cpp#L1024 Do we need this or we can land only test or we don't need it at all. Appreciate if someone can comment and do actions accordingly. Thanks!