Bug 225894 – Window should behave like a legacy platform object without indexed setter

• Component: DOM
• Status: RESOLVED FIXED

[Alexey Shvayka]

Window should behave like a legacy platform object without indexed setter

[Alexey Shvayka]

Created attachment 428887 [details]
Patch

[Alexey Shvayka]

Created attachment 429066 [details]
Patch

Adjust tests and improve coverage, remove slot.disableCaching(), and add UINT_MAX checks.

[EWS Watchlist]

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

[Radar WebKit Bug Importer]

<rdar://problem/78424257>

[Alexey Shvayka]

Created attachment 430091 [details]
Patch

Guard new behavior with isFullWebBrowser(), add a console warning, and simplify getOwnPropertySlotByIndex().

[Alexey Shvayka]

Created attachment 430094 [details]
Patch

Remove MacApplication::isMiniBrowser().

[Chris Dumez]

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.

[Alexey Shvayka]

(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?

[Chris Dumez]

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

[Alexey Shvayka]

Created attachment 430397 [details]
Patch

Use allowsLegacyExpandoIndexedProperties() and release scope before calling Base::putByIndex().

[Alexey Shvayka]

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

[Darin Adler]

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.

[Alexey Shvayka]

Created attachment 430764 [details]
Patch for landing

Tweak allowsLegacyExpandoIndexedProperties() per Darin's suggestion and add a comment to the Cocoa half.

[Alexey Shvayka]

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

[EWS]

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