Bug 231202 - [JSC] JSPropertyNameEnumerator should not have cached prototype chain since empty JSPropertyNameEnumerator is shared
Summary: [JSC] JSPropertyNameEnumerator should not have cached prototype chain since e...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
: 231419 231572 (view as bug list)
Depends on:
Blocks: 231419
  Show dependency tree / graph
 
Reported: 2021-10-04 17:58 PDT by Yusuke Suzuki
Modified: 2022-07-12 21:08 PDT (History)
9 users (show)

See Also:

Attachments
Patch (17.98 KB, patch)
2021-10-04 18:03 PDT, Yusuke Suzuki 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (30.67 KB, patch)
2021-10-04 18:42 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (30.67 KB, patch)
2021-10-04 18:48 PDT, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (30.95 KB, patch)
2021-10-04 18:52 PDT, Yusuke Suzuki 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (34.10 KB, patch)
2021-10-04 19:19 PDT, Yusuke Suzuki 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (34.11 KB, patch)
2021-10-04 19:30 PDT, Yusuke Suzuki 		keith_miller: review+
Details | Formatted Diff | Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2021-10-04 17:58:14 PDT 
[JSC] JSPropertyNameEnumerator should not have cached prototype chain since empty JSPropertyNameEnumerator is shared
Comment 1 Yusuke Suzuki 2021-10-04 18:03:48 PDT 
Created attachment 440132 [details]
Patch

WIP
Comment 2 Yusuke Suzuki 2021-10-04 18:03:51 PDT 
<rdar://problem/83815122>
Comment 3 Yusuke Suzuki 2021-10-04 18:42:34 PDT 
Created attachment 440140 [details]
Patch

WIP
Comment 4 Yusuke Suzuki 2021-10-04 18:48:28 PDT 
Created attachment 440141 [details]
Patch

WIP
Comment 5 Yusuke Suzuki 2021-10-04 18:52:17 PDT 
Created attachment 440142 [details]
Patch

WIP
Comment 6 Yusuke Suzuki 2021-10-04 19:19:52 PDT 
Created attachment 440145 [details]
Patch
Comment 7 Yusuke Suzuki 2021-10-04 19:30:32 PDT 
Created attachment 440147 [details]
Patch
Comment 8 Keith Miller 2021-10-05 10:31:36 PDT 
Comment on attachment 440147 [details]
Patch

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

r=me with some nits

> Source/JavaScriptCore/ChangeLog:11
> +        invariant was broken since we now have shared empty JSPropertyNameEnumerator, which can

Nit: shared empty => shared empty sentinel. Might be a bit clearer.

> Source/JavaScriptCore/ChangeLog:27
> +        While reviewing the code, we also found that watchpoint-based validation didn't care PolyProto.

Nit: care => care about?

> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:107
> +    // JSPropertyNameEnumerator is immutable data structure, which allows VM to cache empty one.

Typo: cache empty one => cache the empty one.

> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:131
> +            if (!(enumeratorAndFlag & StructureRareData::cachedPropertyNameEnumeratorIsValidatedViaTraversingFlag))

Nit: Maybe use OptionSet I personally prefer that but we use this idiom everywhere so idc too much.
Comment 9 Yusuke Suzuki 2021-10-05 10:48:55 PDT 
Comment on attachment 440147 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:11
>> +        invariant was broken since we now have shared empty JSPropertyNameEnumerator, which can
> 
> Nit: shared empty => shared empty sentinel. Might be a bit clearer.

Fixed.

>> Source/JavaScriptCore/ChangeLog:27
>> +        While reviewing the code, we also found that watchpoint-based validation didn't care PolyProto.
> 
> Nit: care => care about?

Fixed.

>> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:107
>> +    // JSPropertyNameEnumerator is immutable data structure, which allows VM to cache empty one.
> 
> Typo: cache empty one => cache the empty one.

Fixed.

>> Source/JavaScriptCore/runtime/JSPropertyNameEnumerator.h:131
>> +            if (!(enumeratorAndFlag & StructureRareData::cachedPropertyNameEnumeratorIsValidatedViaTraversingFlag))
> 
> Nit: Maybe use OptionSet I personally prefer that but we use this idiom everywhere so idc too much.

Because this is uintptr_t including enumerator pointer itself, we cannot use OptionSet.
Comment 10 Yusuke Suzuki 2021-10-05 10:50:00 PDT 
Committed r283556 (242521@main): <https://commits.webkit.org/242521@main>
Comment 11 Justin Michaud 2021-10-11 16:27:23 PDT 
*** Bug 231419 has been marked as a duplicate of this bug. ***
Comment 12 Yusuke Suzuki 2022-07-12 21:08:40 PDT 
*** Bug 231572 has been marked as a duplicate of this bug. ***