We need to allow service workers to intercept app:// requests the same way that we allow them to intercept http(s):// ones.

There are a few pieces here: * split nsJARChannel::AsyncOpen into two methods, where the second one involves calling LookupFile and everything following, and the first one does something similar to HttpBaseChannel::ShouldIntercept and nsHttpChannel::OpenCacheEntry (see the creation of InterceptedChannelChrome) * either make InterceptedChannelChrome/Content support JAR channels, or make a similar class that is strictly for JAR channels and implements nsIInterceptedChannel * implement the hooks necessary in nsJARChannel to synthesize a response or continue with the interrupted AsyncOpen operation I don't think we would need to have any special behaviour for e10s, which is nice.

Fernando, please let jdm know if you need his help here.

Thank you Andrew. He already gave me some good pointers and I'll probably keep bugging him :)

Josh, I still need to add some tests here but I would really appreciate some feedback while I work on them. With this patch I can finally intercept and synthesize responses for app:// requests. Thanks in advance for your feedback.

Sorry, I forgot to add some files

Comment on attachment 8606351 [details] [diff] [review] v1 Review of attachment 8606351 [details] [diff] [review]: ----------------------------------------------------------------- This looks really good to me! ::: modules/libjar/nsInterceptedJARChannel.cpp @@ +3,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "nsInterceptedJARChannel.h" Please rename this file to match the class name, ie. remove the ns prefix. @@ +7,5 @@ > +#include "nsInterceptedJARChannel.h" > +#include "nsIPipe.h" > + > +namespace mozilla { > +namespace net { Just `using namespace mozilla::net` instead. @@ +28,5 @@ > + > +NS_IMETHODIMP > +InterceptedJARChannel::GetIsNavigation(bool* aIsNavigation) > +{ > + *aIsNavigation = true; Do we ever deal with app:// requests that are not for navigation? ::: modules/libjar/nsInterceptedJARChannel.h @@ +25,5 @@ > + > +// An object representing a channel that has been intercepted. This avoids > +// complicating the actual channel implementation with the details of > +// synthesizing responses. > +class InterceptedJARChannel : public nsIInterceptedChannel Please rename this file to match the class name (ie. remove the ns prefix). ::: modules/libjar/nsJARChannel.cpp @@ +874,5 @@ > + NS_GET_IID(nsINetworkInterceptController), > + getter_AddRefs(controller)); > + bool shouldIntercept = false; > + if (controller) { > + bool isNavigation = mLoadFlags & LOAD_DOCUMENT_URI; It looks like this value should be copied to InterceptedJARChannel. ::: modules/libjar/nsJARChannel.h @@ +128,5 @@ > nsCString mJarEntry; > nsCString mInnerJarEntry; > + > + nsRefPtr<nsInputStreamPump> mSynthesizedResponsePump; > + int64_t mSynthesizedStreamLength; This should be initialized in the constructor.

https://hg.mozilla.org/integration/mozilla-inbound/rev/068cc6419d1e https://hg.mozilla.org/integration/mozilla-inbound/rev/f938930b3ac5

The pushed caused a perma-failing M4 for opt builds: 00:56:04 INFO - 1647 INFO TEST-START | dom/workers/test/serviceworkers/test_app_protocol.html 00:56:04 INFO - -*- Webapps.jsm : at install package got app etag=null 00:56:04 INFO - -*- Webapps.jsm : confirmInstall 00:56:04 INFO - -*- Webapps.jsm : _writeManifestFile 00:56:04 INFO - -*- Webapps.jsm : Saving /tmp/tmpXNWJPX.mozrunner/webapps/{2cd23633-676d-4ecb-a1fe-7adf49bf3eae}/update.webapp 00:56:04 INFO - -*- Webapps.jsm : Success saving /tmp/tmpXNWJPX.mozrunner/webapps/{2cd23633-676d-4ecb-a1fe-7adf49bf3eae}/update.webapp 00:56:04 INFO - -*- Webapps.jsm : app.origin: app://{2cd23633-676d-4ecb-a1fe-7adf49bf3eae} 00:56:04 INFO - -*- Webapps.jsm : Saving /tmp/tmpXNWJPX.mozrunner/webapps/webapps.json 00:56:04 INFO - -*- Webapps.jsm : Success saving /tmp/tmpXNWJPX.mozrunner/webapps/webapps.json 00:56:04 INFO - -*- Webapps.jsm : No deviceStorage 00:56:04 INFO - -*- Webapps.jsm : Free storage: 13241229312. Download size: 0 00:56:04 INFO - -*- Webapps.jsm : About to download http://mochi.test:8888/tests/dom/workers/test/serviceworkers/app-protocol/application.zip 00:56:04 INFO - -*- Webapps.jsm : Saving /tmp/tmpXNWJPX.mozrunner/webapps/webapps.json 00:56:04 INFO - -*- Webapps.jsm : Success saving /tmp/tmpXNWJPX.mozrunner/webapps/webapps.json 00:56:04 INFO - -*- Webapps.jsm : onProgress: 1757/1757 00:56:04 INFO - -*- Webapps.jsm : Saving /tmp/tmpXNWJPX.mozrunner/webapps/webapps.json 00:56:04 INFO - -*- Webapps.jsm : File hash computed: 13610955e00dc197657d6ebde7a028cd 00:56:04 INFO - -*- Webapps.jsm : Success saving /tmp/tmpXNWJPX.mozrunner/webapps/webapps.json 00:56:04 INFO - -*- Webapps.jsm : _onDownloadPackage 00:56:04 INFO - -*- Webapps.jsm : Saving /tmp/tmpXNWJPX.mozrunner/webapps/{2cd23633-676d-4ecb-a1fe-7adf49bf3eae}/manifest.webapp 00:56:04 INFO - -*- Webapps.jsm : Success saving /tmp/tmpXNWJPX.mozrunner/webapps/{2cd23633-676d-4ecb-a1fe-7adf49bf3eae}/manifest.webapp 00:56:04 INFO - -*- Webapps.jsm : Saving /tmp/tmpXNWJPX.mozrunner/webapps/webapps.json 00:56:04 INFO - -*- Webapps.jsm : Success saving /tmp/tmpXNWJPX.mozrunner/webapps/webapps.json 00:56:04 INFO - -*- Webapps.jsm : updateAppHandlers: old=null new=({name:"App", launch_path:"/index.html", description:"Test app for bug 1161684"}) 00:56:04 INFO - -*- Webapps.jsm : Saving /tmp/tmpXNWJPX.mozrunner/webapps/webapps.json 00:56:04 INFO - -*- Webapps.jsm : Success saving /tmp/tmpXNWJPX.mozrunner/webapps/webapps.json 00:56:04 INFO - ############################### browserElementPanningAPZDisabled.js loaded 00:56:04 INFO - ############################### browserElementPanning.js loaded 00:56:04 INFO - ######################## BrowserElementChildPreload.js loaded 00:56:10 INFO - JavaScript error: app://{2cd23633-676d-4ecb-a1fe-7adf49bf3eae}/index.html, line 41: TypeError: registration.active is null 00:58:40 INFO - 1432281520202 Browser.Experiments.Experiments TRACE Experiments #0::updateManifest() 00:58:40 INFO - 1432281520203 Browser.Experiments.Experiments TRACE Experiments #0::_run 00:58:40 INFO - 1432281520204 Browser.Experiments.Experiments TRACE Experiments #0::_main iteration 00:58:40 INFO - 1432281520204 Browser.Experiments.Experiments TRACE Experiments #0::_loadManifest 00:58:40 INFO - 1432281520205 Browser.Experiments.Experiments TRACE Experiments #0::httpGetRequest(http://127.0.0.1:8888/experiments-dummy/manifest) 00:58:40 INFO - 1432281520223 Browser.Experiments.Experiments ERROR Experiments #0::httpGetRequest::onLoad() - Request to http://127.0.0.1:8888/experiments-dummy/manifest returned status 404 00:58:40 INFO - 1432281520224 Browser.Experiments.Experiments ERROR Experiments #0::_loadManifest - failure to fetch/parse manifest (continuing anyway): Error: Experiments - XHR status for http://127.0.0.1:8888/experiments-dummy/manifest is 404 00:58:40 INFO - 1432281520226 Browser.Experiments.Experiments TRACE Experiments #0::_evaluateExperiments 00:58:40 INFO - 1432281520232 Browser.Experiments.Experiments TRACE Experiments #0::_main finished, scheduling next run 01:01:15 INFO - Xlib: extension "RANDR" missing on display ":0". 01:01:16 INFO - TEST-INFO | screentopng: exit 0 01:01:16 INFO - 1648 INFO Setting up 01:01:16 INFO - 1649 INFO TEST-PASS | dom/workers/test/serviceworkers/test_app_protocol.html | app installed 01:01:16 INFO - 1650 INFO TEST-PASS | dom/workers/test/serviceworkers/test_app_protocol.html | app downloaded 01:01:16 INFO - 1651 INFO OK: fetch should resolve 01:01:16 INFO - 1652 INFO TEST-PASS | dom/workers/test/serviceworkers/test_app_protocol.html | Message from app: OK: fetch should resolve 01:01:16 INFO - 1653 INFO OK: status should be 200 01:01:16 INFO - 1654 INFO TEST-PASS | dom/workers/test/serviceworkers/test_app_protocol.html | Message from app: OK: status should be 200 01:01:16 INFO - 1655 INFO OK: statusText should be OK 01:01:16 INFO - 1656 INFO TEST-PASS | dom/workers/test/serviceworkers/test_app_protocol.html | Message from app: OK: statusText should be OK 01:01:16 INFO - 1657 INFO OK: body should match 01:01:16 INFO - 1658 INFO TEST-PASS | dom/workers/test/serviceworkers/test_app_protocol.html | Message from app: OK: body should match 01:01:16 INFO - 1659 INFO OK: service worker registered 01:01:16 INFO - 1660 INFO TEST-PASS | dom/workers/test/serviceworkers/test_app_protocol.html | Message from app: OK: service worker registered 01:01:16 INFO - 1661 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/serviceworkers/test_app_protocol.html | Test timed out. - expected PASS Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/6f16344cdb28 https://hg.mozilla.org/integration/mozilla-inbound/rev/57a6450f1c1c

Sorry, I didn't catch this before cause the try build that I did was with debug only builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4eecc712f12 It seems that claim() is failing on linux e10s. New approach without claim(): https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd8763a1990e

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9e1fd16de09 https://hg.mozilla.org/integration/mozilla-inbound/rev/b58ff5fd82dd

https://hg.mozilla.org/mozilla-central/rev/c9e1fd16de09 https://hg.mozilla.org/mozilla-central/rev/b58ff5fd82dd

Fernando, are you planning to backport this to Aurora?

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #16) > Fernando, are you planning to backport this to Aurora? I wasn't planning to do it, unless it's needed. AFAICT we don't need this on Aurora for B2G as this bug was mostly to allow B2G devs to test and develop with service workers on packaged apps while apps are being moved to hosted. Do you need this on Aurora for any reason?

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #17) > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > comment #16) > > Fernando, are you planning to backport this to Aurora? > > I wasn't planning to do it, unless it's needed. AFAICT we don't need this on > Aurora for B2G as this bug was mostly to allow B2G devs to test and develop > with service workers on packaged apps while apps are being moved to hosted. > > Do you need this on Aurora for any reason? No, I was planning to backport a patch on top of this but then we decided to not do that. Thanks!