1405174 - resource://foo// resolves to parent directory when the directory pointed at by resource://foo/ doesn't exist

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Mike Hommey [:glandium]]

This made netwerk/test/unit/test_bug337744.js break after bug 1403366.

STR:
  - Open a scratchpad in a browser context
  - Enter the following script:

Cu.import("resource://gre/modules/NetUtil.jsm");
Cu.import("resource://gre/modules/Services.jsm");

var uri = NetUtil.newURI("http://www.example.com");
var principal = Services.scriptSecurityManager.createCodebasePrincipal(uri, {});

var resProto = Cc['@mozilla.org/network/protocol;1?name=resource'].getService(Ci.nsIResProtocolHandler);
var file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
file.initWithPath("/this/directory/does/not/exist");
var rootURI = Services.io.newFileURI(file);

resProto.setSubstitution("res-test", rootURI);

var channel = NetUtil.newChannel({
  uri: NetUtil.newURI("resource://res-test//"),
  loadingPrincipal: principal,
  securityFlags: Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
  contentPolicyType: Ci.nsIContentPolicy.TYPE_OTHER
});

channel.name

- Click "Display"

Expected result:
  /this/directory/does/not/exist//

Actual result:
  /this/directory/does/not//

Comment 1

[Mike Hommey [:glandium]]

So the problem is that when creating the URI from the file, when the directory exists, Services.io.newFileURI returns a URI that ends with a /, but not when the file doesn't exist, which makes sense.

Then, when the substituting protocol handler creates the resolved uri, it adds resolves './path' from that uri that doesn't end with a /, which makes it relative to the parent directory.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1405174 - Fix resolution of resource: URLs when the target of the substitution doesn't end with a /.

When for some reason the target of a resource: substitution doesn't end
with a / (which can happen when e.g. building a FileURI with a path
that doesn't exist), relative path resolution of the resource URLs end
up rooted under the parent of the non-existing directory.

e.g
  resource://foo/bar is substituted with /path/for/bar if
  resource://foo/ is registered for file:///path/for/foo (instead of
  file:///path/for/foo/)

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

Comment 3

[Mike Hommey [:glandium]]

Comment on attachment 8914630 [details]
Bug 1405174 - Fix resolution of resource: URLs when the target of the substitution doesn't end with a /.

This adds multiple slashes to the resulting path, and jar: urls don't like that, so that breaks omni.ja urls :(

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8914630 [details]
Bug 1405174 - Fix resolution of resource: URLs when the target of the substitution doesn't end with a /.

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

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8914630 [details]
Bug 1405174 - Fix resolution of resource: URLs when the target of the substitution doesn't end with a /.

https://reviewboard.mozilla.org/r/185952/#review191010

Thanks for adding a test for this!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9c05226e2248
Fix resolution of resource: URLs when the target of the substitution doesn't end with a /. r=valentin

Comment 7

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

Backed out

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9c05226e2248004f91a7487016612d32b39184d7&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable

Failure log https://treeherder.mozilla.org/logviewer.html#?job_id=134783673&repo=autoland
> TEST-UNEXPECTED-FAIL | netwerk/test/browser/browser_child_resource.js | Should resolve in main process - Got chrome://global/content/global.xul/test.js, expected chrome://global/content/test.js

Failure log https://treeherder.mozilla.org/logviewer.html#?job_id=134777743&repo=autoland
TEST-UNEXPECTED-FAIL | toolkit/components/addoncompat/tests/browser/browser_addonShims.js | Test timed out -
TEST-UNEXPECTED-FAIL | toolkit/components/addoncompat/tests/browser/browser_addonShims.js | test left unexpected property on window: runAddonShimTests -
TEST-UNEXPECTED-FAIL | toolkit/components/addoncompat/tests/browser/browser_addonShims.js | Found a tab after previous test timed out: resource://addonshim1/page.html -

Comment 8

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

Backed out for failing browser-chrome's toolkit/components/addoncompat/tests/browser/browser_addonShims.js and netwerk/test/browser/browser_child_resource.js:

https://hg.mozilla.org/integration/autoland/rev/c7de45b3805a1a2b567509d65d2b8c9e3c21d750

Comment 9

[Mike Hommey [:glandium]]

The problem with netwerk/test/browser/browser_child_resource.js is that it's setting resource urls to "root" chrome urls, and root chrome urls autoexpand to contain a file name.

That is, Services.io.newURI("chrome://global/content").spec is "chrome://global/content/global.xul", and Services.io.newURI("chrome://global/skin").spec is "chrome://global/skin/global.css".

per https://dxr.mozilla.org/mozilla-central/rev/11fe0a2895aab26c57bcfe61b3041d7837e954cd/chrome/nsChromeProtocolHandler.cpp#87

So, when doing e.g. resProtocol.setSubstitution("testing", Services.io.newURI("chrome://global/content"));, what really happens is resProtocol.setSubstitution("testing", Services.io.newURI("chrome://global/content/global.xul")); and the changes in this bug are breaking the assumptions that follow in the test.

There is absolutely no way to get a pure "chrome://global/content/" url, so something using such chrome urls as resource targets *are* relying on the current behavior of file-relative resolution stripping off the file name...

A way out of this is to only apply the changes in this bug to nsIFileURL (opt-in), or to exclude based on mozilla::dom::IsChromeURI() (opt-out).

Another would be for chrome urls to stop being magic, but I bet this would break a lot of stuff.

As for toolkit/components/addoncompat/tests/browser/browser_addonShims.js, it's kind of the same problem. It uses Services.io.newURI("chrome://addonshim1/content/"), which transforms that to chrome://addonshim1/content/addonshim1.xul, and asks the chrome registry to expand that, so it gets back a jar: uri that points to the xul file in a xpi.

Going through all the things that use setSubstitution, apart from those two tests that failed, only toolkit/components/search/tests/xpcshell/test_require_engines_in_cache.js does something fishy that the changes in this bug could break: resProt.setSubstitution("search-plugins", Services.io.newURI("about:blank")); but it's only trying to read the url as a directory.

Anyways, I think the reasonable way forward here is to only do the changes for nsIFileURL.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8914630 [details]
Bug 1405174 - Fix resolution of resource: URLs when the target of the substitution doesn't end with a /.

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

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8914630 [details]
Bug 1405174 - Fix resolution of resource: URLs when the target of the substitution doesn't end with a /.

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

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8914630 [details]
Bug 1405174 - Fix resolution of resource: URLs when the target of the substitution doesn't end with a /.

https://reviewboard.mozilla.org/r/185952/#review191626

This looks good, but I'd love to see even more tests for it. There seem to be loads of corner cases here.

Comment 13

[Mike Hommey [:glandium]]

(In reply to Valentin Gosu [:valentin] from comment #12)
> Comment on attachment 8914630 [details]
> Bug 1405174 - Fix resolution of resource: URLs when the target of the
> substitution doesn't end with a /.
> 
> https://reviewboard.mozilla.org/r/185952/#review191626
> 
> This looks good, but I'd love to see even more tests for it. There seem to
> be loads of corner cases here.

I don't think there's any corner case left that is not tested, considering the patch only affects nsIFileURLs now.

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8914630 [details]
Bug 1405174 - Fix resolution of resource: URLs when the target of the substitution doesn't end with a /.

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8aeb6541520a
Fix resolution of resource: URLs when the target of the substitution doesn't end with a /. r=valentin

Comment 16

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

Backed out for landing wrong version of patch: https://hg.mozilla.org/integration/autoland/rev/3087a8c87fe4f39ddfe81834418434554b5355a9

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32c618d07fe1
Fix resolution of resource: URLs when the target of the substitution doesn't end with a /. r=valentin

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/32c618d07fe1