Bug 144252 – Numeric setter on prototype doesn't get called.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Ryosuke Niwa]

var y = {set 2(x) { alert(x) }}
y[2] = 5;
var z = {__proto__: {set 3(x) { alert(x) }}};
z[3] = 7;

Chrome and Firefox both alerts 5 and 7.
Safari only alerts 5.

If you replace '3' with 'foo', Safari also alerts 7 and 7.

[Yusuke Suzuki]

I think there are 2 issues, correct?

1.
If the __proto__-executed object is categorized into ALL_BLANK_INDEXING_TYPES, we don't set SlowPut into its structure.


2.
And if the newly generated object is not the array, we don't set the SlowPut into its structure even if there's SlowPut structure in the [[Prototype]] chain.

[Ryosuke Niwa]

FWIW, I have no idea.

[Yusuke Suzuki]

Created attachment 258706 [details]
Patch

[Darin Adler]

Comment on attachment 258706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258706&action=review

> Source/JavaScriptCore/runtime/JSObject.cpp:2045
> +            // Reloop.

This word “reloop” doesn’t exist. I suggest removing this comment unless there’s something clear to say here.

[Darin Adler]

Comment on attachment 258706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258706&action=review

>> Source/JavaScriptCore/runtime/JSObject.cpp:2045
>> +            // Reloop.
> 
> This word “reloop” doesn’t exist. I suggest removing this comment unless there’s something clear to say here.

If we do write a comment here, I think the focus of the comment should be on why it’s safe to call putByIndex and why we’re guaranteed this won’t result in an infinite loop.

[Yusuke Suzuki]

Comment on attachment 258706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258706&action=review

Thank you!

>> Source/JavaScriptCore/runtime/JSObject.cpp:2045
>> +            // Reloop.
> 
> This word “reloop” doesn’t exist. I suggest removing this comment unless there’s something clear to say here.

Oh, I can see many "Reloop"s in JSObject.cpp[1]...
In the mean time, I'll drop this comment!

[1]: http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/JSObject.cpp

[Darin Adler]

(In reply to comment #6)
> Oh, I can see many "Reloop"s in JSObject.cpp[1]...

I guess I wasn’t there when these changes went in to discourage use of this “word”!

[Yusuke Suzuki]

Comment on attachment 258706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258706&action=review

>>>> Source/JavaScriptCore/runtime/JSObject.cpp:2045
>>>> +            // Reloop.
>>> 
>>> This word “reloop” doesn’t exist. I suggest removing this comment unless there’s something clear to say here.
>> 
>> If we do write a comment here, I think the focus of the comment should be on why it’s safe to call putByIndex and why we’re guaranteed this won’t result in an infinite loop.
> 
> Oh, I can see many "Reloop"s in JSObject.cpp[1]...
> In the mean time, I'll drop this comment!
> 
> [1]: http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/JSObject.cpp

> If we do write a comment here, I think the focus of the comment should be on why it’s safe to call putByIndex and why we’re guaranteed this won’t result in an infinite loop.

OK, I'll add the comment "Convert the indexing type to the SlowPutArrayStorage and retry.", what do you think of?

[Yusuke Suzuki]

Committed r188269: <http://trac.webkit.org/changeset/188269>

[Yusuke Suzuki]

(In reply to comment #8)
> Comment on attachment 258706 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258706&action=review
> 
> >>>> Source/JavaScriptCore/runtime/JSObject.cpp:2045
> >>>> +            // Reloop.
> >>> 
> >>> This word “reloop” doesn’t exist. I suggest removing this comment unless there’s something clear to say here.
> >> 
> >> If we do write a comment here, I think the focus of the comment should be on why it’s safe to call putByIndex and why we’re guaranteed this won’t result in an infinite loop.
> > 
> > Oh, I can see many "Reloop"s in JSObject.cpp[1]...
> > In the mean time, I'll drop this comment!
> > 
> > [1]: http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/JSObject.cpp
> 
> > If we do write a comment here, I think the focus of the comment should be on why it’s safe to call putByIndex and why we’re guaranteed this won’t result in an infinite loop.
> 
> OK, I'll add the comment "Convert the indexing type to the
> SlowPutArrayStorage and retry.", what do you think of?

In the meantime, I've added the above comment here.

[Ryosuke Niwa]

Comment on attachment 258706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258706&action=review

> LayoutTests/ChangeLog:8
> +        Update the test expectation file.

Nit: You're updating the expected result, not a test expectation file.
Test expectation files are files such as LayoutTests/TestExpectations that list the expected outcome of each test.

[Yusuke Suzuki]

Comment on attachment 258706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258706&action=review

>> LayoutTests/ChangeLog:8
>> +        Update the test expectation file.
> 
> Nit: You're updating the expected result, not a test expectation file.
> Test expectation files are files such as LayoutTests/TestExpectations that list the expected outcome of each test.

Oh, I see. Thank you for your comment :D