Bug 186166 - Make NetworkProcess get cache storage parameters at creation of the CacheStorage engine
Summary: Make NetworkProcess get cache storage parameters at creation of the CacheStor...
Status: RESOLVED FIXED
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:
Blocks:
 
Reported: 2018-05-31 16:24 PDT by youenn fablet
Modified: 2019-01-22 18:36 PST (History)
9 users (show)

See Also:


Attachments
Patch (56.16 KB, patch)
2018-05-31 16:36 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (56.15 KB, patch)
2018-06-01 08:24 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (56.48 KB, patch)
2018-06-14 16:46 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2018-05-31 16:24:01 PDT
That will help speed up time and ensure we only get a sandbox extension when actually needed.
Comment 1 youenn fablet 2018-05-31 16:36:24 PDT
Created attachment 341702 [details]
Patch
Comment 2 youenn fablet 2018-05-31 16:37:13 PDT
<rdar://problem/40452995>
Comment 3 youenn fablet 2018-06-01 08:24:39 PDT
Created attachment 341762 [details]
Patch
Comment 4 Alex Christensen 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?
Comment 5 youenn fablet 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.
Comment 6 youenn fablet 2018-06-14 16:46:16 PDT
Created attachment 342777 [details]
Patch
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2018-06-14 18:05:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Michael Catanzaro 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.