Closed
Bug 1390346
Opened 7 years ago
Closed 7 years ago
Redirects to moz-extension:-URLs fail when loaded from a xpi, but succeed when extension is unpacked
Categories
(WebExtensions :: Request Handling, defect)
Tracking
(firefox55 unaffected, firefox56 fixed, firefox57 fixed)
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox55 | --- | unaffected |
firefox56 | --- | fixed |
firefox57 | --- | fixed |
People
(Reporter: robwu, Assigned: haik)
References
Details
Attachments
(4 files)
(deleted),
application/x-xpinstall
|
Details | |
(deleted),
application/zip
|
Details | |
(deleted),
text/x-review-board-request
|
jimm
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-review-board-request
|
rpl
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
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.
Reporter | ||
Comment 1•7 years ago
|
||
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•7 years ago
|
||
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
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(haftandilian)
Comment 3•7 years ago
|
||
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.
Reporter | ||
Comment 4•7 years ago
|
||
(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.
Reporter | ||
Comment 5•7 years ago
|
||
(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•7 years ago
|
||
(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.
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.
Assignee | ||
Comment 8•7 years ago
|
||
Talked with Shane on IRC. It sounds like the cached JAR handling in ExtensionProtocolHandler should also use SimpleChannel to let the forwarding work correctly.
Flags: needinfo?(haftandilian)
Reporter | ||
Comment 9•7 years ago
|
||
(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/webrequestRedirectExtUrl@robwu.nl.xpi')
Comment 10•7 years ago
|
||
I've also verified that removing the early return in SubstituteRemoteJarChannel fixes the redirects.
Assignee: nobody → haftandilian
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 12•7 years ago
|
||
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
Flags: needinfo?(rob)
Reporter | ||
Comment 13•7 years ago
|
||
(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).
Flags: needinfo?(rob)
Comment 14•7 years ago
|
||
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•7 years ago
|
||
I'm working on a test.
Assignee | ||
Comment 16•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment 18•7 years ago
|
||
Comment on attachment 8898476 [details]
Bug 1390346 test jar caching in combination with redirects,
or Rob
Attachment #8898476 -
Flags: review?(rob)
Reporter | ||
Comment 19•7 years ago
|
||
mozreview-review |
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')`.
Attachment #8898476 -
Flags: review?(rob)
Comment 20•7 years ago
|
||
mozreview-review |
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.
Attachment #8898476 -
Flags: review?(lgreco)
Comment 21•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Assignee | ||
Comment 25•7 years ago
|
||
(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•7 years ago
|
||
mozreview-review |
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•7 years ago
|
||
mozreview-review |
Comment on attachment 8898476 [details]
Bug 1390346 test jar caching in combination with redirects,
https://reviewboard.mozilla.org/r/169832/#review175438
Attachment #8898476 -
Flags: review?(lgreco) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 32•7 years ago
|
||
mozreview-review |
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?
Assignee | ||
Comment 33•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
mozreview-review |
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
Attachment #8898152 -
Flags: review?(jmathies) → review+
Comment 35•7 years ago
|
||
Comment 37•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9878f96c556e
Redirects to moz-extension:-URLs fail when loaded from a xpi. r=jimm
Keywords: checkin-needed
Comment 38•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/86bdc742c111
test jar caching in combination with redirects, r=rpl
Comment 39•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9878f96c556e
https://hg.mozilla.org/mozilla-central/rev/86bdc742c111
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Reporter | ||
Comment 40•7 years ago
|
||
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).
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(haftandilian)
Assignee | ||
Comment 41•7 years ago
|
||
(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.
Updated•7 years ago
|
Reporter | ||
Comment 42•7 years ago
|
||
(clearing NI; answered in comment 41)
Flags: needinfo?(haftandilian)
Comment 43•7 years ago
|
||
The intermittent looks like it's linux debug :( bumped the timeout a bit to see if that deals with it.
Comment 44•7 years ago
|
||
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
Attachment #8898152 -
Flags: approval-mozilla-beta?
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.
Attachment #8898152 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8898476 [details]
Bug 1390346 test jar caching in combination with redirects,
Tests should also land on beta.
Attachment #8898476 -
Flags: approval-mozilla-beta+
Comment 47•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/e9c83e49ec46
https://hg.mozilla.org/releases/mozilla-beta/rev/2968e01b6f20
Flags: in-testsuite+
Comment 48•7 years ago
|
||
(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.
Flags: qe-verify-
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•