1171651 - Disable the interception of app:// URIs in service workers on release builds

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[(no longer active)]

Our interception of app:// URIs in service workers is completely broken right now.  I will first discuss some of the issues, and will then try to propose a solution.

Because app:// URIs are not fetched through HTTP, pretty much every concept about service workers breaks down with what we have right now.  Here are a number of issues that I know of so far:

* Imagine that a service worker does: event.respondWith(Promise.reject()).  This will right now cancel the underlying nsJARChannel, except that nsJARChannels cannot be properly canceled, so we assert fatally.

* Imagine that a service worker does: event.respondWith("foo", {status: 404}).  There is no way for us to represent this in the context of nsJARChannels, since those don't really have the concept of a status code.  I think what happens in this case is that we would provide a Response with a body of "foo" and nothing will know that this was a 404.

* Imagine that a service worker does: event.respondWith(Response.redirect("http://foo.com")).  nsJARChannels don't know how to redirect, so that won't work.  (Even worse, imagine what should happen when you redirect to another app:// URI!)

* We should probably do the hacks such as bug 804395 for fetch() as well.

I'm pretty sure that there are other cases that I can't think about right now.  For example, anywhere that an HTTP response header does something significant, we're probably failing to handle that properly.  It may even be impossible to decide what the desired behavior is in some of these cases (for example, the case of redirects.)

There are various assumptions made by different places in Gecko that may not hold with our current setup, the example of responding with a rejected promise is one such example, but there are probably many more to be found.


Almost all of these boil down to the fact that app:// URIs are fetched through something other than HTTP.

I think the best way to fix this is to change that.

My idea is to modify the app protocol handler to create a special kind of nsHttpChannel that we will need to implement here.  These special channels will behave exactly the same as normal HTTP channels, *except* when we "decide to go to the network".  Going to the network may mean the service worker not responding to the fetch event, the skip service worker flag being set, or even the default case of there not being a service worker at all.

When this type of channel wants to go to the network, it will create an nsJARChannel internally, AsyncOpen it, and will handle the notifications from the JAR channel and send the data back to the consumer.

Conceptually you can imagine this being similar to our normal HTTP necko stack talking to a remote HTTP server which does not understand any request headers, and does only generate either a network error, or 200 responses with no response header.

This approach has a number of benefits:
1. From the viewpoint of everything outside this special mode of HTTP channels, things are the same no matter whether we're intercepting http(s):// or app://.  This means that for example all of the tests that we currently have for http(s):// will actually be valid for b2g app:// interception.
2. When we hit app:// specific bugs, there will be a single place in the code base to look at, rather than the current situation where various things that are making assumptions about channels, requests, responses and whatnot mysteriously failing.
3. From the viewpoint of the app://, the interception semantics are the same as in the HTTPS case, so if in the future we transition a gaia app using service workers to be served off of https://, its interception code will work the exact same way, since the differences in of the JAR and HTTP channels will be completely hidden from the service worker (since before we decide to go to the network, the service worker is intercepting an HTTP channel.)


The alternative is to fix all of the places in Gecko where we are making the wrong assumptions and fix them, and also write pretty much copies of all of the tests that we have for service workers with HTTP(S) for app://.  That is a pretty bad alternative in my opinion.


What do people think about this plan?

Comment 1

[[:fabrice] Fabrice Desré]

I lost this argument before, but since we now have support for http served packages (bug 1036275), why do we still try to shoehorn SW support in app:// ?

Comment 2

[(no longer active)]

(In reply to Fabrice Desré [:fabrice] from comment #1)
> I lost this argument before, but since we now have support for http served
> packages (bug 1036275), why do we still try to shoehorn SW support in app://
> ?

I would *love* for us to remove support for intercepting app://.  I complained several times before about this as well to no avail.  If we can use bug 1036275 instead of intercepting app:// URIs, that would be lovely.

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

FTR supporting the app:// protocol on service workers was done mostly because:

1. https://bugzilla.mozilla.org/show_bug.cgi?id=1147214#c8 and https://bugzilla.mozilla.org/show_bug.cgi?id=1147214#c10
2. The lack of sw pre-loading support (Bug 1156198)
3. The inability to run perf tests in raptor using hosted apps (Bug 1159153)

At this point, having the basic support to intercept app:// URIs unblocks people to start working on the sw related changes required by the new Gaia architecture without moving to a hosted app model and to start writing performance tests that could run in Raptor today.

I agree (and suffered myself) that the interception support is quite poor and hacky. We only support the very basic cases, similar to what we do with XHR requests to app:// URIs. And there are many other cases where we fail badly. Although I don't expect Gaia apps to find these cases that easily, at least using serviceworkerware[1] as they are supposed to use.

I like Ehsan's proposal, but given that supporting the interception of app:// URIs should be a temporary solution, I wonder if it's worth putting any extra effort here. Maybe we should simply focus on pushing forward the move to hosted apps (or hosted packaged apps) and to fix the related blockers.

Francisco, what do you think?

Antonio, you expressed your concerns about not supporting sw and the interception of app:// URIs requests at https://bugzilla.mozilla.org/show_bug.cgi?id=1147214#c8 Is this still the case?

Eli, I haven't checked in a while, but if bug 1159153 is still valid, do you think that we could prioritize that to unblock writing perf tests for service workers enabled apps?

Thanks!

[1] https://github.com/arcturus/serviceworkerware

Comment 4

[:Eli Perelman]

Bug 1159153 comment 1

Comment 5

[(no longer active)]

FWIW, if we plan to not use this interception in production code, I would like to see a patch which #ifdef's this code out on RELEASE_BUILD to make sure that we don't end up shipping it accidentally.

Comment 6

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #3)
> FTR supporting the app:// protocol on service workers was done mostly
> because:
> 
> 1. https://bugzilla.mozilla.org/show_bug.cgi?id=1147214#c8 and
> https://bugzilla.mozilla.org/show_bug.cgi?id=1147214#c10
> 2. The lack of sw pre-loading support (Bug 1156198)
> 3. The inability to run perf tests in raptor using hosted apps (Bug 1159153)
> 
> At this point, having the basic support to intercept app:// URIs unblocks
> people to start working on the sw related changes required by the new Gaia
> architecture without moving to a hosted app model and to start writing
> performance tests that could run in Raptor today.
> 
> I agree (and suffered myself) that the interception support is quite poor
> and hacky. We only support the very basic cases, similar to what we do with
> XHR requests to app:// URIs. And there are many other cases where we fail
> badly. Although I don't expect Gaia apps to find these cases that easily, at
> least using serviceworkerware[1] as they are supposed to use.
> 
> I like Ehsan's proposal, but given that supporting the interception of
> app:// URIs should be a temporary solution, I wonder if it's worth putting
> any extra effort here. Maybe we should simply focus on pushing forward the
> move to hosted apps (or hosted packaged apps) and to fix the related
> blockers.
> 
> Francisco, what do you think?


Already commented in a sepparate thread, I won't put any extra resources in adding more support, current one is enough, as will move to a pure hosted environment, but having this support now will unblock us for testing and trying several of the concepts that we are working on with Sw.

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

I'll do this on the next sprint

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

This is a naive approach, but it might be enough. We still ship the code, but we don't do the interception.

Comment 9

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

Comment on attachment 8626760 [details] [diff] [review]
v1

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

I'd like to see the test changes.

::: modules/libjar/nsJARChannel.cpp
@@ +948,5 @@
>      mListener = listener;
>      mListenerContext = ctx;
>      mIsPending = true;
>  
> +#ifndef RELEASE_BUILD

Please add this bug's number here for posterity. This is also going to break tests that rely on this right? So those tests should be disabled with appropriate entries in mochitest.ini.

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0da291231b82

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8627101 [details] [diff] [review]
v2

Andrea, could you take a look while Nikhil is on PTO, please? Thanks!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb0d49600bd4

Comment 14

[(no longer active)]

Fernando, it seems to me that the condition under which you have disabled the test is wrong: <https://hg.mozilla.org/integration/mozilla-inbound/rev/eb0d49600bd4#l1.12>.

This test will start failing on debug builds when the code is migrated to beta.

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/eb0d49600bd4