Bug 231202

Summary: [JSC] JSPropertyNameEnumerator should not have cached prototype chain since empty JSPropertyNameEnumerator is shared
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: c.jaentsch, eduard.comanici, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 231419    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch keith_miller: review+

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