Bug 142936 – Array.prototype.shift should check exception after thisObj->get(exec, 0)

• Status: NEW
• Component: JavaScriptCore

[Yusuke Suzuki]

Array.prototype.unshift should check exception after thisObj->get(exec, 0) since it could raise exceptions.

[Yusuke Suzuki]

Created attachment 249973 [details]
Patch

[Yusuke Suzuki]

Created attachment 249974 [details]
Patch

[Darin Adler]

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.

[Geoffrey Garen]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

In fact, when dropping the workaround check in callGetter, we can see the regression with the attached test cases in this issue.

[Ahmad Saleem]

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!