Closed Bug 1197379 Opened 4 years ago Closed 4 years ago

remove app:// protocol support from service worker interception

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: bkelly, Assigned: ehsan)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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
Duplicate of this bug: 1193319
Blocks: 1222008
Found some more code that we can delete!
Attachment #8712674 - Flags: review?(josh)
Attachment #8711788 - Attachment is obsolete: true
Attachment #8711788 - Flags: review?(josh)
Attachment #8712674 - Flags: review?(josh) → review+
Depends on: 1243670
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)
https://hg.mozilla.org/mozilla-central/rev/b46a851ba158
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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)
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)
Depends on: 1233670
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.
(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.
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.
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)
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.
(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 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.
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)
No longer depends on: 1233670
(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.
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)
Depends on: 1243849
(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.
I'm planning to look at comment 25 tomorrow.  Sorry, too many fires to fight today.
Flags: needinfo?(ehsan)
Flags: needinfo?(ehsan)
See Also: → 1308075
You need to log in before you can comment on or make changes to this bug.