Bug 225894 - Window should behave like a legacy platform object without indexed setter
Summary: Window should behave like a legacy platform object without indexed setter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-17 16:16 PDT by Alexey Shvayka
Modified: 2021-06-07 18:10 PDT (History)
16 users (show)

See Also:


Attachments
Patch (14.92 KB, patch)
2021-05-17 16:21 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (27.05 KB, patch)
2021-05-19 10:37 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (35.24 KB, patch)
2021-05-29 07:17 PDT, Alexey Shvayka
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (35.21 KB, patch)
2021-05-29 07:44 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (31.89 KB, patch)
2021-06-02 13:40 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch for landing (32.17 KB, patch)
2021-06-07 11:29 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Shvayka 2021-05-17 16:16:46 PDT
Window should behave like a legacy platform object without indexed setter
Comment 1 Alexey Shvayka 2021-05-17 16:21:53 PDT
Created attachment 428887 [details]
Patch
Comment 2 Alexey Shvayka 2021-05-19 10:37:45 PDT
Created attachment 429066 [details]
Patch

Adjust tests and improve coverage, remove slot.disableCaching(), and add UINT_MAX checks.
Comment 3 EWS Watchlist 2021-05-19 10:38:25 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 4 Radar WebKit Bug Importer 2021-05-24 16:17:20 PDT
<rdar://problem/78424257>
Comment 5 Alexey Shvayka 2021-05-29 07:17:52 PDT
Created attachment 430091 [details]
Patch

Guard new behavior with isFullWebBrowser(), add a console warning, and simplify getOwnPropertySlotByIndex().
Comment 6 Alexey Shvayka 2021-05-29 07:44:46 PDT
Created attachment 430094 [details]
Patch

Remove MacApplication::isMiniBrowser().
Comment 7 Chris Dumez 2021-05-29 10:01:44 PDT
Comment on attachment 430094 [details]
Patch

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

> Source/WTF/wtf/cocoa/RuntimeApplicationChecksCocoa.h:39
> +WTF_EXPORT_PRIVATE bool isRunningTest(const String& bundleID);

We used to have a way to detect we were running tests and we dropped it because we didn’t think it was a good idea to have test specific behavior.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:100
> +inline static bool isFullWebBrowser()

Why? Why would the behavior be different in web browsers than apps?

If we are worried about compatibility, a linked on after check would be more appropriate.
Comment 8 Alexey Shvayka 2021-05-29 10:51:58 PDT
(In reply to Chris Dumez from comment #7)
> > Source/WTF/wtf/cocoa/RuntimeApplicationChecksCocoa.h:39
> > +WTF_EXPORT_PRIVATE bool isRunningTest(const String& bundleID);
> 
> We used to have a way to detect we were running tests and we dropped it
> because we didn’t think it was a good idea to have test specific behavior.

Yeah, this isn't perfect; is there a way to import isFullWebBrowser() of Shared/Cocoa/DefaultWebBrowserChecks.mm rather than exposing isRunningTest() on WTF and calling it directly?

> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:100
> > +inline static bool isFullWebBrowser()
> 
> Why? Why would the behavior be different in web browsers than apps?
> 
> If we are worried about compatibility

I think we are: apps may either add expando indices to WindowProxy directly, or more likely read / write indexed properties of sloppy function's |this| value, which accidentally happens to be a WindowProxy. We had the current behavior for a long time + "length" is [Replaceable].

Maybe I'm being too cautious and there is enough metrics to ship this change without isFullWebBrowser()?

> a linked on after check would be more appropriate.

Could you please expand on this?
Comment 9 Chris Dumez 2021-05-29 12:03:56 PDT
(In reply to Alexey Shvayka from comment #8)
> (In reply to Chris Dumez from comment #7)
> > > Source/WTF/wtf/cocoa/RuntimeApplicationChecksCocoa.h:39
> > > +WTF_EXPORT_PRIVATE bool isRunningTest(const String& bundleID);
> > 
> > We used to have a way to detect we were running tests and we dropped it
> > because we didn’t think it was a good idea to have test specific behavior.
> 
> Yeah, this isn't perfect; is there a way to import isFullWebBrowser() of
> Shared/Cocoa/DefaultWebBrowserChecks.mm rather than exposing isRunningTest()
> on WTF and calling it directly?
> 
> > > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:100
> > > +inline static bool isFullWebBrowser()
> > 
> > Why? Why would the behavior be different in web browsers than apps?
> > 
> > If we are worried about compatibility
> 
> I think we are: apps may either add expando indices to WindowProxy directly,
> or more likely read / write indexed properties of sloppy function's |this|
> value, which accidentally happens to be a WindowProxy. We had the current
> behavior for a long time + "length" is [Replaceable].
> 
> Maybe I'm being too cautious and there is enough metrics to ship this change
> without isFullWebBrowser()?
> 
> > a linked on after check would be more appropriate.
> 
> Could you please expand on this?

I am on a phone now but please Greg for isLinkedOnAfter(). Basically we use this to gate new behaviors on new SDKs. Mitigates compatibility issues with apps.
Comment 10 Alexey Shvayka 2021-06-02 13:40:32 PDT
Created attachment 430397 [details]
Patch

Use allowsLegacyExpandoIndexedProperties() and release scope before calling Base::putByIndex().
Comment 11 Alexey Shvayka 2021-06-02 13:41:04 PDT
(In reply to Chris Dumez from comment #9)
> I am on a phone now but please Greg for isLinkedOnAfter(). Basically we use
> this to gate new behaviors on new SDKs. Mitigates compatibility issues with apps.

Works like a charm, with built-in support for test runners. Thanks Chris!
A thought: with similar approach to JSC API we could slowly fix a few quirks, cleaning up JSC built-ins and possibly making userland API code more sensible.
Comment 12 Darin Adler 2021-06-04 14:38:40 PDT
Comment on attachment 430397 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:105
> +#if PLATFORM(COCOA)
> +    static bool linkedWithNewSDK = linkedOnOrAfter(SDKVersion::FirstWithoutExpandoIndexedPropertiesOnWindow);
> +#else
> +    static bool linkedWithNewSDK = true;
> +#endif
> +    return !linkedWithNewSDK;

This function needs a why comment in the COCOA half. What is it about older clients that makes us want to keep allowing these? I think it’s pretty easy to explain: It’s hard to prove that older iOS and macOS apps don’t accidentally depend on this behavior, so we keep it for them. I think that on further reflection we might later decide to narrow the legacy quirk to be iOS-only: may not be needed for macOS, watchOS, or tvOS.

Aside from that, I’d write it more like this for better clarity:

#if PLATFORM(COCOA)
    static bool requiresQuirk = !linkedOnOrAfter(SDKVersion::FirstWithoutExpandoIndexedPropertiesOnWindow);
    return requiresQuirk;
#else
    return false;
#endif

Your variable name might vary. I like adding value through the naming, going a little beyond variable name just repeating what the function call says.
Comment 13 Alexey Shvayka 2021-06-07 11:29:39 PDT
Created attachment 430764 [details]
Patch for landing

Tweak allowsLegacyExpandoIndexedProperties() per Darin's suggestion and add a comment to the Cocoa half.
Comment 14 Alexey Shvayka 2021-06-07 17:35:42 PDT
Comment on attachment 430764 [details]
Patch for landing

(In reply to Darin Adler from comment #12)

Thank you for review!

> Your variable name might vary. I like adding value through the naming, going
> a little beyond variable name just repeating what the function call says.

"requiresQuirk" seems to be a nice name that doesn't duplicate "allowsLegacyExpandoIndexedProperties" nor "linkedOnOrAfter".
Comment 15 EWS 2021-06-07 18:10:31 PDT
Committed r278585 (238575@main): <https://commits.webkit.org/238575@main>

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