1163545 - Bypass AppCache completely when Service Workers supported & registered

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Szmozsánszky István [:flaki]]

Discussions on enhancing backward-compatibility of the Service Workers spec resulted in allowing AppCache & Service Workers used simultaneously, but a registered service worker script in a supporting browser should override and evict the AppCache completely.

Related spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=27482

Discussion: https://github.com/slightlyoff/ServiceWorker/issues/697

Chrome status (partially implemented): https://code.google.com/p/chromium/issues/detail?id=410665

Found a FixMe by :nsm, but didn't see this filed so I went ahead filing this bug: https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#

Comment 1

[Szmozsánszky István [:flaki]]

(In reply to Szmozsánszky István [:flaki] from comment #0)
> Found a FixMe by :nsm, but didn't see this filed so I went ahead filing this
> bug:

Correct link is https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#1191

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I don't think I'll be working on this immediately. Clearing ni?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I think we need to consider this a bit more as well.  I believe there are sites that are built on appcache today, but also want to use a service worker for push (or to get android chrome's install banner).  They can't migrate off appcache, but might have a service worker for other reasons.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I think we might need to do something here for v1.  We need to at least make sure things don't completely blow up for people using SW and appcache simultaneously.

We can move this back to v2 or v3 if people don't think its a critical problem.

Comment 5

[(no longer active)]

We have telemetry on app cache usage <https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2015-08-26&keys=__none__!__none__!__none__&max_channel_version=release%252F40&measure=HTTP_OFFLINE_CACHE_DOCUMENT_LOAD&min_channel_version=release%252F39&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2015-08-07&table=0&trim=1&use_submission_date=0> and on the release channel, if I'm doing the math correctly, the usage is 0.043% of page loads.

Based on that, I don't really think this should block v1, but if the fix is super simple, it's definitely nice to fix.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: appCache.patch

Comment 7

[Andrea Marchesini [:baku]]

Attachment: appCache.patch

Comment 8

[Josh Matthews [:jdm]]

Comment on attachment 8658797 [details] [diff] [review]
appCache.patch

Review of attachment 8658797 [details] [diff] [review]:
-----------------------------------------------------------------

A test for these changes would be useful, I think.

::: dom/base/nsContentSink.cpp
@@ +1063,5 @@
>    if (!mDocShell) {
>      return;
>    }
>  
> +  // If this document has been interecepted, let's skip the processing of the

Don't bother processing offline manifest for documents with a ServiceWorker.

Comment 9

[Andrea Marchesini [:baku]]

I'll file a follow up for the test. I'm going to land this patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/982d9a863989

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/982d9a863989

Comment 11

[Kohei Yoshino [:kohei]]

Added a note to https://www.fxsitecompat.com/en-US/docs/2015/application-cache-api-has-been-deprecated/