Bug 96596 – If a prototype has indexed setters and its instances have indexed storage, then all put_by_val's should have a bad time

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

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.

[Filip Pizlo]

*** Bug 96605 has been marked as a duplicate of this bug. ***

[Ádám Kallai]

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]

Created attachment 164025 [details]
work in progress

[Filip Pizlo]

Created attachment 164268 [details]
moar

Almost there.

[Filip Pizlo]

Created attachment 164301 [details]
the patch

It seems to work!

[WebKit Review Bot]

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]

Created attachment 164309 [details]
almost ready to land

Still need to rebase some tests, and add some new tests.

[Filip Pizlo]

(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]

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]

Created attachment 164326 [details]
patch with new tests

[Filip Pizlo]

Created attachment 164329 [details]
rebased patch

[WebKit Review Bot]

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]

Created attachment 164331 [details]
the patch

Fix style

[WebKit Review Bot]

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]

(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 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]

*** Bug 42801 has been marked as a duplicate of this bug. ***

[Gavin Barraclough]

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]

(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]

(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]

Landed in http://trac.webkit.org/changeset/128802

[Filip Pizlo]

(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]

(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]

(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]

(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.