Enable app:// urls to use SW

RESOLVED FIXED in Firefox 40

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: overholt, Assigned: dougt)

Tracking

(Blocks: 1 bug)

unspecified
mozilla40
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

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

Comment 2

3 years ago
Do we need to support app:// urls in Cache as well?  Restricted to http and https right now.
(Reporter)

Comment 3

3 years ago
We should have this depend on the Necko work for supporting !// resources.
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.
(Reporter)

Comment 6

3 years ago
Let's close it, then, and re-open if we need to.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
(Assignee)

Updated

3 years ago
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Updated

3 years ago
Assignee: nobody → dougt
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.
(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.
Flags: needinfo?(ptheriault)
Flags: needinfo?(fabrice)
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.
(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.
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 ? ;)
Flags: needinfo?(fabrice)
(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?
Flags: needinfo?(ptheriault)
(Assignee)

Comment 13

3 years ago
Created attachment 8596057 [details] [diff] [review]
add_push_to_about_serviceworkers

This patch allows us to create sw from app: urls.
Attachment #8596057 - Flags: review?(fabrice)
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?
Attachment #8596057 - Flags: review?(fabrice)
(Assignee)

Comment 15

3 years ago
Comment on attachment 8596057 [details] [diff] [review]
add_push_to_about_serviceworkers

nope. :(
Attachment #8596057 - Attachment is obsolete: true
(Assignee)

Comment 16

3 years ago
Created attachment 8597281 [details] [diff] [review]
make_app_play_well_with_sw
Attachment #8597281 - Flags: review?(fabrice)
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.
Attachment #8597281 - Flags: review?(fabrice) → review+

Comment 18

3 years ago
baku would be a good reviewer, I think!
(Assignee)

Updated

3 years ago
Attachment #8597281 - Flags: review?(amarchesini)
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?
Attachment #8597281 - Flags: review?(amarchesini) → review+
ah... can we have a test for app:// ?
(Assignee)

Comment 21

3 years ago
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?
(Assignee)

Comment 22

3 years ago
Created attachment 8598138 [details] [diff] [review]
0002-Bug-1147214-Make-app-play-nicely-with-service-worker.patch
(Assignee)

Updated

3 years ago
Blocks: 1151180
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.
(Assignee)

Comment 24

3 years ago
Created attachment 8598245 [details] [diff] [review]
0002-Bug-1147214-Make-app-play-nicely-with-service-worker.patch
Attachment #8598138 - Attachment is obsolete: true
(Assignee)

Comment 25

3 years ago
hijacking this bug.  if there is more work for fetch, file followups plz.
Summary: Intercept app:// requests → Enable app:// urls to use SW
https://hg.mozilla.org/mozilla-central/rev/784db048ee31
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Created attachment 8600270 [details]
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.
Flags: needinfo?(dougt)
(Assignee)

Comment 29

3 years ago
I think we still need some of these evil preferences:
    https://github.com/mozilla/gecko-dev/commit/aa211f7ed61396d1dad764ca10a396747abe3f9d
Flags: needinfo?(dougt)
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!
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.

Updated

3 years ago
Depends on: 1171651
You need to log in before you can comment on or make changes to this bug.