Bug 220809

Summary: REGRESSION (r270874): Some React Native apps are reported broken on iOS
Product: WebKit Reporter: Alexey Shvayka <ashvayka>
Component: JavaScriptCoreAssignee: Alexey Shvayka <ashvayka>
Status: RESOLVED FIXED    
Severity: Major CC: ews-watchlist, keith_miller, mark.lam, msaboff, saam, smoley, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=38970
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Alexey Shvayka 2021-01-21 10:29:52 PST 
REGRESSION (r270874): Some React Native apps are reported broken on iOS
Comment 1 Alexey Shvayka 2021-01-21 10:33:05 PST 
Created attachment 418061 [details]
Patch
Comment 2 Alexey Shvayka 2021-01-21 10:37:45 PST 
Comment on attachment 418061 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSObject.cpp:2001
> +    bool reportAsEnumerable = !(slot.attributes() & PropertyAttribute::DontEnum) || structure(vm)->typeInfo().getOwnPropertySlotIsWrongAboutDontEnum();

The flag should probably be queried from slotBase() rather than |this|.
Comment 3 Alexey Shvayka 2021-01-21 11:10:50 PST 
Created attachment 418064 [details]
Patch

Query GetOwnPropertySlotIsWrongAboutDontEnum from the slotBase().
Comment 4 Radar WebKit Bug Importer 2021-01-21 16:51:17 PST 
<rdar://problem/73475533>
Comment 5 Saam Barati 2021-01-22 11:05:22 PST 
I'm going to test this soon and can review too
Comment 6 Saam Barati 2021-01-25 16:09:36 PST 
Comment on attachment 418064 [details]
Patch

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

Confirmed this fixes the React native crashes we were seeing

> Source/JavaScriptCore/runtime/JSTypeInfo.h:62
> +static constexpr unsigned GetOwnPropertySlotIsWrongAboutDontEnum = 1 << 20;

nit, I think I'd call this:
GetOwnPropertySlotMayBeWrongAboutDontEnum

since we don't know definitively, but the runtime must be conservative.

> Source/WebCore/bridge/runtime_array.h:38
> +    static constexpr unsigned StructureFlags = Base::StructureFlags | OverridesGetOwnPropertySlot | OverridesGetOwnPropertyNames | InterceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero | GetOwnPropertySlotIsWrongAboutDontEnum;

is it worth reverting these back to how they used to be and provide this flag? Or should we make them truthful? I don't think we have evidence that this part of the patch broke stuff
Comment 7 Alexey Shvayka 2021-01-25 17:27:53 PST 
Created attachment 418359 [details]
Patch for landing
Comment 8 EWS 2021-01-25 18:13:16 PST 
Committed r271873: <https://trac.webkit.org/changeset/271873>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418359 [details].