remove app:// protocol support from service worker interception

RESOLVED FIXED in Firefox 47

Status

()

Core
DOM: Service Workers
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bkelly, Assigned: Away for a while)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
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.
Assignee: nobody → ehsan
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1193319
(Assignee)

Comment 3

2 years ago
Created attachment 8711788 [details] [diff] [review]
Remove support for intercepting app:// URIs using service workers
Attachment #8711788 - Flags: review?(josh)
(Assignee)

Updated

2 years ago
Blocks: 1222008
(Assignee)

Comment 4

2 years ago
Created attachment 8712674 [details] [diff] [review]
Remove support for intercepting app:// URIs using service workers

Found some more code that we can delete!
Attachment #8712674 - Flags: review?(josh)
(Assignee)

Updated

2 years ago
Attachment #8711788 - Attachment is obsolete: true
Attachment #8711788 - Flags: review?(josh)

Updated

2 years ago
Attachment #8712674 - Flags: review?(josh) → review+

Comment 5

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b46a851ba158
Depends on: 1243670

Updated

2 years ago
Depends on: 1243671

Comment 6

2 years ago
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.
Flags: needinfo?(ehsan)

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b46a851ba158
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b46a851ba158
So now that the music app is completely broken, as the tests show, where do we go?

(bug 1243670 has been filed)
Flags: needinfo?(bkelly)
(Reporter)

Comment 10

2 years ago
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
Flags: needinfo?(bkelly)
Comment hidden (off-topic)

Updated

2 years ago
Depends on: 1233670
(Reporter)

Comment 12

2 years ago
Are the tests failing due to app:// support being removed?  Or the issue raised in comment 6 about http:// apps?
Flags: needinfo?(bkelly)
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

2 years ago
(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

2 years ago
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 hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)
Comment hidden (off-topic)

Updated

2 years ago
Depends on: 1243819
(Assignee)

Comment 22

2 years ago
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!  :-)
No longer depends on: 1243819
Flags: needinfo?(ehsan)
(Assignee)

Updated

2 years ago
Flags: needinfo?(overholt)
(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.
(Assignee)

Comment 24

2 years ago
(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

2 years ago
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

2 years ago
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
Flags: needinfo?(jdarcangelo)

Updated

2 years ago
No longer depends on: 1233670
(Assignee)

Comment 27

2 years ago
(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

2 years ago
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
Flags: needinfo?(jdarcangelo)
(Assignee)

Updated

2 years ago
Depends on: 1243849
(Assignee)

Comment 29

2 years ago
(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.
(Assignee)

Comment 30

2 years ago
I'm planning to look at comment 25 tomorrow.  Sorry, too many fires to fight today.
(Assignee)

Updated

2 years ago
Flags: needinfo?(ehsan)
(Assignee)

Updated

a year ago
Flags: needinfo?(ehsan)
(Assignee)

Updated

a year ago
See Also: → bug 1308075
You need to log in before you can comment on or make changes to this bug.