Bug 232737 – Add basic support for launching CaptivePortalMode WebProcesses

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

<rdar://84473037>

[Chris Dumez]

Created attachment 443357 [details]
WIP Patch

[Chris Dumez]

Created attachment 443392 [details]
WIP Patch (with API test)

[Chris Dumez]

Created attachment 443401 [details]
WIP Patch

[Chris Dumez]

Created attachment 443406 [details]
Patch

[Brent Fulgham]

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;

[Alexey Proskuryakov]

The content of attachment 443357 [details] has been deleted

[Alexey Proskuryakov]

The content of attachment 443392 [details] has been deleted

[Alexey Proskuryakov]

The content of attachment 443401 [details] has been deleted

[Chris Dumez]

Created attachment 443412 [details]
Patch

[Tim Horton]

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?

[Chris Dumez]

Created attachment 443418 [details]
Patch

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

Created attachment 443431 [details]
Patch

[Tim Horton]

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.

[Chris Dumez]

Created attachment 443435 [details]
Patch

[Alex Christensen]

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?

[Chris Dumez]

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.

[Alex Christensen]

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.

[Chris Dumez]

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

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 443642 [details]
Patch

[Chris Dumez]

Created attachment 443646 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

ping review?

[Brent Fulgham]

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.

[Chris Dumez]

Committed r285594 (244102@main): <https://commits.webkit.org/244102@main>