Closed Bug 1147214 Opened 9 years ago Closed 9 years ago

Enable app:// urls to use SW

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: overholt, Assigned: dougt)

References

Details

Attachments

(3 files, 2 obsolete files)

      No description provided.
Jonas thinks this may not be necessary but Vivien and Fabrice will know more as prototyping happens.
Do we need to support app:// urls in Cache as well?  Restricted to http and https right now.
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.
Let's close it, then, and re-open if we need to.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
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)
Attached patch add_push_to_about_serviceworkers (obsolete) — Splinter Review
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)
Comment on attachment 8596057 [details] [diff] [review]
add_push_to_about_serviceworkers

nope. :(
Attachment #8596057 - Attachment is obsolete: true
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+
baku would be a good reviewer, I think!
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:// ?
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?
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.
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
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Attached file 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)
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.
Depends on: 1171651
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.