Bug 229203 – [WK2] Implement process-swapping based on Cross-Origin-Opener-Policy HTTP header

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Implement process-swapping based on Cross-Origin-Opener-Policy HTTP header to improve security.

[Radar WebKit Bug Importer]

<rdar://problem/82047686>

[Chris Dumez]

Created attachment 435727 [details]
WIP patch

[Chris Dumez]

Created attachment 435729 [details]
WIP patch

[Chris Dumez]

Created attachment 435776 [details]
Patch

[Chris Dumez]

Created attachment 435778 [details]
Patch

[Chris Dumez]

Created attachment 435785 [details]
Patch

[Alex Christensen]

Comment on attachment 435785 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:1042
> +    if (m_isContinuingLoadAfterResponsePolicyCheck) {

Should this be std::exchange(m_isContinuingLoadAfterResponsePolicyCheck, false)?

> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:233
> +    auto* previousMainFrame = m_page.mainFrame();

May as well use a RefPtr here.

> Source/WebKit/UIProcess/ProvisionalPageProxy.h:131
> +        uint64_t listenerID, const UserData&);

Can we keep all the parameters on one line?

> Source/WebKit/UIProcess/WebPageProxy.cpp:5638
> +    if (!navigation) {

Why is this needed now but wasn't before?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1221
> +    [webViewConfiguration preferences].javaScriptCanOpenWindowsAutomatically = YES;

Is this only needed on iOS?  Could we only do it on iOS if it is?
setJavaScriptCanOpenWindowsAutomaticallyIfNecessary(webViewConfiguration)

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:7226
> +    // When the provisional load starts in the provisional process, kill the WebView's processes.

I don't understand this comment.

[Chris Dumez]

Comment on attachment 435785 [details]
Patch

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

>> Source/WebCore/loader/DocumentLoader.cpp:1042
>> +    if (m_isContinuingLoadAfterResponsePolicyCheck) {
> 
> Should this be std::exchange(m_isContinuingLoadAfterResponsePolicyCheck, false)?

I doesn't hurt. I will make the change.

>> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:233
>> +    auto* previousMainFrame = m_page.mainFrame();
> 
> May as well use a RefPtr here.

OK

>> Source/WebKit/UIProcess/ProvisionalPageProxy.h:131
>> +        uint64_t listenerID, const UserData&);
> 
> Can we keep all the parameters on one line?

Not new but OK.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:5638
>> +    if (!navigation) {
> 
> Why is this needed now but wasn't before?

I'll double check if this is still needed. It might be a reminiscence of an earlier iteration. Basically, the issue is that we need a NavigationAction object to do process-swapping. We have the same logic in decidePolicyForNavigationAction, for the same reason.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1221
>> +    [webViewConfiguration preferences].javaScriptCanOpenWindowsAutomatically = YES;
> 
> Is this only needed on iOS?  Could we only do it on iOS if it is?
> setJavaScriptCanOpenWindowsAutomaticallyIfNecessary(webViewConfiguration)

Yes, only iOS requires it but I didn't feel adding #ifdefs was worth it?

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:7226
>> +    // When the provisional load starts in the provisional process, kill the WebView's processes.
> 
> I don't understand this comment.

Oops, looks like a bad copy/paste. Will drop.

[Chris Dumez]

Comment on attachment 435785 [details]
Patch

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

>>> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1221
>>> +    [webViewConfiguration preferences].javaScriptCanOpenWindowsAutomatically = YES;
>> 
>> Is this only needed on iOS?  Could we only do it on iOS if it is?
>> setJavaScriptCanOpenWindowsAutomaticallyIfNecessary(webViewConfiguration)
> 
> Yes, only iOS requires it but I didn't feel adding #ifdefs was worth it?

From our API:
/*! @abstract A Boolean value indicating whether JavaScript can open
 windows without user interaction.
 @discussion The default value is NO in iOS and YES in OS X.
 */
@property (nonatomic) BOOL javaScriptCanOpenWindowsAutomatically;

So this is the reason we need to set it to YES only on iOS: It is already YES on macOS by default. Therefore, it doesn't hurt to set it to YES no matter what.

[Chris Dumez]

Created attachment 435892 [details]
Patch

[Chris Dumez]

Comment on attachment 435892 [details]
Patch

I think the imported/w3c/web-platform-tests/IndexedDB/serialize-sharedarraybuffer-throws.https.html flakiness may be a regression from this patch. I'll investigate.

[Chris Dumez]

Created attachment 436208 [details]
Patch

[Chris Dumez]

Created attachment 436222 [details]
Patch

[Chris Dumez]

Created attachment 436244 [details]
Patch

[Chris Dumez]

Created attachment 436246 [details]
Patch

[Chris Dumez]

Created attachment 436285 [details]
Patch

[Chris Dumez]

Created attachment 436311 [details]
Patch

[Chris Dumez]

Created attachment 436319 [details]
Patch

[EWS]

Committed r281516 (240889@main): <https://commits.webkit.org/240889@main>

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

imported/w3c/web-platform-tests/IndexedDB/serialize-sharedarraybuffer-throws.https.html still shows up as a flaky test on EWS. I will create a new bug for this.

[Chris Dumez]

Follow-up in https://trac.webkit.org/changeset/281632/webkit to fix fast/loader/reload-zero-byte-plugin.html.