WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
96596
If a prototype has indexed setters and its instances have indexed storage, then all put_by_val's should have a bad time
https://bugs.webkit.org/show_bug.cgi?id=96596
Summary
If a prototype has indexed setters and its instances have indexed storage, th...
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
Details
Formatted Diff
Diff
moar
(54.69 KB, patch)
2012-09-14 20:01 PDT
,
Filip Pizlo
no flags
Details
Formatted Diff
Diff
the patch
(78.56 KB, patch)
2012-09-15 19:04 PDT
,
Filip Pizlo
oliver
: review+
Details
Formatted Diff
Diff
almost ready to land
(82.48 KB, patch)
2012-09-15 23:24 PDT
,
Filip Pizlo
no flags
Details
Formatted Diff
Diff
the patch
(87.66 KB, patch)
2012-09-16 00:35 PDT
,
Filip Pizlo
no flags
Details
Formatted Diff
Diff
patch with new tests
(174.09 KB, patch)
2012-09-16 16:19 PDT
,
Filip Pizlo
no flags
Details
Formatted Diff
Diff
rebased patch
(173.39 KB, patch)
2012-09-16 18:02 PDT
,
Filip Pizlo
no flags
Details
Formatted Diff
Diff
the patch
(173.39 KB, patch)
2012-09-16 18:10 PDT
,
Filip Pizlo
barraclough
: review+
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
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.
Top of Page
Format For Printing
XML
Clone This Bug