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.
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.
FWIW, I have no idea.
Created attachment 258706 [details] Patch
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.
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.
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
(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”!
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?
Committed r188269: <http://trac.webkit.org/changeset/188269>
(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.
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.
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