Bug 96596

Summary: If a prototype has indexed setters and its instances have indexed storage, then all put_by_val's should have a bad time
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, brendan, dglazkov, ggaren, kadam, mhahnenberg, msaboff, oliver, ossy, rakuco, webkit.review.bot, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 91933, 96611, 97001    
Bug Blocks: 96962    
Attachments:
Description Flags
work in progress
none
moar
none
the patch
oliver: review+
almost ready to land
none
the patch
none
patch with new tests
none
rebased patch
none
the patch barraclough: review+, webkit.review.bot: commit-queue-

Filip Pizlo
Reported 2012-09-12 21:05:34 PDT
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.
Attachments
work in progress (33.42 KB, patch)
2012-09-13 18:41 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
moar (54.69 KB, patch)
2012-09-14 20:01 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (78.56 KB, patch)
2012-09-15 19:04 PDT, Filip Pizlo 		oliver: review+
DetailsFormatted DiffDiff
almost ready to land (82.48 KB, patch)
2012-09-15 23:24 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (87.66 KB, patch)
2012-09-16 00:35 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
patch with new tests (174.09 KB, patch)
2012-09-16 16:19 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
rebased patch (173.39 KB, patch)
2012-09-16 18:02 PDT, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (173.39 KB, patch)
2012-09-16 18:10 PDT, Filip Pizlo 		barraclough: review+
webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2012-09-12 23:41:44 PDT
*** Bug 96605 has been marked as a duplicate of this bug. ***
Ádám Kallai
Comment 2 2012-09-13 02:13:37 PDT
Also this test fails on Qt (fast/js/primitive-property-access-edge-cases.html). I'm going to skip it until the proper fix.
Filip Pizlo
Comment 3 2012-09-13 18:41:30 PDT
Created attachment 164025 [details] work in progress
Filip Pizlo
Comment 4 2012-09-14 20:01:39 PDT
Created attachment 164268 [details] moar Almost there.
Filip Pizlo
Comment 5 2012-09-15 19:04:31 PDT
Created attachment 164301 [details] the patch It seems to work!
WebKit Review Bot
Comment 6 2012-09-15 19:07:32 PDT
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.
Filip Pizlo
Comment 7 2012-09-15 23:24:28 PDT
Created attachment 164309 [details] almost ready to land Still need to rebase some tests, and add some new tests.
Filip Pizlo
Comment 8 2012-09-16 00:33:01 PDT
(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.
Filip Pizlo
Comment 9 2012-09-16 00:35:21 PDT
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.
Filip Pizlo
Comment 10 2012-09-16 16:19:03 PDT
Created attachment 164326 [details] patch with new tests
Filip Pizlo
Comment 11 2012-09-16 18:02:50 PDT
Created attachment 164329 [details] rebased patch
WebKit Review Bot
Comment 12 2012-09-16 18:05:34 PDT
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.
Filip Pizlo
Comment 13 2012-09-16 18:10:41 PDT
Created attachment 164331 [details] the patch Fix style
WebKit Review Bot
Comment 14 2012-09-16 19:00:17 PDT
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
Filip Pizlo
Comment 15 2012-09-16 19:09:33 PDT
(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.
Gavin Barraclough
Comment 16 2012-09-17 00:33:37 PDT
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+.
Gavin Barraclough
Comment 17 2012-09-17 00:34:41 PDT
*** Bug 42801 has been marked as a duplicate of this bug. ***
Gavin Barraclough
Comment 18 2012-09-17 00:36:14 PDT
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.
Filip Pizlo
Comment 19 2012-09-17 12:10:36 PDT
(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.
Filip Pizlo
Comment 20 2012-09-17 13:09:11 PDT
(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+.
Filip Pizlo
Comment 21 2012-09-17 13:56:53 PDT
Landed in http://trac.webkit.org/changeset/128802
Filip Pizlo
Comment 22 2012-09-17 14:24:40 PDT
(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.
Csaba Osztrogonác
Comment 23 2012-09-18 05:08:40 PDT
(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?
Brendan Eich
Comment 24 2012-09-18 14:33:47 PDT
(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
Filip Pizlo
Comment 25 2012-09-18 15:08:35 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.