Bug 202891 – Pass CORS-enabled schemes through WebProcess instead of having them NetworkProcess-global

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Pass CORS-enabled schemes through WebProcess instead of having them NetworkProcess-global

[Alex Christensen]

Created attachment 380842 [details]
Patch

[youenn fablet]

Service Worker API tests are failing, is it related?

[Alex Christensen]

I verified the three API test failures do not reproduce on my Mac after applying the patch.  They must've been failing because of something else.

[Alex Christensen]

Created attachment 380925 [details]
Patch

[Alex Christensen]

(I re-uploaded the same patch to run through EWS again)

[Alex Christensen]

Created attachment 380935 [details]
Patch

[Alex Christensen]

Comment on attachment 380935 [details]
Patch

I'm quite confused.  I don't see how this patch can break those two tests in that way on EWS and I can't reproduce it locally.  Youenn, any ideas?  Only the "blob" scheme hits my changed code, and that's not registered or default CORS.

[youenn fablet]

It is indeed strange that iOS is ok but not MacOS.
I do not see either why this patch is failing these tests.

Looking at the error message, both tests do two page loads.
The first one to register a service worker.
The second one gets intercepted by the service worker.
It seems that the second one is not intercepted by the service worker.

We have some optimisations in ServiceWorkerProvider::mayHaveServiceWorkerRegisteredForOrigin to not go to the network process. This is based on WebProcessPool checking for the sql file, which is potentially racy. Plan is to fix that by doing registration matching in network process and remove the need for ServiceWorkerProvider::mayHaveServiceWorkerRegisteredForOrigin.
You could try to upload a patch with ServiceWorkerProvider::mayHaveServiceWorkerRegisteredForOrigin always returning true.

[youenn fablet]

Comment on attachment 380935 [details]
Patch

LGTM other than the failing API test.

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:533
> +    for (auto&& scheme : WTFMove(schemes))

No specific reason for WTFMove() here, or is it for documentation purposes?

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:52
> +NetworkLoadChecker::NetworkLoadChecker(NetworkProcess& networkProcess, NetworkSchemeRegistry* schemeRegistry, FetchOptions&& options, PAL::SessionID sessionID, WebPageProxyIdentifier webPageProxyID, HTTPHeaderMap&& originalRequestHeaders, URL&& url, RefPtr<SecurityOrigin>&& sourceOrigin, RefPtr<SecurityOrigin>&& topOrigin, PreflightPolicy preflightPolicy, String&& referrer, bool isHTTPSUpgradeEnabled, bool shouldCaptureExtraNetworkLoadMetrics, LoadType requestLoadType)

RefPtr<NetworkSchemeRegistry>&& with WTFMove
and I guess Ref<NetworkProces>>&& as well

> Source/WebKit/WebProcess/WebProcess.cpp:1182
> +        m_networkProcessConnection->connection().send(Messages::NetworkConnectionToWebProcess::RegisterURLSchemesAsCORSEnabled(WebCore::LegacySchemeRegistry::allURLSchemesRegisteredAsCORSEnabled()), 0);

We probably do not have tests for that.

[Alex Christensen]

(In reply to youenn fablet from comment #8)
> You could try to upload a patch with
> ServiceWorkerProvider::mayHaveServiceWorkerRegisteredForOrigin always
> returning true.

That makes the API test ServiceWorkers.SWProcessConnectionCreation always fail.

[Alex Christensen]

http://trac.webkit.org/r251138
I'll watch the bots to see if the ServiceWorkers API test actually start failing from this unrelated change and roll out if they do.

[Alex Christensen]

Rolled out in https://trac.webkit.org/changeset/251146/webkit because the bots did see the same failure that EWS did.  Still can't reproduce.

Youenn, could you fix the race condition and I'll retry this?

[Alex Christensen]

Removing race condition in https://bugs.webkit.org/show_bug.cgi?id=203055

[Alex Christensen]

Created attachment 381582 [details]
retry all ews

[Alex Christensen]

Created attachment 381819 [details]
Patch

[Alex Christensen]

ServiceWorker API tests pass.  The one timeout on EWS is WebAuthenticationPanel.PanelHidSuccess2 which is quite flaky <https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.WebAuthenticationPanel.PanelHidSuccess2> and I reported that to the author.

[Alex Christensen]

http://trac.webkit.org/r251547

[Radar WebKit Bug Importer]

<rdar://problem/56590108>