Disambiguate the OverridesGetPropertyNames structure flag.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

<rdar://problem/63823557>

[Mark Lam]

Created attachment 401357 [details]
proposed patch.

[Keith Miller]

Comment on attachment 401357 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=401357&action=review

> JSTests/stress/stack-overflow-below-JSObject-getPropertyNames.js:16
> +            caughtStackOverflowOnce = true;

Why do we want to catch it once?

> Source/JavaScriptCore/runtime/JSObject.cpp:2345
>          if (prototype->structure(vm)->typeInfo().overridesGetPropertyNames()) {

Doesn't this check mean we are not recursing?

[Saam Barati]

Comment on attachment 401357 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=401357&action=review

> Source/JavaScriptCore/runtime/JSObject.cpp:2337
> +    // This is needed because we're recursing in the getPropertyNames() calls below.

Is the idea that an overridden getProperty names calls back into here?

I don’t see how your test does or proves this. Why not make a super duper long prototype chain to prove this is happening?

[Mark Lam]

(In reply to Saam Barati from comment #3)
> > Source/JavaScriptCore/runtime/JSObject.cpp:2337
> > +    // This is needed because we're recursing in the getPropertyNames() calls below.
> 
> Is the idea that an overridden getProperty names calls back into here?
> 
> I don’t see how your test does or proves this. Why not make a super duper
> long prototype chain to prove this is happening?

Both you and Keith have pointed out that this is not expected.  The tests does prove that the issue manifests on trunk.  Looks like the root issue lies elsewhere.  I will investigate further.

[Mark Lam]

Comment on attachment 401357 [details]
proposed patch.

Taking out of review while I dig for more details.

[Mark Lam]

(In reply to Keith Miller from comment #2)
> Comment on attachment 401357 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401357&action=review
> 
> > JSTests/stress/stack-overflow-below-JSObject-getPropertyNames.js:16
> > +            caughtStackOverflowOnce = true;
> 
> Why do we want to catch it once?

This was added so that the test can end quickly vs taking forever.  But this check turns out to be unnecessary.  We just need to rethrow in the catch to avoid running the for-in loop after the catch block.  The issue does not repro if I remove the try-catch or the for-in loop.

But none of these may matter once the real root cause.  Still digging.

[Mark Lam]

Created attachment 401430 [details]
proposed patch.

[Saam Barati]

Comment on attachment 401430 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=401430&action=review

> Source/JavaScriptCore/ChangeLog:17
> +        for definition (2).  OverridesGetPropertyNames now only means definition (1).

so (1) implies (2), right?

Do we assert that?

but doesn't that also mean that (1) implies slow for...in?

I wouldn't expect getOwnNonIndexPropertyNames means you can't cache property enumerator but that's what your change does b/c changes in canCacheOwnKeys

> Source/JavaScriptCore/ChangeLog:18
> +

so you say some code conflated these two things. What was the actual bug this lead to?

[Mark Lam]

Comment on attachment 401430 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=401430&action=review

>> Source/JavaScriptCore/ChangeLog:17
>> +        for definition (2).  OverridesGetPropertyNames now only means definition (1).
> 
> so (1) implies (2), right?
> 
> Do we assert that?
> 
> but doesn't that also mean that (1) implies slow for...in?
> 
> I wouldn't expect getOwnNonIndexPropertyNames means you can't cache property enumerator but that's what your change does b/c changes in canCacheOwnKeys

Yes, (1) implies (2).  The asserts in Structure::validateFlags() already asserts that if getPropertyNames is overridden, then the OverridesAnyFormOfGetPropertyNames flag should be set.  It also asserts that the OverridesGetPropertyNames should be set.  So, this effectively asserts what you're asking.

Regarding this slowing does for...in, how so?  We were already doing this to begin with: in the old code, Structure::canCacheOwnKeys() used to check typeInfo().overridesGetPropertyNames(), and OverridesGetPropertyName is set when any of the getPropertyName, getOwnPropertyNames, getOwnNonIndexPropertyNames are overridden.  There were some cases where it was even set when getPropertyName was not overridden.  For example, JSArray did not override getPropertyNames but sets OverridesGetPropertyNames.  This means canCacheOwnKeys() always returned false for JSArray ... no different than with my patch.  So, I fail to see how things have changed to be more pessimistic after my patch.  Can you give me a specific class and scenario to demonstrate your point?

The only cases where I added OverridesAnyFormOfGetPropertyNames where OverridesGetPropertyNames wasn't present before are 1. JSAPIValueWrapper, and 2. JSDataView.  Are you saying JSDataView is the case which will be the slow down here?

>> Source/JavaScriptCore/ChangeLog:18
>> +
> 
> so you say some code conflated these two things. What was the actual bug this lead to?

Here are the rough details:

1. The repro test case was invoking JSObject::getPropertyNames on a JSArray. 

2. In the while loop at the bottom of JSObject::getPropertynames, we check `if (prototype->structure(vm)->typeInfo().overridesGetPropertyNames()) {`.  

3. The test overrides proto as follows: `arg0.__proto__ = arr1` where both arg0 and arr1 are JArrays.

4. As commented above, JSArray sets OverridesGetPropertyNames but does not override getPropertyNames().
    It actually meant to set OverridesAnyFormOfGetPropertyNames (after I disambiguated it) because JSArray overrides getOwnNonIndexPropertyNames().

5. When we get to the check at (2), we ask if the prototype overridesGetPropertyNames().
    Since JSArray sets OverridesGetPropertyNames, the answer is yes / true.
    JSObject::getPropertynames() then proceeds to invoke `prototype->methodTable(vm)->getPropertyNames(prototype, globalObject, propertyNames, mode);`
    But because JSArray does actually overrides getPropertyNames(), we're actually invoking JSObject::getPropertyNames() here.

Viola!  Infinite loop.

That's the bug.  The fix is that JSArray should not be setting OverridesGetPropertyNames.  It should be setting OverridesAnyFormOfGetPropertyNames.

[Mark Lam]

(In reply to Mark Lam from comment #9)
...
> 5. When we get to the check at (2), we ask if the prototype
> overridesGetPropertyNames().
>     Since JSArray sets OverridesGetPropertyNames, the answer is yes / true.
>     JSObject::getPropertynames() then proceeds to invoke
> `prototype->methodTable(vm)->getPropertyNames(prototype, globalObject,
> propertyNames, mode);`
>     But because JSArray does actually overrides getPropertyNames(), we're
> actually invoking JSObject::getPropertyNames() here.

typo: I meant to say "because JSArray does NOT actually override getPropertyNames()".

[Saam Barati]

(In reply to Mark Lam from comment #9)
> Comment on attachment 401430 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401430&action=review
> 
> >> Source/JavaScriptCore/ChangeLog:17
> >> +        for definition (2).  OverridesGetPropertyNames now only means definition (1).
> > 
> > so (1) implies (2), right?
> > 
> > Do we assert that?
> > 
> > but doesn't that also mean that (1) implies slow for...in?
> > 
> > I wouldn't expect getOwnNonIndexPropertyNames means you can't cache property enumerator but that's what your change does b/c changes in canCacheOwnKeys
> 
> Yes, (1) implies (2).  The asserts in Structure::validateFlags() already
> asserts that if getPropertyNames is overridden, then the
> OverridesAnyFormOfGetPropertyNames flag should be set.  It also asserts that
> the OverridesGetPropertyNames should be set.  So, this effectively asserts
> what you're asking.
> 
> Regarding this slowing does for...in, how so?  We were already doing this to
> begin with: in the old code, Structure::canCacheOwnKeys() used to check
> typeInfo().overridesGetPropertyNames(), and OverridesGetPropertyName is set
> when any of the getPropertyName, getOwnPropertyNames,
> getOwnNonIndexPropertyNames are overridden.  There were some cases where it
> was even set when getPropertyName was not overridden.  For example, JSArray
> did not override getPropertyNames but sets OverridesGetPropertyNames.  This
> means canCacheOwnKeys() always returned false for JSArray ... no different
> than with my patch.  So, I fail to see how things have changed to be more
> pessimistic after my patch.  Can you give me a specific class and scenario
> to demonstrate your point?
> 
> The only cases where I added OverridesAnyFormOfGetPropertyNames where
> OverridesGetPropertyNames wasn't present before are 1. JSAPIValueWrapper,
> and 2. JSDataView.  Are you saying JSDataView is the case which will be the
> slow down here?
> 
> >> Source/JavaScriptCore/ChangeLog:18
> >> +
> > 
> > so you say some code conflated these two things. What was the actual bug this lead to?
> 
> Here are the rough details:
> 
> 1. The repro test case was invoking JSObject::getPropertyNames on a JSArray. 
> 
> 2. In the while loop at the bottom of JSObject::getPropertynames, we check
> `if (prototype->structure(vm)->typeInfo().overridesGetPropertyNames()) {`.  
> 
> 3. The test overrides proto as follows: `arg0.__proto__ = arr1` where both
> arg0 and arr1 are JArrays.
> 
> 4. As commented above, JSArray sets OverridesGetPropertyNames but does not
> override getPropertyNames().
>     It actually meant to set OverridesAnyFormOfGetPropertyNames (after I
> disambiguated it) because JSArray overrides getOwnNonIndexPropertyNames().
> 
> 5. When we get to the check at (2), we ask if the prototype
> overridesGetPropertyNames().
>     Since JSArray sets OverridesGetPropertyNames, the answer is yes / true.
>     JSObject::getPropertynames() then proceeds to invoke
> `prototype->methodTable(vm)->getPropertyNames(prototype, globalObject,
> propertyNames, mode);`
>     But because JSArray does actually overrides getPropertyNames(), we're
> actually invoking JSObject::getPropertyNames() here.
> 
> Viola!  Infinite loop.
> 
> That's the bug.  The fix is that JSArray should not be setting
> OverridesGetPropertyNames.  It should be setting
> OverridesAnyFormOfGetPropertyNames.

Makes sense. Please add this into your changelog since it's the most interesting part, and the motivation for the change.

[Saam Barati]

Comment on attachment 401430 [details]
proposed patch.

r=me

[Mark Lam]

Thanks for the review.  Landed in r262827: <http://trac.webkit.org/r262827>.