[JSC] Strict, Sloppy and Arrow functions should have different classInfo

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Robin Morisset]

'arguments' behave differently in sloppy and strict mode. So if we cache HasOwnProperty('arguments') in strict mode, then try to call it again in sloppy mode (or vice-versa) the result will be wrong.
The simplest solution is just not to ever cache HasOwnProperty('arguments').
Adding a bit for sloppy/strict in the keys would also work, but it would complicate things for the common case, and I don't expect HasOwnProperty('arguments') to be super common.

[Robin Morisset]

rdar://problem/50202206

[Robin Morisset]

Created attachment 369178 [details]
Patch

[Saam Barati]

(In reply to Robin Morisset from comment #0)
> 'arguments' behave differently in sloppy and strict mode. So if we cache
> HasOwnProperty('arguments') in strict mode, then try to call it again in
> sloppy mode (or vice-versa) the result will be wrong.
> The simplest solution is just not to ever cache HasOwnProperty('arguments').
> Adding a bit for sloppy/strict in the keys would also work, but it would
> complicate things for the common case, and I don't expect
> HasOwnProperty('arguments') to be super common.

Why don't these have different structures?

[Saam Barati]

Comment on attachment 369178 [details]
Patch

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

> JSTests/stress/has-own-property-arguments.js:3
> +class A extends Function {}
> +new A("'use strict';").hasOwnProperty('arguments');
> +new A().hasOwnProperty('arguments');

I'm super confused what this is doing. This is just calling the constructor with a string as the first argument.

Also, classes are always in strict mode. Can you explain what's going on? Can this also test what it expects the results to be?

> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:95
> +        // We don't cache accesses to arguments, because it depends on whether we are in strict or sloppy mode.
> +        if (propName == "arguments")
> +            return;

This seems wrong. We should be reflecting this in Structure. It's unlikely the bug is just here and not also in Get

[Filip Pizlo]

(In reply to Robin Morisset from comment #0)
> 'arguments' behave differently in sloppy and strict mode. So if we cache
> HasOwnProperty('arguments') in strict mode, then try to call it again in
> sloppy mode (or vice-versa) the result will be wrong.
> The simplest solution is just not to ever cache HasOwnProperty('arguments').
> Adding a bit for sloppy/strict in the keys would also work, but it would
> complicate things for the common case, and I don't expect
> HasOwnProperty('arguments') to be super common.

Why wouldn't the sloppy-strict change be caught by the cache?

A function can't change its mind about whether it is strict or sloppy.  Is the issue that function objects for strict functions have the same structure as function objects for sloppy functions?

[Yusuke Suzuki]

Function constructor should pick a correct structure, I think this is the issue.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #6)
> Function constructor should pick a correct structure, I think this is the
> issue.

Ah, no. Investigating

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #7)
> (In reply to Yusuke Suzuki from comment #6)
> > Function constructor should pick a correct structure, I think this is the
> > issue.
> 
> Ah, no. Investigating

The issue is that InternalFunction::createSubclassStructureSlow caches the structure, and when using the previous structure, we are assuming that "If classInfo is the same, we should reuse the cached one". This is wrong for this case.

[Yusuke Suzuki]

Created attachment 372671 [details]
Patch

[Yusuke Suzuki]

Created attachment 372672 [details]
Patch

[Saam Barati]

Comment on attachment 372672 [details]
Patch

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

r=me

> JSTests/stress/has-own-property-arguments.js:8
> +shouldBe(new A().hasOwnProperty('arguments'), true);

Can you also add tests that would break get by id inline caching?

[Yusuke Suzuki]

Comment on attachment 372672 [details]
Patch

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

Thanks!

>> JSTests/stress/has-own-property-arguments.js:8
>> +shouldBe(new A().hasOwnProperty('arguments'), true);
> 
> Can you also add tests that would break get by id inline caching?

Sure! Added.

[Yusuke Suzuki]

Committed r246709: <https://trac.webkit.org/changeset/246709>