1147214 - Enable app:// urls to use SW

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andrew Overholt [:overholt]]

Jonas thinks this may not be necessary but Vivien and Fabrice will know more as prototyping happens.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Do we need to support app:// urls in Cache as well?  Restricted to http and https right now.

Comment 3

[Andrew Overholt [:overholt]]

We should have this depend on the Necko work for supporting !// resources.

Comment 4

[Andrew Overholt [:overholt]]

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/app/AppProtocolHandler.cpp

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It seems like we will be able to do prototyping without this. So hopefully we can get away without ever having to fix this bug.

Comment 6

[Andrew Overholt [:overholt]]

Let's close it, then, and re-open if we need to.

Comment 7

[[:fabrice] Fabrice Desré]

Not sure why you re-opened, but note that if you set the dom.apps.developer_mode pref to true, you can install any kind of app (even certified) as a hosted app.

Comment 8

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fabrice Desré [:fabrice] from comment #7)
> Not sure why you re-opened, but note that if you set the
> dom.apps.developer_mode pref to true, you can install any kind of app (even
> certified) as a hosted app.

While that's true, I don't think we'll be able to get completely rid of the app:// protocol. Even with the new architecture and the new "let's update everything we can" I think there will still be some apps that will *not* be updateable and that we might want/need to be installed in a way that's more similar to what we currently have than to the new model.

For example, to allow OEMs to not have to modify Gecko code to add their own functionality we might have to end enabling something like the engineering mode API (a command/response API in other words) for some apps. But those apps will *not* be updateable, nor downloadeable, and should have a "dangerous to link" and basically all the things that the current app:// apps do.

So... I really think we should get app:// working both as a cacheable target *and* as a valid service worker origin.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

FWIW, for the development work required to start porting Gaia apps to the new architecture, we don't need app:// as a valid service worker origin. We can use the developer mode as Fabrice mentions in comment 7.

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #9)
> FWIW, for the development work required to start porting Gaia apps to the
> new architecture, we don't need app:// as a valid service worker origin. We
> can use the developer mode as Fabrice mentions in comment 7.

I know that (although at this step it would be handy to have this working also cause otherwise preloading doesn't work and so the initial boot of the phone is... kinda different of how it should be). I wasn't talking that much about development as of the end device.

Comment 11

[[:fabrice] Fabrice Desré]

I'm pretty sure we won't keep the app:// protocol in the mid to long term. I don't mind if someone wants to add SW worker support for app, but at some point we'll break it.

Antonio, what about `installing` your dangerous apps from https://myapp.local ? ;)

Comment 12

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Antonio Manuel Amaya Calvo (:amac) from comment #8)
> (In reply to Fabrice Desré [:fabrice] from comment #7)
> > Not sure why you re-opened, but note that if you set the
> > dom.apps.developer_mode pref to true, you can install any kind of app (even
> > certified) as a hosted app.
> 
> While that's true, I don't think we'll be able to get completely rid of the
> app:// protocol. Even with the new architecture and the new "let's update
> everything we can" I think there will still be some apps that will *not* be
> updateable and that we might want/need to be installed in a way that's more
> similar to what we currently have than to the new model.
> 
> For example, to allow OEMs to not have to modify Gecko code to add their own
> functionality we might have to end enabling something like the engineering
> mode API (a command/response API in other words) for some apps. But those
> apps will *not* be updateable, nor downloadeable, and should have a
> "dangerous to link" and basically all the things that the current app://
> apps do.
> 
> So... I really think we should get app:// working both as a cacheable target
> *and* as a valid service worker origin.

Can we achieve equivalent security while also deprecating app:

- prevent updates as per comment 11
- with allowing linking to apps, I think we need a mechanism for apps to be able to whitelist their valid entry points (including none, to prohibit linking entirely)
- same with framing: we need to be able to set x-frame-options

Would that be enough?

Comment 13

[Doug Turner (:dougt)]

Attachment: add_push_to_about_serviceworkers

This patch allows us to create sw from app: urls.

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 8596057 [details] [diff] [review]
add_push_to_about_serviceworkers

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

Are you sure this patch is in the right bug?

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 8596057 [details] [diff] [review]
add_push_to_about_serviceworkers

nope. :(

Comment 16

[Doug Turner (:dougt)]

Attachment: make_app_play_well_with_sw

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 8597281 [details] [diff] [review]
make_app_play_well_with_sw

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

looks fine to me, but that would not hurt to have a review from someone that actually knows that code.

Comment 18

[(no longer active)]

baku would be a good reviewer, I think!

Comment 19

[Andrea Marchesini [:baku]]

Comment on attachment 8597281 [details] [diff] [review]
make_app_play_well_with_sw

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

::: dom/cache/TypeUtils.cpp
@@ +67,5 @@
>  
>    if (aSchemeValidOut) {
>      nsAutoCString scheme(Substring(flatURL, schemePos, schemeLen));
> +    *aSchemeValidOut = scheme.LowerCaseEqualsLiteral("app") ||
> +                       scheme.LowerCaseEqualsLiteral("http") ||

I would change the order of these conditions in order to avoid the number lower comparisons we do.
What about if we do: https, http and then app?

::: dom/workers/ServiceWorkerScriptCache.cpp
@@ +433,1 @@
>    }

I would like to have a check of the scheme in case the channel is not a nsIHttpChannel.
What about if you retrieve the URI from the request and you check its scheme?

Comment 20

[Andrea Marchesini [:baku]]

ah... can we have a test for app:// ?

Comment 21

[Doug Turner (:dougt)]

as for testing, what were you thinking?  i am going to land my test push app in gaia soon which will use sw. is that what you're looking for?

Comment 22

[Doug Turner (:dougt)]

Attachment: 0002-Bug-1147214-Make-app-play-nicely-with-service-worker.patch

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I also don't expect us to keep the app:// protocol for very long.

For content that we don't want to update, we should be able to do some internal configuration so that even though it has a https:// URL, we won't ever update the serviceworker/package that backs that URL.

Comment 24

[Doug Turner (:dougt)]

Attachment: 0002-Bug-1147214-Make-app-play-nicely-with-service-worker.patch

Comment 25

[Doug Turner (:dougt)]

hijacking this bug.  if there is more work for fetch, file followups plz.

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/784db048ee31

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/784db048ee31

Comment 28

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

Attachment: sw_app.zip

Hi Doug,

was trying this out, but couldn't make it work.

I've attached a zip file containing an app that can be loaded via webide. It's a standard (non privileged nor certified) package app, that only tries to register a sw.

When running as an app, with app:// protocol, I get the following error:
Error: Failed to load script (nsresult = 0x8053001a)

If, I load the same over an https connection via the browser in the same phone, it manages to install correctly the sw.

Didn't reopen the bug since I don't know if I'm missing something, or doing something directly wrong.

Comment 29

[Doug Turner (:dougt)]

I think we still need some of these evil preferences:
    https://github.com/mozilla/gecko-dev/commit/aa211f7ed61396d1dad764ca10a396747abe3f9d

Comment 30

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

Yup, I was using those evil things:

user_pref("devtools.serviceWorkers.testing.enabled", true);
user_pref("dom.serviceWorkers.enabled", true);
user_pref("dom.apps.developer_mode", true);
user_pref("network.disable.ipc.security", true);

Will double check on my side. Thanks!

Comment 31

[Pulsebot]

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

Comment 32

[Fernando Jiménez Moreno [:ferjm]]

You are seeing that error because bug 1120501 accidentally reverted this change http://hg.mozilla.org/mozilla-central/rev/784db048ee31#l1.13 I just pushed that bit back.

Comment 33

[Carsten Book [:Tomcat]]

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