RESOLVED FIXED 186166
Make NetworkProcess get cache storage parameters at creation of the CacheStorage engine
https://bugs.webkit.org/show_bug.cgi?id=186166
Summary Make NetworkProcess get cache storage parameters at creation of the CacheStor...
youenn fablet
Reported 2018-05-31 16:24:01 PDT
That will help speed up time and ensure we only get a sandbox extension when actually needed.
Attachments
Patch (56.16 KB, patch)
2018-05-31 16:36 PDT, youenn fablet
no flags
Patch (56.15 KB, patch)
2018-06-01 08:24 PDT, youenn fablet
no flags
Patch (56.48 KB, patch)
2018-06-14 16:46 PDT, youenn fablet
no flags
youenn fablet
Comment 1 2018-05-31 16:36:24 PDT
youenn fablet
Comment 2 2018-05-31 16:37:13 PDT
youenn fablet
Comment 3 2018-06-01 08:24:39 PDT
Alex Christensen
Comment 4 2018-06-14 16:19:03 PDT
Comment on attachment 341762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=341762&action=review Is it common to load the first page without initializing a cache storage engine at all? > Source/WebKit/NetworkProcess/NetworkProcess.cpp:855 > +void NetworkProcess::cacheStorageParameters(PAL::SessionID sessionID, CacheStorageParametersCallback&& callback) This seems to ask the UIProcess every time this is called unless it is called more than once before we get a response. Do we store the result anywhere? A cacheStorageParameters cache if you will... if (we already have parameters) call completion handler immediately else retrieve cache storage parameters Then store them on NetworkProcess in NetworkProcess::setCacheStorageParameters > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:582 > + m_websiteDataStores.set(store->sessionID(), WTFMove(store)); Parameter evaluation order is undefined. We need to get the sessionID before moving from store. > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:590 > + if (!sessionID.isEphemeral()) > + m_websiteDataStores.remove(sessionID); Don't we want to remove ephemeral sessions, too?
youenn fablet
Comment 5 2018-06-14 16:30:45 PDT
> View in context: > https://bugs.webkit.org/attachment.cgi?id=341762&action=review > > Is it common to load the first page without initializing a cache storage > engine at all? Yes, it is common, you typically do not use a cache storage engine unless your page is served by a service worker. In such a case, we would spin NetworkProcess, spin WebProcess, spin StorageProcess, spin another WebProcess, instantiate service worker in this process, execute some JavaScript and then grab the sandbox extension for cache storage API. > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:855 > > +void NetworkProcess::cacheStorageParameters(PAL::SessionID sessionID, CacheStorageParametersCallback&& callback) > > This seems to ask the UIProcess every time this is called unless it is > called more than once before we get a response. Do we store the result > anywhere? A cacheStorageParameters cache if you will... It does ask UIProcess each time. This is fine as we create one CacheStorageEngine per session ID and this is the one that will cache the information. The CacheStorageEngine stays alive as long as the network process is not notified that the sessionID is no longer valid. > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:582 > > + m_websiteDataStores.set(store->sessionID(), WTFMove(store)); > > Parameter evaluation order is undefined. We need to get the sessionID > before moving from store. Ah, that is right, since we are creating a RefPtr from a Ref. I'll fix it. > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:590 > > + if (!sessionID.isEphemeral()) > > + m_websiteDataStores.remove(sessionID); > > Don't we want to remove ephemeral sessions, too? There should be no need since we are not adding them to the map anyway.
youenn fablet
Comment 6 2018-06-14 16:46:16 PDT
WebKit Commit Bot
Comment 7 2018-06-14 18:05:56 PDT
Comment on attachment 342777 [details] Patch Clearing flags on attachment: 342777 Committed r232863: <https://trac.webkit.org/changeset/232863>
WebKit Commit Bot
Comment 8 2018-06-14 18:05:57 PDT
All reviewed patches have been landed. Closing bug.
Michael Catanzaro
Comment 9 2019-01-22 18:36:15 PST
Comment on attachment 342777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=342777&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:138 > + for (auto& callbacks : m_cacheStorageParametersCallbacks.values()) { > + for (auto& callback : callbacks) > + callback(String { }, 0); > + } See bug #193702.
Note You need to log in before you can comment on or make changes to this bug.