Bug 186166 – Make NetworkProcess get cache storage parameters at creation of the CacheStorage engine

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

That will help speed up time and ensure we only get a sandbox extension when actually needed.

[youenn fablet]

Created attachment 341702 [details]
Patch

[youenn fablet]

<rdar://problem/40452995>

[youenn fablet]

Created attachment 341762 [details]
Patch

[Alex Christensen]

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]

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

Created attachment 342777 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 342777 [details]
Patch

Clearing flags on attachment: 342777

Committed r232863: <https://trac.webkit.org/changeset/232863>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

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.