Allow JAR channels to be intercepted by service workers

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: ferjm, Assigned: ferjm)

Tracking

unspecified
mozilla41
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [s2])

Attachments

(2 attachments, 4 obsolete attachments)

We need to allow service workers to intercept app:// requests the same way that we allow them to intercept http(s):// ones.
Depends on: 1147214
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.
Assignee: nobody → ferjmoreno
Depends on: 1161288
Whiteboard: [s1]
Fernando, please let jdm know if you need his help here.
Flags: needinfo?(ferjmoreno)
Thank you Andrew. He already gave me some good pointers and I'll probably keep bugging him :)
Flags: needinfo?(ferjmoreno)
Posted patch WIP (obsolete) — Splinter Review
Depends on: 1164100
Attachment #8604166 - Attachment is obsolete: true
Attachment #8604680 - Attachment is obsolete: true
Posted patch WIP (obsolete) — Splinter Review
Posted patch v1 (obsolete) — Splinter Review
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.
Attachment #8605909 - Attachment is obsolete: true
Attachment #8606342 - Flags: feedback?(josh)
Posted patch v1Splinter Review
Sorry, I forgot to add some files
Attachment #8606342 - Attachment is obsolete: true
Attachment #8606342 - Flags: feedback?(josh)
Attachment #8606351 - Flags: feedback?(josh)
Whiteboard: [s1] → [s2]
Posted patch TestsSplinter Review
Attachment #8607039 - Flags: review?(josh)
Attachment #8606351 - Flags: feedback?(josh) → review?(josh)
Attachment #8607039 - Flags: review?(josh) → review+
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.
Attachment #8606351 - Flags: review?(josh) → review+
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/mozilla-central/rev/c9e1fd16de09
https://hg.mozilla.org/mozilla-central/rev/b58ff5fd82dd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Fernando, are you planning to backport this to Aurora?
Flags: needinfo?(ferjmoreno)
Blocks: 1168208
(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?
Flags: needinfo?(ferjmoreno)
(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!
Component: DOM → DOM: Service Workers
You need to log in before you can comment on or make changes to this bug.