put_by_val optimizations inline the hole and out-of-bounds store case. But this is subtly wrong if the prototype of the base object has setters. This can be side-stepped while preserving the optimization by: 1) If an object with indexed property storage transitions to having a prototype that has or once did have indexed accessors, then its indexing type should be set to ArrayStorageSlowHoles 2) If an object with a prototype that has or once did have indexed accessors transitions to having indexed property storage, then its indexing type should be set to ArrayStorageSlowHoles 3) If an object known to be used as a prototype transitions to having indexed accessors, then (I) its global object should have the havingABadTime flag set, (II) a heap walk should commence, (III) all structures belonging to that global object that have an indexing type indicating the presence of ArrayStorage should be changed in-place to have be ArrayStorageSlowHoles, (IV) a watchpoint for havingABadTime should be fired. 4) If an object transitions to having indexing property storage while its structure's global object has the havingABadTime set then the indexing type should be set to ArrayStorageSlowHoles. 5) If a structure check is used to enforce an object having indexing storage that is not ArrayStorageSlowHoles, then we should also emit a speculation watchpoint for that global object's havingABadTime flag.
*** Bug 96605 has been marked as a duplicate of this bug. ***
Also this test fails on Qt (fast/js/primitive-property-access-edge-cases.html). I'm going to skip it until the proper fix.
Created attachment 164025 [details] work in progress
Created attachment 164268 [details] moar Almost there.
Created attachment 164301 [details] the patch It seems to work!
Attachment 164301 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/runtime/SparseArrayValueMap.h:49: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/SparseArrayValueMap.h:49: The parameter name "map" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/SparseArrayValueMap.h:49: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSObject.h:529: The parameter name "globalData" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 164309 [details] almost ready to land Still need to rebase some tests, and add some new tests.
(In reply to comment #7) > Created an attachment (id=164309) [details] > almost ready to land > > Still need to rebase some tests, and add some new tests. It looks like I missed some cases! Put_by_val to primitives completely side-steps the JSObject::put logic.
Created attachment 164311 [details] the patch The differences from the previously reviewed patch: - Added JSObject::unwrappedGlobalObject(), since globalObject() doesn't work for global-this. That was causing assertion failures. - Added JSValue::putToPrimitiveByIndex() and refactored JSObject::attemptToInterceptPutByIndexOnHole() so that the logic can be shared.
Created attachment 164326 [details] patch with new tests
Created attachment 164329 [details] rebased patch
Attachment 164329 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/JavaScriptCore/runtime/JSObject.h:506: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSObject.h:506: The parameter name "i" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSObject.h:506: The parameter name "v" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 164331 [details] the patch Fix style
Comment on attachment 164331 [details] the patch Attachment 164331 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13863701 New failing tests: fast/js/array-bad-time.html fast/js/object-bad-time.html fast/js/object-slow-put.html fast/js/array-slow-put.html
(In reply to comment #14) > (From update of attachment 164331 [details]) > Attachment 164331 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/13863701 > > New failing tests: > fast/js/array-bad-time.html > fast/js/object-bad-time.html > fast/js/object-slow-put.html > fast/js/array-slow-put.html I don't see the failures in the output. I'm going to ignore.
Comment on attachment 164331 [details] the patch I'd like to see a test case that cover a couple of cross-frame scenarios. E.g. I have two frames, one adds a numeric setter to its array prototype, the second frame behaves correctly wrt to array objects from the first frame. (The patch looks like it will handle any cases I can think of correctly, since having a bad time is related to array creation, just would like to see test coverage for this). Still, this is awesome enough as is & there is plenty of test coverage already in this patch, so r+.
*** Bug 42801 has been marked as a duplicate of this bug. ***
Oh, I've marked bug#42801 as a dupe of this, could you quickly test brendan's test case & check if it's covered by the test cases you have already, & if not please add. Cheers, G.
(In reply to comment #18) > Oh, I've marked bug#42801 as a dupe of this, could you quickly test brendan's test case & check if it's covered by the test cases you have already, & if not please add. Cheers, G. Yup, his test is virtually identical to fast/js/array-slow-put.
(In reply to comment #16) > (From update of attachment 164331 [details]) > I'd like to see a test case that cover a couple of cross-frame scenarios. E.g. I have two frames, one adds a numeric setter to its array prototype, the second frame behaves correctly wrt to array objects from the first frame. (The patch looks like it will handle any cases I can think of correctly, since having a bad time is related to array creation, just would like to see test coverage for this). I can add such a test, but this will trivially work since PutByVal's and GetByVal's don't make assumptions about whether or not you're having a bad time in either the code's frame, the object's frame, or any other frame. They just do either a structure check (which will fail if the object's frame started having a bad time after the structure check was compiled) or an indexing type check (which, similarly to the structure check, will fail if the object's frame started having a bad time) or no inlined check at all (if it's a generic array access which will always go to C++ code, which will do the right things). > > Still, this is awesome enough as is & there is plenty of test coverage already in this patch, so r+.
Landed in http://trac.webkit.org/changeset/128802
(In reply to comment #15) > (In reply to comment #14) > > (From update of attachment 164331 [details] [details]) > > Attachment 164331 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/13863701 > > > > New failing tests: > > fast/js/array-bad-time.html > > fast/js/object-bad-time.html > > fast/js/object-slow-put.html > > fast/js/array-slow-put.html > > I don't see the failures in the output. I'm going to ignore. Actually I figured it out. Chrome has non-compliant behavior with respect to indexed accessors and read-only properties. Nothing I can do about that.
(In reply to comment #21) > Landed in http://trac.webkit.org/changeset/128802 It caused regression everywhere - https://bugs.webkit.org/show_bug.cgi?id=97001 Could you check it, please?
(In reply to comment #22) > (In reply to comment #15) > > (In reply to comment #14) > > > (From update of attachment 164331 [details] [details] [details]) > > > Attachment 164331 [details] [details] [details] did not pass chromium-ews (chromium-xvfb): > > > Output: http://queues.webkit.org/results/13863701 > > > > > > New failing tests: > > > fast/js/array-bad-time.html > > > fast/js/object-bad-time.html > > > fast/js/object-slow-put.html > > > fast/js/array-slow-put.html > > > > I don't see the failures in the output. I'm going to ignore. > > Actually I figured it out. Chrome has non-compliant behavior with respect to indexed accessors and read-only properties. > > Nothing I can do about that. Chrome issue on file? /be
(In reply to comment #24) > (In reply to comment #22) > > (In reply to comment #15) > > > (In reply to comment #14) > > > > (From update of attachment 164331 [details] [details] [details] [details]) > > > > Attachment 164331 [details] [details] [details] [details] did not pass chromium-ews (chromium-xvfb): > > > > Output: http://queues.webkit.org/results/13863701 > > > > > > > > New failing tests: > > > > fast/js/array-bad-time.html > > > > fast/js/object-bad-time.html > > > > fast/js/object-slow-put.html > > > > fast/js/array-slow-put.html > > > > > > I don't see the failures in the output. I'm going to ignore. > > > > Actually I figured it out. Chrome has non-compliant behavior with respect to indexed accessors and read-only properties. > > > > Nothing I can do about that. > > Chrome issue on file? > > /be Well, it would have been on file if the tests caused these tests to fail on their bots. But the tests aren't failing on their bots, so it may be that these particular failures were spurious and they've actually fixed the issue in their tree. In any case, unless they preemptively skipped these tests (I see no evidence of that in the repo), then either (a) the tests will pass because they are conformant or (b) the tests will fail and they'll find out about it.