Bug 182444 – ServiceWorker registration should store any script fetched through importScripts

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

We should be implementing https://w3c.github.io/ServiceWorker/#importscripts basically.

[Radar WebKit Bug Importer]

<rdar://problem/37164835>

[Jim Pick]

I've been having problems with service worker generated using Google's Workbox tool not working offline after a period of time (on iOS).

I believe it's because Workbox splits its functionality into several small .js files which loaded by importScripts. The offline functionality seems to work for while, and then it doesn't. I believe it's because the workbox-v3.2.0/*.js requests in the cache are getting evicted, and they aren't stored otherwise. When if fails, I get "Safari cannot open the page because your iPhone is not connected to the Internet."

The app is simple and completely open source:

https://dat-shopping-list.glitch.me/

The code to generate the service worker is here:

https://glitch.com/edit/#!/dat-shopping-list?path=server/makeServiceWorker.js:1:0

I've tried adding a cache-control http header to the /workbox-* files of 'public,max-age=31536000,immutable' but it seems the scripts still get evicted after a short period of time.

The http headers on google's CDN for workbox only has a one hour expiry.

[youenn fablet]

Created attachment 341102 [details]
WIP

[youenn fablet]

Created attachment 341764 [details]
Patch

[EWS Watchlist]

Comment on attachment 341764 [details]
Patch

Attachment 341764 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7926412

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/registration-mime-types.https.html

[EWS Watchlist]

Created attachment 341770 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[youenn fablet]

Created attachment 341771 [details]
Patch

[Chris Dumez]

Comment on attachment 341771 [details]
Patch

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

> Source/WebCore/workers/WorkerScriptLoader.cpp:57
> +    bool isServiceWorkerGlobalScope = workerGlobalScope.isServiceWorkerGlobalScope();

is<ServiceWorkerGlobalScope>(workerGlobalScope) is the preferred way.

> Source/WebCore/workers/WorkerScriptLoader.cpp:235
> +    return !m_cachedScript.isNull() ? m_cachedScript : m_script.toString();

I am not sure how useful m_cachedScript is here. I think we could reuse m_script and merely append() the script to it, no? Looking at the implementation of StringBuilder, a single append(String) then toString() should be pretty efficient.

> Source/WebCore/workers/service/ServiceWorkerContextData.cpp:35
> +    HashMap<URL, ImportedScript> map;

Could you please add a CrossThreadCopierBase specialization for HashMap to CrossThreadCopier.h? (we already have one for Vector and HashSet). Then you can use crossThreadCopy(scriptResourceMap) here.

> Source/WebCore/workers/service/ServiceWorkerContextData.h:46
> +        String responseURL;

Why isn't this stored as a URL type?

> Source/WebCore/workers/service/ServiceWorkerContextData.h:145
> +    // FIXME: Add a templated version of HashMap encode/decode

What's wrong with the one in ArgumentCoders.h? We already encode / decode HashMap, there's got to be a better way than duplicating this code here.

> Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:133
> +std::optional<ServiceWorkerContextData::ImportedScript> ServiceWorkerGlobalScope::scriptResource(const URL& url) const

Cannot we return a ServiceWorkerContextData::ImportedScript* to avoid copying?

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:50
> +static const int schemaVersion = 2;

do we need to rename all the v1 methods below to v2? 

also, there is an ASSERT_NOT_REACHED() below when this is not true:
if (currentSchema == v1RecordsTableSchema() || currentSchema == v1RecordsTableSchemaAlternate())

Wouldn't people with an existing database hit it?

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:355
> +        HashMap<URL, ServiceWorkerContextData::ImportedScript> scriptResourceMap;

Where is this populated?

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:360
> +

extra blank line?

[Chris Dumez]

Comment on attachment 341771 [details]
Patch

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

>> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:50
>> +static const int schemaVersion = 2;
> 
> do we need to rename all the v1 methods below to v2? 
> 
> also, there is an ASSERT_NOT_REACHED() below when this is not true:
> if (currentSchema == v1RecordsTableSchema() || currentSchema == v1RecordsTableSchemaAlternate())
> 
> Wouldn't people with an existing database hit it?

I would be good if Brady could take a look at the changes to this file.

[youenn fablet]

Created attachment 341942 [details]
Patch

[youenn fablet]

Created attachment 341952 [details]
Patch

[youenn fablet]

Created attachment 341966 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 341966 [details]
Patch

Clearing flags on attachment: 341966

Committed r232516: <https://trac.webkit.org/changeset/232516>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

Sorry to respond to a close ticket but I can't find a way to know when this fix will be integrated into official (non- technical preview) version of Safari iOS. More generally, is there a table mapping official Safari release and WebKit preview releases ?

[youenn fablet]

(In reply to bertrand from comment #15)
> Sorry to respond to a close ticket but I can't find a way to know when this
> fix will be integrated into official (non- technical preview) version of
> Safari iOS. More generally, is there a table mapping official Safari release
> and WebKit preview releases ?

Hi Bertrand,
I believe Mojave or iOS12 should have the fix.

Thanks Youenn. I'll try Workbox again with ios 12.1.4 safari then.

Thus, I assume there is no way to know for sure when a Webkit fix is embedded into an official release... That's a problem.

[Ryosuke Niwa]

(In reply to bertrand from comment #17)
> Thanks Youenn. I'll try Workbox again with ios 12.1.4 safari then.
> 
> Thus, I assume there is no way to know for sure when a Webkit fix is
> embedded into an official release... That's a problem.

It's because after we branch, we merge many fixes. So we can't say we included revision X of WebKit in version Y of Safari.

Here's some trick. If you open Safari's version number, it'd say something like 14607.1.~. Strip "14" or whatever leading two digits, and then look for that branch in:
https://trac.webkit.org/browser/webkit/branches

e.g.
https://trac.webkit.org/browser/webkit/branches/safari-607-branch in this case.
Some tag/branch created off of this branch is probably what Safari includes.