1161684 - Allow JAR channels to be intercepted by service workers

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 1

[Josh Matthews [:jdm]]

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.

Comment 2

[Andrew Overholt [:overholt]]

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

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: Trigger fetch event for JAR requests

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

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.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Sorry, I forgot to add some files

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Tests

Comment 10

[Josh Matthews [:jdm]]

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.

Comment 11

[Pulsebot]

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

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 14

[Pulsebot]

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

Comment 15

[Phil Ringnalda (:philor)]

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

Comment 16

[(no longer active)]

Fernando, are you planning to backport this to Aurora?

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

(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?

Comment 18

[(no longer active)]

(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!