Bug 202891 - Pass CORS-enabled schemes through WebProcess instead of having them NetworkProcess-global
Summary: Pass CORS-enabled schemes through WebProcess instead of having them NetworkPr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-12 21:43 PDT by Alex Christensen
Modified: 2019-10-24 12:42 PDT (History)
3 users (show)

See Also:


Attachments
Patch (36.99 KB, patch)
2019-10-12 21:49 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (36.95 KB, patch)
2019-10-14 15:29 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (38.43 KB, patch)
2019-10-14 16:40 PDT, Alex Christensen
youennf: review+
Details | Formatted Diff | Diff
retry all ews (36.94 KB, patch)
2019-10-22 12:34 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (37.63 KB, patch)
2019-10-24 09:38 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-10-12 21:43:26 PDT
Pass CORS-enabled schemes through WebProcess instead of having them NetworkProcess-global
Comment 1 Alex Christensen 2019-10-12 21:49:23 PDT
Created attachment 380842 [details]
Patch
Comment 2 youenn fablet 2019-10-14 11:51:36 PDT
Service Worker API tests are failing, is it related?
Comment 3 Alex Christensen 2019-10-14 13:59:20 PDT
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.
Comment 4 Alex Christensen 2019-10-14 15:29:39 PDT
Created attachment 380925 [details]
Patch
Comment 5 Alex Christensen 2019-10-14 15:43:18 PDT
(I re-uploaded the same patch to run through EWS again)
Comment 6 Alex Christensen 2019-10-14 16:40:10 PDT
Created attachment 380935 [details]
Patch
Comment 7 Alex Christensen 2019-10-14 21:24:35 PDT
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.
Comment 8 youenn fablet 2019-10-15 01:46:35 PDT
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.
Comment 9 youenn fablet 2019-10-15 01:55:20 PDT
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.
Comment 10 Alex Christensen 2019-10-15 08:51:11 PDT
(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.
Comment 11 Alex Christensen 2019-10-15 09:10:38 PDT
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.
Comment 12 Alex Christensen 2019-10-15 10:20:06 PDT
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?
Comment 13 Alex Christensen 2019-10-16 14:59:08 PDT
Removing race condition in https://bugs.webkit.org/show_bug.cgi?id=203055
Comment 14 Alex Christensen 2019-10-22 12:34:10 PDT
Created attachment 381582 [details]
retry all ews
Comment 15 Alex Christensen 2019-10-24 09:38:46 PDT
Created attachment 381819 [details]
Patch
Comment 16 Alex Christensen 2019-10-24 11:37:29 PDT
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.
Comment 17 Alex Christensen 2019-10-24 11:50:39 PDT
http://trac.webkit.org/r251547
Comment 18 Radar WebKit Bug Importer 2019-10-24 12:42:44 PDT
<rdar://problem/56590108>