Window should behave like a legacy platform object without indexed setter
Created attachment 428887 [details] Patch
Created attachment 429066 [details] Patch Adjust tests and improve coverage, remove slot.disableCaching(), and add UINT_MAX checks.
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
<rdar://problem/78424257>
Created attachment 430091 [details] Patch Guard new behavior with isFullWebBrowser(), add a console warning, and simplify getOwnPropertySlotByIndex().
Created attachment 430094 [details] Patch Remove MacApplication::isMiniBrowser().
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.
(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?
(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.
Created attachment 430397 [details] Patch Use allowsLegacyExpandoIndexedProperties() and release scope before calling Base::putByIndex().
(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 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.
Created attachment 430764 [details] Patch for landing Tweak allowsLegacyExpandoIndexedProperties() per Darin's suggestion and add a comment to the Cocoa half.
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".
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].