1390346 - Redirects to moz-extension:-URLs fail when loaded from a xpi, but succeed when extension is unpacked

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect)

Description

[Rob Wu [:robwu]]

Attachment: webrequest_redirect_to_extension_url-1-an+fx.xpi

Tested with Firefox 56.0b2 and 57.0a1 (2017-08-14), e10s enabled.

Bug 1380186 claims to add support for redirecting to moz-extension URLs to Firefox 56. However, there are a few examples where the requested resource fails to load. This happens when the extension is loaded from a xpi file (unpacked mode is somehow not affected), and when e10s is enabled.

Steps to reproduce:
1. Download the attached xpi file (webrequest_redirect_to_extension_url-1-an+fx.xpi)
2. Click on the button in the opened page to start the test.


The extension does the following:
1. Visit example.com.
2. Load a script resource (visible in console of new tab).
3. Load a script resource.
4. Load a style resource (visible in document of new tab).
5. Load a style resource.
6. Load a frame resource (visible in document of new tab).
7. Load a frame resource.


Expected:
- The test page should report "SUCCESS" for every test step. This indicates that the redirection to a moz-extension:-URL has succeeded.

(the expected result happens when the extension is loaded in unpacked mode. To test unpacked mode, download unpacked.zip, extract it to a directory and load it at about:debugging.)


Actual, when the extension is installed from an xpi:
- step 3 fails (the first script loads successfully, but any later scripts fail)
- step 6 and 7 fail (subframe should load but they do not)

(step 4 and 5 both pass, so apparently there is no issue in redirecting style sheets)



Additional information:
- This bug only happens when e10s is enabled. The tests pass in Firefox 56 and 57 on desktop when e10s is disabled (browser.tabs.remote.autostart.2 = false).
- On Android, e10s is disabled by default, so Android is not affected.

Comment 1

[Rob Wu [:robwu]]

Attachment: unpacked.zip

The source code of the signed xpi, and also the code that should be used when you want to simultaneously test the xpi and unpacked extension in one browser profile (the xpi has a fixed extension ID, the unpacked extension does not, so they can both be loaded at the same time).

The previously attached xpi is generated by running "make" and uploading the generated xpi to the developer dashboard of AMO.

These test cases were created after reducing a bug report by Decentraleyes. This bug is a blocker for their migration to the WebExtensions API.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Rather than testing with or without e10s, you can just use

extensions.webextensions.remote

I'm able to reproduce with remote=true, unable to reproduce with remote=false.

I spent some time looking at this last night and this morning.  I think the tests are overly complex requiring time to understand the test itself and making it difficult to isolate issues.  For example, only testing the subframe case results in the first subframe working and the second failing, much like the script test.  If I move the css to load first, then the subframes, then the js, only the css works (no js or subframes).  Of note is that css files are handled differently than others in the extension protocol.

I'm leaning (a little) towards this not being a redirect bug, but rather a protocol bug, or at potentially a combination issue.  CSS is handled different[1] than anything else in the extension protocol, and it's passing.

To explain for others the repro (must be in XPI, unpacked works fine):

load a page from a domain.
using tab.executeScript, insert a script tag with src to a fake file on the domain.
using webrequest.onBeforeRequest listener, redirect the fake script to a script in the extension
do that twice, the second request fails

Do the same with an iframe rather than a script tag.

Lets see if kmag or haik has any thoughts on this.

[1] http://searchfox.org/mozilla-central/source/netwerk/protocol/res/ExtensionProtocolHandler.cpp#466

Comment 3

[Shane Caraveo (:mixedpuppy)]

Another note, I did a some webrequest logging.

With a CSS file, I get onBeforeRequest->onBeforeRedirect->onBeforeRequest->onCompleted

With script or subframe I only get onBeforeRequest->onBeforeRedirect, nothing through content policy or http-on-modify-request.

Comment 4

[Rob Wu [:robwu]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> Rather than testing with or without e10s, you can just use
> 
> extensions.webextensions.remote
> 
> I'm able to reproduce with remote=true, unable to reproduce with
> remote=false.

I tested on Linux, where this flag is false by default.
Flipping the flag to true does not change the test output for me in Firefox 57 (2017-08-14) on Linux.

I do see another pref, extensions.webextensions.protocol.remote, defaulting to true.
If I set that flag to false, then all script/frame requests fail.


> I spent some time looking at this last night and this morning.  I think the
> tests are overly complex requiring time to understand the test itself and
> making it difficult to isolate issues.

To manually test the issue:
1. Install webrequest_redirect_to_extension_url-1-an+fx.xpi
2. To test, open a NEW example.com tab and run any combination of the following in the JS console:
* To test scripts:
        var s = document.createElement('script');
        s.src = '/intercept_installed' + Date.now(); // cache bust to rule out caching.
        s.onload = () => alert('loaded');
        s.onerror = () => alert('error');
        document.body.appendChild(s);

* To test style sheets:
        var s = document.createElement('link');
        s.rel = 'stylesheet';
        s.href = '/intercept_installed' + Date.now(); // cache bust to rule out caching.
        s.onload = () => alert('loaded');
        s.onerror = () => alert('error');
        document.body.appendChild(s);

* To test subframes:
        var s = document.createElement('iframe');
        s.src = '/intercept_installed' + Date.now(); // cache bust to rule out caching.
        document.documentElement.appendChild(s);
        // TEST: Check whether the frame is loaded (some text) or not (stays empty).



> For example, only testing the
> subframe case results in the first subframe working and the second failing,
> much like the script test.  If I move the css to load first, then the
> subframes, then the js, only the css works (no js or subframes).

I can confirm this. So it seems that for this bug, there is no difference between the resource being requested as a script or a frame.

I also found that this bug persists until the tab is closed.
Reloading the page (even with Ctrl-F5) does not help.
Opening a new tab does allow the resource to be redirected once, but after that the bug shows up again.

Comment 5

[Rob Wu [:robwu]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> I'm leaning (a little) towards this not being a redirect bug, but rather a
> protocol bug, or at potentially a combination issue.

If I directly load the resource (without redirects), then the request always succeeds.

Repeat step 1 and 2 from my previous comment (comment 4) and repeat the following snippet multiple times. The shown frame will always be non-blank (don't forget to replace the UUID in the URL with the UUID of the extension in your local installation.
        var s = document.createElement('iframe');
        s.src = 'moz-extension://66612fcb-6583-4908-9f01-d6690f923abf/resources/dummy.js';
        document.documentElement.appendChild(s);
        // TEST: Check whether the frame is loaded (some text) or not (stays empty).

But if I then use similar code with a redirect (repeat the STR from comment 4 with the frame test), then the request will fail.

So it seems that loading the resource ONCE (directly or through a redirect, does not matter) will prevent the resource from being loaded again through a redirect.

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to Rob Wu [:robwu] from comment #4)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> > Rather than testing with or without e10s, you can just use
> > 
> > extensions.webextensions.remote
> > 
> > I'm able to reproduce with remote=true, unable to reproduce with
> > remote=false.
> 
> I tested on Linux, where this flag is false by default.
> Flipping the flag to true does not change the test output for me in Firefox
> 57 (2017-08-14) on Linux.
> 
> I do see another pref, extensions.webextensions.protocol.remote, defaulting
> to true.
> If I set that flag to false, then all script/frame requests fail.

Do you have .remote=true and .protocol.remote=false?  I'd suspect total failure at that point as well, especially for redirects.

I'm suspecting this change:

https://reviewboard.mozilla.org/r/158200/diff/3#index_header

In substitutechannel, if extensions.webextensions.protocol.remote (which I suspect is actually now necessary if webextensions.remote=true), we call SubstituteRemoteChannel.  That calls either SubstituteRemoteFileChannel or SubstituteRemoteJarChannel depending on whether the url is jar'd.  The File version *always* calls NewSimpleChannel.  The Jar version *only* calls NewSimpleChannel on the *first* access, after which it is a cache hit and it never creates the new channel.  CSS is immune to that because it is handled separately and calls NewSimpleChannel itself.  I beleive that call is necessary in order to have a SimpleChannelParent which is required for a redirect to work when webextensions.remote=true.

Comment 7

[Synzvato]

I can confirm that disabling e10s hides the issue. On Linux, the preference extensions.webextensions.remote is indeed disabled by default, and it (at least on my side) does not appear to effect reproducibility. The best, temporary, workaround I can think of is setting extensions.alwaysUnpack to true before installing an affected extension.

Comment 8

[Haik Aftandilian [:haik]]

Talked with Shane on IRC. It sounds like the cached JAR handling in ExtensionProtocolHandler should also use SimpleChannel to let the forwarding work correctly.

Comment 9

[Rob Wu [:robwu]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> (In reply to Rob Wu [:robwu] from comment #4)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> > > Rather than testing with or without e10s, you can just use
> > > 
> > > extensions.webextensions.remote
> > > 
> > > I'm able to reproduce with remote=true, unable to reproduce with
> > > remote=false.
> > 
> > I tested on Linux, where this flag is false by default.
> > Flipping the flag to true does not change the test output for me in Firefox
> > 57 (2017-08-14) on Linux.
> > 
> > I do see another pref, extensions.webextensions.protocol.remote, defaulting
> > to true.
> > If I set that flag to false, then all script/frame requests fail.
> 
> Do you have .remote=true and .protocol.remote=false?  I'd suspect total
> failure at that point as well, especially for redirects.

My initial report is with default values for Firefox Nightly 57 on Linux, i.e
extensions.webextensions.remote = false
extensions.webextensions.protocol.remote = true

When I turn .protocol.remote to false, then all resource redirects fail (regardless of the .remote setting), but normal requests still go through.



Cached jars are indeed seemingly causing issues.
When I force the JAR file to be evicted from the cache, then the redirect succeeds again (once).

To evict the jar,
1. Vist about:debugging and turn on add-on debugging (to enable Chrome debugging).
2. Open the console for the content process that hosts example.com: Tools > Browser Content Toolbox.
2. Run the following code (replace "/tmp/nightlyprof3" with the actual location of your profile directory):

Components.classes['@mozilla.org/observer-service;1']
.getService(Components.interfaces.nsIObserverService)
.notifyObservers(null, 'flush-cache-entry', '/tmp/nightlyprof3/extensions/[e-mail]')

Comment 10

[Shane Caraveo (:mixedpuppy)]

I've also verified that removing the early return in SubstituteRemoteJarChannel fixes the redirects.

Comment 11

[Haik Aftandilian [:haik]]

Attachment: Bug 1390346 - Redirects to moz-extension:-URLs fail when loaded from a xpi.

Review commit: https://reviewboard.mozilla.org/r/169510/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169510/

Comment 12

[Haik Aftandilian [:haik]]

The posted fix changes the way a moz-extension URI is handled when it's for a packed extension (JAR/XPI file) and the JAR file is cached. Now, the cached JAR case is handled using SimpleChannel similarly to to the un-cached case. I tested using the provided webrequest_redirect_to_extension_url-1-an+fx.xpi and the tests appeared to pass with the fix. Thanks for providing the test code.

Linux try push:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=93d46e8d20a23ad95b37701142b2386151da83ca

Windows/Mac try push:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=712dba222ad97e75d5e48f08ffcf421eb9b3dff8

:robwu, would you be able to verify that the fix works for you?

Linux x64 build: https://queue.taskcluster.net/v1/task/Z1ChqmiJR1-qucEWFGpFtg/runs/0/artifacts/public/build/target.tar.bz2

Comment 13

[Rob Wu [:robwu]]

(In reply to Haik Aftandilian [:haik] from comment #12)
> :robwu, would you be able to verify that the fix works for you?
> 
> Linux x64 build:
> https://queue.taskcluster.net/v1/task/Z1ChqmiJR1-qucEWFGpFtg/runs/0/
> artifacts/public/build/target.tar.bz2

I can confirm that the test is fixed.
Yep, I run the xpi, for extensions.webextensions.remote = false (default) and true,

Can you also add a regression test too, to ensure that this flow does not break again?
My existing automated test can reasonably easily be converted to a webextension test (but let me know if that takes too much effort, then I can separately submit a patch).

Comment 14

[Shane Caraveo (:mixedpuppy)]

Lets use an xpcshell test based on 

toolkit/components/extensions/test/xpcshell/test_ext_redirects.js:test_content_channel_redirect_to_extension 

but using an xpi via Extension.generateXPI as is done in test_ext_expirements.js.

Comment 15

[Shane Caraveo (:mixedpuppy)]

I'm working on a test.

Comment 16

[Haik Aftandilian [:haik]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> I'm working on a test.

Thanks, Shane. I'll work on getting this in Nightly. Will you use this bug or another bug for the test?

Comment 17

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1390346 test jar caching in combination with redirects,

Review commit: https://reviewboard.mozilla.org/r/169832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169832/

Comment 18

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8898476 [details]
Bug 1390346 test jar caching in combination with redirects,

or Rob

Comment 19

[Rob Wu [:robwu]]

Comment on attachment 8898476 [details]
Bug 1390346 test jar caching in combination with redirects,

https://reviewboard.mozilla.org/r/169832/#review175094

This is a very precise regression test. If you are confident that the resource type does not matter, then this is sufficient. Otherwise, could you add more tests so that we have test coverage for the "redirect to moz-extension"? (this is not required for this bug, so if you don't want to, feel free to open a new bug and assign it to me).

::: toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html:31
(Diff revision 1)
> +        "webRequest",
> +        "webRequestBlocking",
> +        "<all_urls>",
> +      ],
> +      "web_accessible_resources": [
> +          "finished.html",

Nit: Inconsistent spacing in this JSON. 2 and 4-space indentation is mixed.

::: toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html:53
(Diff revision 1)
> +    background: async () => {
> +      browser.webRequest.onBeforeRequest.addListener(details => {
> +        if (details.url.includes("intercept")) {
> +          return {redirectUrl: browser.extension.getURL("finished.html")};
> +        }
> +      }, {urls: ["<all_urls>"]}, ["blocking"]);

If you replace `<all_urls>` with `"*://*/intercept*"`, then you don't need the `details.url.includes` check.

::: toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html:64
(Diff revision 1)
> +            s.src = "/intercept?r=" + Math.random();
> +            s.onload = () => resolve('loaded');
> +            s.onerror = () => resolve('error');
> +            document.documentElement.appendChild(s);
> +            setTimeout(() => resolve('timed_out'), 2000);
> +          });`,

Can you verify that the expected redirect happened? In the current code, the test will still pass if the background page is removed.

For example, by having finished.html use `parent.postMessage('finished', '*');`, and before inserting the frame, add a `window.onmessage` listener. In this listener, assert that `event.source === s.contentWindow` and also `event.origin === browser.extension.getURL('finished.html')`.

Comment 20

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8898476 [details]
Bug 1390346 test jar caching in combination with redirects,

https://reviewboard.mozilla.org/r/169832/#review175084

Hi Shane,
this new test looks good, most of the comments below are related to small nits (the one related to the indentation of the manifest properties is probably going to make eslint to fail), but I've also a couple of question/proposed additions for the test plan.

In particular, let me know what you think about the following three:
- an explicit assertion to ensure that the expected extension url has been loaded
- should we run this test with the "extensions.webextensions.protocol.remote" pref explicitly set to true and false?
- is there anything preventing this test to be an xpcshell test?

::: toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html:1
(Diff revision 1)
> +<!DOCTYPE HTML>

nit, how about prefixing the filename with `test_ext_webrequest_*` so that it is grouped with the other webrequest test files?

also, is there anything preventing this test to be an xpcshell test?

::: toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html:31
(Diff revision 1)
> +        "webRequest",
> +        "webRequestBlocking",
> +        "<all_urls>",
> +      ],
> +      "web_accessible_resources": [
> +          "finished.html",

this line is indented with 3 spaces instead of 2 (some for the lines between 21 and 24.

::: toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html:46
(Diff revision 1)
> +          </head>
> +          <body>
> +            <h1>redirected!</h1>
> +          </body>
> +        </html>
> +      `.trim(),

nit, is `.trim()` necessary here?

::: toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html:57
(Diff revision 1)
> +        }
> +      }, {urls: ["<all_urls>"]}, ["blocking"]);
> +
> +      async function testSubFrameResource(tabId, code) {
> +        let [result] = await browser.tabs.executeScript(tabId, {code: `
> +          new Promise((resolve, reject) => {

nit, `reject` seems to be unused here

::: toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html:60
(Diff revision 1)
> +      async function testSubFrameResource(tabId, code) {
> +        let [result] = await browser.tabs.executeScript(tabId, {code: `
> +          new Promise((resolve, reject) => {
> +            var s = document.createElement('iframe');
> +            s.src = "/intercept?r=" + Math.random();
> +            s.onload = () => resolve('loaded');

I think that it would be great to also add an explicitly assertion that the expected extension page has been actually loaded in the iframe (e.g.: using something like `browser.test.assertEq(browser.runtime.getURL("finished.html"), s.contentWindow.location.href);` inside the arrow function assigned to the `s.onload` property, or even better by using the `browser.test.sendMessage` method inside the loaded extension page and two `await extension.awaitMessage` in the test code)

::: toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html:61
(Diff revision 1)
> +        let [result] = await browser.tabs.executeScript(tabId, {code: `
> +          new Promise((resolve, reject) => {
> +            var s = document.createElement('iframe');
> +            s.src = "/intercept?r=" + Math.random();
> +            s.onload = () => resolve('loaded');
> +            s.onerror = () => resolve('error');

nit, is onerror ever fired by the iframe element?

iirc in the w3c spec all the embed elements (e.g. like iframe and img) have to provide an onerror property, but only for img the error event is explicitly mentioned in the spec, while iframe doesn't have such explicit requirement (and so I'm wondering if the error event is actually fired for them on Firefox).

::: toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html:74
(Diff revision 1)
> +      let tab = await browser.tabs.create({url: "http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_sample.html"});
> +      browser.test.assertEq("loaded", await testSubFrameResource(tab.id), "frame 1 loaded");
> +      // Bug 1390346 If jar caching breaks redirects, this next test will fail.
> +      browser.test.assertEq("loaded", await testSubFrameResource(tab.id), "frame 2 loaded");
> +      await browser.tabs.remove(tab.id);
> +      browser.test.sendMessage("requestsCompleted");

nit, or `browser.test.notifyPass("...);` here and `await extension.awaitFinish("...");` at line 82.

::: toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html:80
(Diff revision 1)
> +    },
> +  });
> +}
> +
> +add_task(async function test_redirect_to_jar() {
> +  let extension = getExtension();

Based on the comments on the bugzilla issue, it seems that the redirection was working differently based on the kind of the resource loaded (e.g. css files, script and html files), and so I agree with Rob that we should probably add more test cases to better cover this from regressing in the future (but I also agree that we can opt to do it in a follow up)

::: toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html:80
(Diff revision 1)
> +    },
> +  });
> +}
> +
> +add_task(async function test_redirect_to_jar() {
> +  let extension = getExtension();

I'm wondering if it would be better to use `await SpecialPowers.pushPrefEnv({"set": [["extensions.webextensions.protocol.remote", ...]]});` to test this with the protocol remote explicitly set to both true and false across the platforms.

Comment 21

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8898476 [details]
Bug 1390346 test jar caching in combination with redirects,

https://reviewboard.mozilla.org/r/169832/#review175084

> this line is indented with 3 spaces instead of 2 (some for the lines between 21 and 24.

sorry typo in the review comment: s/some for the lines between/same for the lines between/

Comment 22

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8898476 [details]
Bug 1390346 test jar caching in combination with redirects,

https://reviewboard.mozilla.org/r/169832/#review175084

I originally tried this as an xpcshell test and run into issues getting everything working.  I want to get this done quickly so this is more expedient.  If we werent on a short road to 57 I might delay to figure out the issues.

I think that the extensions.webextensions.protocol.remote pref is broken in this way:

webextensions.protocol.remote=true should always work
webextensions.protocol.remote=false + webextensions.remote=true should NOT work

Basically, that pref should be removed and the code should work as if that is always true.

> nit, or `browser.test.notifyPass("...);` here and `await extension.awaitFinish("...");` at line 82.

Not sure I like the readability of that...since the tests above could fail it's not really a pass.

> Based on the comments on the bugzilla issue, it seems that the redirection was working differently based on the kind of the resource loaded (e.g. css files, script and html files), and so I agree with Rob that we should probably add more test cases to better cover this from regressing in the future (but I also agree that we can opt to do it in a follow up)

I don't think we need them.  CSS bypasses the problem, but any other type doesn't.

> I'm wondering if it would be better to use `await SpecialPowers.pushPrefEnv({"set": [["extensions.webextensions.protocol.remote", ...]]});` to test this with the protocol remote explicitly set to both true and false across the platforms.

I think the pref needs to be removed, I don't actually beleive it works in all combinations due the requirement to have the parent/child channel.  Setting it to false would just break redirects in general.

Comment 23

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8898476 [details]
Bug 1390346 test jar caching in combination with redirects,

https://reviewboard.mozilla.org/r/169832/#review175094

The resource type does not matter.  Only CSS is different, and bypasses this issue entirely.  If that were to change in the future, it would be to make css the same as everything else.  Since we localize css files, I don't see that happening.

Comment 24

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8898476 [details]
Bug 1390346 test jar caching in combination with redirects,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169832/diff/1-2/

Comment 25

[Haik Aftandilian [:haik]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #22)
> I think that the extensions.webextensions.protocol.remote pref is broken in
> this way:
> 
> webextensions.protocol.remote=true should always work
> webextensions.protocol.remote=false + webextensions.remote=true should NOT
> work
> 
> Basically, that pref should be removed and the code should work as if that
> is always true.

Yes, extensions.webextensions.protocol.remote=true is required for extensions to work now that file system read access sandboxing is enabled for content processes and the extension process. Setting it to false will require also setting sandboxing prefs to disable read access sandboxing.

In some cases, extensions.webextensions.protocol.remote=false will not break moz-extension loads if they're from the parent process due to the extension process being disabled with extensions.webextensions.remote=false.

The pref extensions.webextensions.protocol.remote was only added to aid debugging and provide a fallback shortly after moz-extension remoting was landed. If it's causing confusion, we could remove it. It has been helpful for debugging in a few instances.

Comment 26

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8898476 [details]
Bug 1390346 test jar caching in combination with redirects,

https://reviewboard.mozilla.org/r/169830/#review175442

Thanks for the additional details, it sounds good to me.

r=me (with the small typo fixed and with try green also on android, otherwise we can skip it on android in the meantime or make it able to pass)

::: toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html:75
(Diff revision 2)
> +      browser.test.assertEq("loaded", result[0], "frame 1 loaded");
> +      browser.test.assertEq(redirectUrl, result[1], "frame 1 redirected");
> +      // Bug 1390346 If jar caching breaks redirects, this next test will fail.
> +      result = await testSubFrameResource(tab.id)
> +      browser.test.assertEq("loaded", result[0], "frame 2 loaded");
> +      browser.test.assertEq(redirectUrl, result[1], "frame 1 redirected");

this message should be "frame 2 redirected"

Comment 27

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8898476 [details]
Bug 1390346 test jar caching in combination with redirects,

https://reviewboard.mozilla.org/r/169832/#review175438

Comment 28

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8898476 [details]
Bug 1390346 test jar caching in combination with redirects,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169832/diff/2-3/

Comment 29

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8898476 [details]
Bug 1390346 test jar caching in combination with redirects,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169832/diff/3-4/

Comment 30

[Haik Aftandilian [:haik]]

Comment on attachment 8898152 [details]
Bug 1390346 - Redirects to moz-extension:-URLs fail when loaded from a xpi.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169510/diff/1-2/

Comment 31

[Haik Aftandilian [:haik]]

Comment on attachment 8898152 [details]
Bug 1390346 - Redirects to moz-extension:-URLs fail when loaded from a xpi.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169510/diff/2-3/

Comment 32

[Jim Mathies [:jimm]]

Comment on attachment 8898152 [details]
Bug 1390346 - Redirects to moz-extension:-URLs fail when loaded from a xpi.

https://reviewboard.mozilla.org/r/169510/#review175516

::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:412
(Diff revision 3)
> +  // debugging purposes only. With process-level sandboxing, child
> +  // processes (specifically content and extension processes), will
> +  // not be able to load most moz-extension URI's when the pref is
> +  // set to false.
>    mUseRemoteFileChannels = IsNeckoChild() &&
>      Preferences::GetBool("extensions.webextensions.protocol.remote");

Does this pref serve a purpose? maybe we should get rid of it?

Comment 33

[Haik Aftandilian [:haik]]

Comment on attachment 8898152 [details]
Bug 1390346 - Redirects to moz-extension:-URLs fail when loaded from a xpi.

https://reviewboard.mozilla.org/r/169510/#review175516

> Does this pref serve a purpose? maybe we should get rid of it?

My preference would be to keep it for now because it's useful for debugging.

Beyond debugging, it doesn't serve a purpose. And now that we've landed filesystem read access sandboxing, setting the pref to false requires flipping some sandbox prefs to get working extensions. That said, I've found it useful a few times while debugging extension issues.

Comment 34

[Jim Mathies [:jimm]]

Comment on attachment 8898152 [details]
Bug 1390346 - Redirects to moz-extension:-URLs fail when loaded from a xpi.

https://reviewboard.mozilla.org/r/169510/#review176466

Comment 35

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=637c18157b45169dccce23a523323d4f69d71503

Comment 36

[Shane Caraveo (:mixedpuppy)]

both patches need checkin.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9878f96c556e
Redirects to moz-extension:-URLs fail when loaded from a xpi. r=jimm

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86bdc742c111
test jar caching in combination with redirects, r=rpl

Comment 39

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

https://hg.mozilla.org/mozilla-central/rev/9878f96c556e
https://hg.mozilla.org/mozilla-central/rev/86bdc742c111

Comment 40

[Rob Wu [:robwu]]

Can these patched be uplifted to Firefox 56?
Otherwise the redirect-to-moz-extension-URL feature that was introduced in Firefox 56 is of very limited use (and also in an unexplicable, hard-to-debug way for those who aren't aware of the bug).

Comment 41

[Haik Aftandilian [:haik]]

(In reply to Rob Wu [:robwu] from comment #40)
> Can these patched be uplifted to Firefox 56?
> Otherwise the redirect-to-moz-extension-URL feature that was introduced in
> Firefox 56 is of very limited use (and also in an unexplicable,
> hard-to-debug way for those who aren't aware of the bug).

Yes, we can get this uplifted to 56. Let's make sure bug 1393402 isn't a bug in the code we need to fix first.

Comment 42

[Rob Wu [:robwu]]

(clearing NI; answered in comment 41)

Comment 43

[Shane Caraveo (:mixedpuppy)]

The intermittent looks like it's linux debug :(  bumped the timeout a bit to see if that deals with it.

Comment 44

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8898152 [details]
Bug 1390346 - Redirects to moz-extension:-URLs fail when loaded from a xpi.

Approval Request Comment
[Feature/Bug causing the regression]: 1376496
[User impact if declined]: Potential failure to load jar/cached extension urls
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: test patch also in this bug, possibly bump the timeout per bug 1393402
[Is the change risky?]: moderate
[Why is the change risky/not risky?]: the affect here is limited to extensions running out of process.
[String changes made/needed]: none

Comment 45

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8898152 [details]
Bug 1390346 - Redirects to moz-extension:-URLs fail when loaded from a xpi.

Fix for url redirects from extensions. Please land for beta 7.

Comment 46

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8898476 [details]
Bug 1390346 test jar caching in combination with redirects,

Tests should also land on beta.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e9c83e49ec46
https://hg.mozilla.org/releases/mozilla-beta/rev/2968e01b6f20

Comment 48

[Andrei Vaida [:avaida]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #44)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Shane's assessment on manual testing needs and the fact that this fix has automated coverage.