<rdar://84473037>
Created attachment 443357 [details] WIP Patch
Created attachment 443392 [details] WIP Patch (with API test)
Created attachment 443401 [details] WIP Patch
Created attachment 443406 [details] Patch
Comment on attachment 443406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443406&action=review > Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.h:95 > + JSC::Options::Options::useJIT() = false; I think for miniMode we also want: JSC::Options::Options::useGenerationalGC() = false; JSC::Options::Options::useConcurrentGC() = false;
The content of attachment 443357 [details] has been deleted
The content of attachment 443392 [details] has been deleted
The content of attachment 443401 [details] has been deleted
Created attachment 443412 [details] Patch
Comment on attachment 443412 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443412&action=review > Source/WebKit/UIProcess/WebPageProxy.cpp:3439 > + auto shouldEnableMiniMode = (policies ? policies->miniModeEnabled() : isMiniModeEnabledGlobally()) ? WebProcessProxy::IsMiniModeEnabled::Yes : WebProcessProxy::IsMiniModeEnabled::No; If the global switch is not "force enable mini mode" but instead a /fallback/, why not just use the normal bit on defaultWebpagePreferences in WKWebViewConfiguration?
Created attachment 443418 [details] Patch
(In reply to Tim Horton from comment #11) > Comment on attachment 443412 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=443412&action=review > > > Source/WebKit/UIProcess/WebPageProxy.cpp:3439 > > + auto shouldEnableMiniMode = (policies ? policies->miniModeEnabled() : isMiniModeEnabledGlobally()) ? WebProcessProxy::IsMiniModeEnabled::Yes : WebProcessProxy::IsMiniModeEnabled::No; > > If the global switch is not "force enable mini mode" but instead a > /fallback/, why not just use the normal bit on defaultWebpagePreferences in > WKWebViewConfiguration? Captive portal mode can be enabled / disabled system wide. Then it can be overridden by browsers per-site via WKWebpagePreferences. The issue with WKWebViewConfiguration.defaultWebpagePreferences.captivePortalModeEnabled is that it only applies to WebViews using this configuration, it is not app-global. Also, when the client is setting captivePortalModeEnabled on a WKWebpagePreferences object, how do I know if I am supposed to set the global NSUserDefault or not? How do I differentiate the case where the client wants to set it globally vs the client wants to set it by default for all views using this WKWebViewConfiguration (or only for this navigation)? This is why I introduced a WKWebpagePreferences._captivePortalModeEnabled class property (readwrite), so that the client can set or query the global state of the feature.
(In reply to Chris Dumez from comment #13) > (In reply to Tim Horton from comment #11) > > Comment on attachment 443412 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=443412&action=review > > > > > Source/WebKit/UIProcess/WebPageProxy.cpp:3439 > > > + auto shouldEnableMiniMode = (policies ? policies->miniModeEnabled() : isMiniModeEnabledGlobally()) ? WebProcessProxy::IsMiniModeEnabled::Yes : WebProcessProxy::IsMiniModeEnabled::No; > > > > If the global switch is not "force enable mini mode" but instead a > > /fallback/, why not just use the normal bit on defaultWebpagePreferences in > > WKWebViewConfiguration? > > Captive portal mode can be enabled / disabled system wide. Then it can be > overridden by browsers per-site via WKWebpagePreferences. > > The issue with > WKWebViewConfiguration.defaultWebpagePreferences.captivePortalModeEnabled is > that it only applies to WebViews using this configuration, it is not > app-global. > > Also, when the client is setting captivePortalModeEnabled on a > WKWebpagePreferences object, how do I know if I am supposed to set the > global NSUserDefault or not? How do I differentiate the case where the > client wants to set it globally vs the client wants to set it by default for > all views using this WKWebViewConfiguration (or only for this navigation)? > > This is why I introduced a WKWebpagePreferences._captivePortalModeEnabled > class property (readwrite), so that the client can set or query the global > state of the feature. After discussing with Tim and Wenson offline, I think we have a simpler proposal. Reworking the patch now..
Created attachment 443431 [details] Patch
Comment on attachment 443431 [details] Patch Not sure about all of the "is" prefixes (also you have both "shouldUse" and "shouldEnable" and I think they have the same meaning?), but this seems better to me.
Created attachment 443435 [details] Patch
Comment on attachment 443435 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443435&action=review > Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.h:100 > JSC::ExecutableAllocator::setJITEnabled(false); This does some stronger things that we may want to do in captive portal mode. We should also audit the users of disable-jit and see if they can migrate to this as a replacement. > Source/WebKit/UIProcess/API/APIWebsitePolicies.h:163 > + std::optional<bool> m_captivePortalModeEnabled; This can just be a bool that defaults to WebKit::captivePortalModeEnabledBySystem(), right? > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:7630 > +// On iOS, the captive portal mode API requires a browser entitlement. Why don't we give TestWebKitAPI this entitlement to test what happens when you have it?
Comment on attachment 443435 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443435&action=review >> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.h:100 >> JSC::ExecutableAllocator::setJITEnabled(false); > > This does some stronger things that we may want to do in captive portal mode. > We should also audit the users of disable-jit and see if they can migrate to this as a replacement. > This does some stronger things that we may want to do in captive portal mode. I don't understand this comment. Also, why are you commenting on the pre-existing disable-jit flag. I am not relying on this pre-existing disable-jit flag because we turn off more JSC features in captive portal mode. Also, in a follow-up patch, it will also impact WebCore settings, not just JSC options. >> Source/WebKit/UIProcess/API/APIWebsitePolicies.h:163 >> + std::optional<bool> m_captivePortalModeEnabled; > > This can just be a bool that defaults to WebKit::captivePortalModeEnabledBySystem(), right? The reason I used an std::optional<bool> is that the system setting may change during the lifetime of the APIWebsitePolicies object. If I use a bool and set the value at construction time, then we won't update it if the system setting changes. This is especially problematic for WKWebViewConfiguration.defaultWebpagePreferences which has a long lifetime. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:7630 >> +// On iOS, the captive portal mode API requires a browser entitlement. > > Why don't we give TestWebKitAPI this entitlement to test what happens when you have it? It would be nice, yes. I don't know how to do that yet but I will look into it.
Comment on attachment 443435 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443435&action=review >>> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.h:100 >>> JSC::ExecutableAllocator::setJITEnabled(false); >> >> This does some stronger things that we may want to do in captive portal mode. >> We should also audit the users of disable-jit and see if they can migrate to this as a replacement. > > I may be wrong, but I think ExecutableAllocator::setJITEnabled makes it so the process is unable to allocate executable memory even if compromised, whereas JSC::Options::useJIT() = false; just makes it so it doesn't try to under normal circumstances.
(In reply to Alex Christensen from comment #20) > Comment on attachment 443435 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=443435&action=review > > >>> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.h:100 > >>> JSC::ExecutableAllocator::setJITEnabled(false); > >> > >> This does some stronger things that we may want to do in captive portal mode. > >> We should also audit the users of disable-jit and see if they can migrate to this as a replacement. > > > > > > I may be wrong, but I think ExecutableAllocator::setJITEnabled makes it so > the process is unable to allocate executable memory even if compromised, > whereas JSC::Options::useJIT() = false; just makes it so it doesn't try to > under normal circumstances. Oh I see. I didn't understand that initially. ExecutableAllocator::setJITEnabled() may be more suitable then, I'll look into it.
Comment on attachment 443435 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443435&action=review >>>>> Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.h:100 >>>>> JSC::ExecutableAllocator::setJITEnabled(false); >>>> >>>> This does some stronger things that we may want to do in captive portal mode. >>>> We should also audit the users of disable-jit and see if they can migrate to this as a replacement. >>> >>> >> >> I may be wrong, but I think ExecutableAllocator::setJITEnabled makes it so the process is unable to allocate executable memory even if compromised, whereas JSC::Options::useJIT() = false; just makes it so it doesn't try to under normal circumstances. > > Oh I see. I didn't understand that initially. ExecutableAllocator::setJITEnabled() may be more suitable then, I'll look into it. If I remember correctly, the reason I used `JSC::Options::useJIT() = false` is that `[webView _isJITEnabled:(void(^)(BOOL))completionHandler]` was returning NO when using `JSC::ExecutableAllocator::setJITEnabled(false)`. This may be an issue with _isJITEnabled not properly detecting that JIT is disable though.
Created attachment 443642 [details] Patch
Created attachment 443646 [details] Patch
Per Alex's feedback: - Now using `ExecutableAllocator::setJITEnabled(false)` instead of `JSC::Options::Options::useJIT() = false;`. - Also giving the browser entitlement to TestWebKitAPI on iOS so I can enable the API tests on iOS as well.
ping review?
Comment on attachment 443646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=443646&action=review r=me > Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.h:74 > +/*! @abstract A boolean indicating whether captive portal mode is enabled. nit: Captive Portal ? > Tools/TestWebKitAPI/Configurations/TestWebKitAPI-iOS.entitlements:14 > + <true/> Nit: Weird whitespace.
Committed r285594 (244102@main): <https://commits.webkit.org/244102@main>