Bug 240003 – SWOriginStore is no longer needed

• Status: REOPENED
• Component: Service Workers

[youenn fablet]

SWOriginStore is no longer needed

[youenn fablet]

Created attachment 458732 [details]
Patch

[youenn fablet]

Comment on attachment 458732 [details]
Patch

WPE is a unified build issue, putting as r?

[Chris Dumez]

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.

[youenn fablet]

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.

[Chris Dumez]

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

[Chris Dumez]

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

[youenn fablet]

Created attachment 458782 [details]
Patch for landing

[youenn fablet]

Created attachment 458790 [details]
Patch for landing

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/92780026>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 240197

[youenn fablet]

Created attachment 459161 [details]
Reduced patch

[Chris Dumez]

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.