Bug 197631 - [JSC] Strict, Sloppy and Arrow functions should have different classInfo
Summary: [JSC] Strict, Sloppy and Arrow functions should have different classInfo
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-06 15:15 PDT by Robin Morisset
Modified: 2019-06-22 00:49 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.54 KB, patch)
2019-05-06 15:21 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (12.94 KB, patch)
2019-06-21 21:58 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (12.16 KB, patch)
2019-06-21 22:00 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 2019-05-06 15:15:37 PDT
'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.
Comment 1 Robin Morisset 2019-05-06 15:15:59 PDT
rdar://problem/50202206
Comment 2 Robin Morisset 2019-05-06 15:21:36 PDT
Created attachment 369178 [details]
Patch
Comment 3 Saam Barati 2019-05-06 15:28:44 PDT
(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?
Comment 4 Saam Barati 2019-05-06 15:30:57 PDT
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
Comment 5 Filip Pizlo 2019-06-04 10:49:41 PDT
(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?
Comment 6 Yusuke Suzuki 2019-06-21 20:03:23 PDT
Function constructor should pick a correct structure, I think this is the issue.
Comment 7 Yusuke Suzuki 2019-06-21 20:04:25 PDT
(In reply to Yusuke Suzuki from comment #6)
> Function constructor should pick a correct structure, I think this is the
> issue.

Ah, no. Investigating
Comment 8 Yusuke Suzuki 2019-06-21 20:08:31 PDT
(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.
Comment 9 Yusuke Suzuki 2019-06-21 21:58:40 PDT
Created attachment 372671 [details]
Patch
Comment 10 Yusuke Suzuki 2019-06-21 22:00:49 PDT
Created attachment 372672 [details]
Patch
Comment 11 Saam Barati 2019-06-21 22:38:48 PDT
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?
Comment 12 Yusuke Suzuki 2019-06-22 00:12:50 PDT
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.
Comment 13 Yusuke Suzuki 2019-06-22 00:48:12 PDT
Committed r246709: <https://trac.webkit.org/changeset/246709>