Bug 240003 - SWOriginStore is no longer needed
Summary: SWOriginStore is no longer needed
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 240197
Blocks:
  Show dependency treegraph
 
Reported: 2022-05-03 04:00 PDT by youenn fablet
Modified: 2022-05-14 00:19 PDT (History)
10 users (show)

See Also:


Attachments
Patch (59.61 KB, patch)
2022-05-03 04:46 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (60.27 KB, patch)
2022-05-04 02:30 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (61.02 KB, patch)
2022-05-04 06:01 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Reduced patch (30.03 KB, patch)
2022-05-11 06:11 PDT, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2022-05-03 04:00:06 PDT
SWOriginStore is no longer needed
Comment 1 youenn fablet 2022-05-03 04:46:13 PDT
Created attachment 458732 [details]
Patch
Comment 2 youenn fablet 2022-05-03 06:31:17 PDT
Comment on attachment 458732 [details]
Patch

WPE is a unified build issue, putting as r?
Comment 3 Chris Dumez 2022-05-03 07:54:47 PDT
Comment on attachment 458732 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        is to optimize the case of a page being loaded by service worker instead of app cache which is a tiny edge case.

I don't understand. As I remember it, every page load has to check if there is a service worker handling the load or not. My understanding is that we introduced the SWOriginStore to check if there was a service worker registered for a given origin, so we could bypass the service worker check with the network process before proceeding with the load. Seems useful to me, for every load. 

Am I remembering wrong? Is this no longer needed for some reason?

The idea of this store was to optimize page loads for pages NOT handled by service workers, which is a common case. Your change log seems to state the opposite.
Comment 4 youenn fablet 2022-05-03 07:58:40 PDT
We used to do IPC to get the registration then decide what to do.
We are now sending the load to network process and matching the navigation loads when receiving them in network process. Hence why we no longer care about this optimization.
Comment 5 Chris Dumez 2022-05-03 08:08:32 PDT
(In reply to youenn fablet from comment #4)
> We used to do IPC to get the registration then decide what to do.
> We are now sending the load to network process and matching the navigation
> loads when receiving them in network process. Hence why we no longer care
> about this optimization.

Oh, I get it now. Our design has indeed changed a lot since the introduction of this "optimization".
Comment 6 Chris Dumez 2022-05-03 08:10:27 PDT
Comment on attachment 458732 [details]
Patch

r=me but please add the missing include (or namespace) to make the WPT bot happy:
/app/webkit/Source/WebKit/WebProcess/Storage/WebSharedWorkerContextManagerConnection.cpp:118:47: error: ‘ScriptExecutionContextIdentifier’ has not been declared
Comment 7 youenn fablet 2022-05-04 02:30:46 PDT
Created attachment 458782 [details]
Patch for landing
Comment 8 youenn fablet 2022-05-04 06:01:19 PDT
Created attachment 458790 [details]
Patch for landing
Comment 9 EWS 2022-05-05 00:39:27 PDT
Committed r293824 (250297@main): <https://commits.webkit.org/250297@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 458790 [details].
Comment 10 Radar WebKit Bug Importer 2022-05-05 00:40:14 PDT
<rdar://problem/92780026>
Comment 11 WebKit Commit Bot 2022-05-06 17:43:05 PDT
Re-opened since this is blocked by bug 240197
Comment 12 youenn fablet 2022-05-11 06:11:49 PDT
Created attachment 459161 [details]
Reduced patch
Comment 13 Chris Dumez 2022-05-11 14:31:04 PDT
Comment on attachment 459161 [details]
Reduced patch

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

> Source/WebCore/loader/DocumentLoader.cpp:-582
> -        callback(std::nullopt);

I did a quick speedometer investigation and I think this is the regression right here. We used to early return here on Speedometer quite a bit:
```
CHRIS: DocumentLoader::matchRegistration() early return
1   0x183df25e2 WebCore::DocumentLoader::matchRegistration(WTF::URL const&, WTF::CompletionHandler<void (std::__1::optional<WebCore::ServiceWorkerRegistrationData>&&)>&&)
2   0x182d83a9c WebCore::DocumentLoader::responseReceived(WebCore::CachedResource&, WebCore::ResourceResponse const&, WTF::CompletionHandler<void ()>&&)
3   0x182dc9761 WTF::Detail::CallableWrapper<WebCore::CachedRawResource::didAddClient(WebCore::CachedResourceClient&)::$_0, void, WebCore::ResourceRequest&&>::call(WebCore::ResourceRequest&&)
4   0x182dbed60 WebCore::iterateRedirects(WebCore::CachedResourceHandle<WebCore::CachedRawResource>&&, WebCore::CachedRawResourceClient&, WTF::Vector<std::__1::pair<WebCore::ResourceRequest, WebCore::ResourceResponse>, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&, WTF::CompletionHandler<void (WebCore::ResourceRequest&&)>&&)
5   0x182dbe60e WebCore::CachedRawResource::didAddClient(WebCore::CachedResourceClient&)
6   0x182e2e650 WebCore::ThreadTimers::sharedTimerFiredInternal()
7   0x182e35daf WebCore::timerFired(__CFRunLoopTimer*, void*)
8   0x7ff8064bbf59 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
9   0x7ff8064bba48 __CFRunLoopDoTimer
10  0x7ff8064bb5b8 __CFRunLoopDoTimers
11  0x7ff8064a1cf6 __CFRunLoopRun
12  0x7ff8064a0e5c CFRunLoopRunSpecific
13  0x7ff807301d6a -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
14  0x7ff80738c7b7 -[NSRunLoop(NSRunLoop) run]
15  0x7ff806124816 _xpc_objc_main
16  0x7ff806124239 xpc_main
17  0x17a948341 WebKit::XPCServiceMain(int, char const**)
18  0x11affd51e
```

Now, instead of this very fast check, we end up doing IPC to the network process.