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: | JavaScriptCore | Assignee: | 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
Filip Pizlo
2012-09-12 21:05:34 PDT
*** 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. |