1197379 - remove app:// protocol support from service worker interception

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Over in bug 1171651 it seems we discussed our long term plan was not to ship app protocol interception.  Filing this bug to track actually removing the support from the tree when we get to that point.

Comment 1

[(no longer active)]

Fabrice said that we can take this change now! \o/

Since this will be an issue when fixing bug 1222008, let's rip the band-aid right now.

Comment 3

[(no longer active)]

Attachment: Remove support for intercepting app:// URIs using service workers

Comment 4

[(no longer active)]

Attachment: Remove support for intercepting app:// URIs using service workers

Found some more code that we can delete!

Comment 5

[Pulsebot]

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

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Is the intention here to entirely remove apps support (http:// and app:// origins) or only packaged apps (app:// origin) support? I see that you are removing some tests for apps with http:// origins.

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

[Hubert Figuiere [:hub]]

So now that the music app is completely broken, as the tests show, where do we go?

(bug 1243670 has been filed)

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

My impression is we got the go ahead for this from fabrice in the spirit of this dev-platform thread:

  https://groups.google.com/forum/#!topic/mozilla.dev.platform/gF-kiJV21ro

Comment 11

[Hubert Figuiere [:hub]]

It was my feeling that "doing the right thing™" was what was supposed to happen. Totally breaking everything for the other teams just because you feel like it ain't part of "doing the right thing™". Even worse when you don't let people know. *hint* it breaks everything and you didn't even try to tell the concerned people "I'm gonna break it".

So now I wake up with a bug "your test are broken fix them". Great way to start the day. It is not like we have stopped riding the train.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Are the tests failing due to app:// support being removed?  Or the issue raised in comment 6 about http:// apps?

Comment 13

[Andrew Overholt [:overholt]]

I was under the obviously-mistaken impression that no gaia apps were yet using Service Workers and thus removing this wasn't breaking anything immediately.

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Ben Kelly [:bkelly] from comment #12)
> Are the tests failing due to app:// support being removed?  Or the issue
> raised in comment 6 about http:// apps?

I don't know why the Music app tests are failing, but it is probably not related to what I mentioned in comment 6.

(In reply to Andrew Overholt [:overholt] from comment #13)
> I was under the obviously-mistaken impression that no gaia apps were yet
> using Service Workers and thus removing this wasn't breaking anything
> immediately.

All the usage of SW in Gaia should be under a build flag (NGA_SERVICE_WORKERS=1) and I don't see it being set anywhere when running the tests.

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

It seems that the patch is also removing the work done on bug 1161288 and the Music app is using the Fetch API. So I guess that's the reason of the tests failures.

Comment 16

[Hubert Figuiere [:hub]]

So shall we XHR with http: as protocol?u

Comment 17

[Hubert Figuiere [:hub]]

So can we get a backout now?

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Hubert Figuiere [:hub] from comment #16)
> So shall we XHR with http: as protocol?u

I don't think moving Music from packaged to hosted (so it gets http:// origin) is currently possible given the current permissions model. Using XHR would work with app:// origins and we could also leave Fetch support for app:// origins for a while. But if the intention is to eventually remove all support for app:// origins from the platform we should try to find an alternative solution. In any case, I'd like to wait for [1] to be answered. Maybe we should just freeze (or maybe fork) Gecko for B2G for the time being.

[1] https://groups.google.com/d/msg/mozilla.dev.fxos/-X5fw4WDr7U/KVSLvTujBQAJ

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Hubert Figuiere [:hub] from comment #17)
> So can we get a backout now?

If I am not wrong, that's exactly what Tier 3 does not allow us to do :|

Comment 20

[Hubert Figuiere [:hub]]

So maybe we should stop working on Gaia since Platform remove feature that are needed. I mean if the point of Tier-3 was to kill Gaia, why not just saying so. I wouldn't have wasted time this week on fixing bugs, write testing code for stuff I want to land to make things better.

The platform is burning.

Comment 21

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

(In reply to Hubert Figuiere [:hub] from comment #20)
> So maybe we should stop working on Gaia since Platform remove feature that

I don't think this is the forum for that discussion, we have mailing lists.

Our colleagues from platform did, are, and will be doing the best efforts in their hands to help us, but so far we are in Tier-3.

We can ask nicely if there is a big problem to back-out temporary this bug, taking into account it's nature, but even if it's not possible, we should provide a solution (I would go for mocking the fetch api via xhr) in gaia side.

Comment 22

[(no longer active)]

Please give me some time to go through the comments to see what's going on!

Also as Francisco asked, please let's not convert this bug to a discussion forum for the tier-3 issue.  Thanks!  :-)

Comment 23

[[:fabrice] Fabrice Desré]

(In reply to Ben Kelly [:bkelly] from comment #10)
> My impression is we got the go ahead for this from fabrice in the spirit of
> this dev-platform thread:
> 
>   https://groups.google.com/forum/#!topic/mozilla.dev.platform/gF-kiJV21ro

fwiw, I didn't say anything about removing support for fetch("app://"). Fernando is doing the job shimming fetch with xhr, but I would really appreciate people not sneaking in unrelated changes.

Comment 24

[(no longer active)]

(In reply to [:fabrice] Fabrice Desré from comment #23)
> (In reply to Ben Kelly [:bkelly] from comment #10)
> > My impression is we got the go ahead for this from fabrice in the spirit of
> > this dev-platform thread:
> > 
> >   https://groups.google.com/forum/#!topic/mozilla.dev.platform/gF-kiJV21ro
> 
> fwiw, I didn't say anything about removing support for fetch("app://").
> Fernando is doing the job shimming fetch with xhr, but I would really
> appreciate people not sneaking in unrelated changes.

Assuming a little bit of good faith would be appreciated here.  I didn't realize that's what that code is supposed to do, and the test added for this didn't catch it either, so removing that part was just an accident which unhelpfully wasn't caught by the test either.

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8712674 [details] [diff] [review]
Remove support for intercepting app:// URIs using service workers

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

Thank you for looking and trying to fix the Fetch API changes, Ehsan! It is highly appreciated.

As you asked on IRC, these are the tests that I think are testing http:// origins.

::: dom/workers/test/serviceworkers/test_aboutserviceworkers.html
@@ -1,1 @@
> -<!DOCTYPE HTML>

This one is using a hosted app.

::: dom/workers/test/serviceworkers/test_app_installation.html
@@ -1,1 @@
> -<!DOCTYPE html>

This one.

::: dom/workers/test/serviceworkers/test_clear_origin_data.html
@@ -1,1 @@
> -<!DOCTYPE html>

And this one as well.

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

Justin, you mentioned that the Music app is using the Fetch API, but I can only see that it is using it when NGA_SERVICE_WORKERS=1 [1]. Could the error be somewhere else?

[1] https://mxr.mozilla.org/gaia/source/apps/music/js/view.js#151

Comment 27

[(no longer active)]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #15)
> It seems that the patch is also removing the work done on bug 1161288 and
> the Music app is using the Fetch API. So I guess that's the reason of the
> tests failures.

I looked at this with Fernando on IRC.  As far as I can tell, the code removed in <https://hg.mozilla.org/integration/mozilla-inbound/rev/b46a851ba158#l4.48> is a slightly worse version of the code in the ultimate else branch here <http://hg.mozilla.org/mozilla-central/file/2b73b0a4d52b/dom/fetch/FetchDriver.cpp#l510>.  That's the code that test_fetch_app_protocol.html examines and it makes the test pass.  Therefore I don't think I actually undid the work done in bug 1161288.

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

It seems that the problem is not on the Fetch API but on the Cache API:

[5592] WARNING: CacheStorage not supported on untrusted origins.: file /Volumes/mozilladev/mozilla-inbound/dom/cache/CacheStorage.cpp, line 173
[5592] WARNING: 'NS_FAILED(mStatus)', file /Volumes/mozilladev/mozilla-inbound/dom/cache/CacheStorage.cpp, line 379

Comment 29

[(no longer active)]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #28)
> It seems that the problem is not on the Fetch API but on the Cache API:
> 
> [5592] WARNING: CacheStorage not supported on untrusted origins.: file
> /Volumes/mozilladev/mozilla-inbound/dom/cache/CacheStorage.cpp, line 173
> [5592] WARNING: 'NS_FAILED(mStatus)', file
> /Volumes/mozilladev/mozilla-inbound/dom/cache/CacheStorage.cpp, line 379

Bug 1243849 should fix this.

Comment 30

[(no longer active)]

I'm planning to look at comment 25 tomorrow.  Sorry, too many fires to fight today.