580508 - nsURLHelper.cpp net_ParseFileURL is asserting on resource:// URLs that are aliased to other resource:// URLs

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Weave Sync registers a resource subsitution for "resource://services-sync", aliasing it to "resource://gre/services-sync":

http://mxr.mozilla.org/mozilla-central/source/services/sync/Weave.js#85

This causes assertions in net_ParseFileURL because the code expects to have a "file:///" URL by this point:

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsURLHelper.cpp#200

However, at this point the url is "resource://gre/services-sync/..."

Comment 1

[Philipp von Weitershausen [:philikon]]

nsResURL::EnsureFile() makes the assumption that whatever its URI resolves to is a file:// URI, so it simply calls GetFileFromURLSpec which ends up calling net_ParseFileURL:

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/res/nsResProtocolHandler.cpp#118

There might be other places that make the same assumption, so perhaps nsResProtocolHandler::SetSubstitution should check whether baseURI is a resource:// URI and resolve before storing the substitution rule in the mSubstitutions hash?

Comment 3

[Doug Turner (:dougt)]

philipp, that sounds good.  care to post a patch w/ tests?

Comment 4

[Philipp von Weitershausen [:philikon]]

Attachment: wip v1

Here's a WIP. It compiles and runs without problems, except the registration doesn't seem to happen, e.g.:

  Cu.import("resource:///modules/services.jsm");
  var resProt = Services.io.getProtocolHandler("resource")
               .QueryInterface(Ci.nsIResProtocolHandler);
  var greModulesURI = Services.io.newURI("resource://gre/modules/", null, null);
  resProt.setSubstitution("my-gre-modules", greModulesURI);

  // Returns null
  resProt.getSubstitution("my-gre-modules");

It's probably a minor detail I'm overlooking. Perhaps another set of eyes will spot the problem more quickly.

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: patch v1

Got it to work, had an extra getter_AddRefs() in there by mistake.

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 461503 [details] [diff] [review]
patch v1

Biesi is out & we need this ASAP, so switching review

Comment 7

[Jason Duell]

Comment on attachment 461503 [details] [diff] [review]
patch v1

Looks good.

>+    if (!scheme.Equals(NS_LITERAL_CSTRING("resource"))) {
>+        return mSubstitutions.Put(root, baseURI) ? NS_OK : NS_ERROR_UNEXPECTED;
>+    }
>+
>+    // baseURI is a resource URI, let's resolve it first.
>+    nsCAutoString newBase;
>+    rv = ResolveURI(baseURI, newBase);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsCOMPtr<nsIURI> newBaseURI;
>+    rv = mIOService->NewURI(newBase, nsnull, nsnull,
>+                            getter_AddRefs(newBaseURI));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    return mSubstitutions.Put(root, newBaseURI) ? NS_OK : NS_ERROR_UNEXPECTED;

I have a very minor preference for putting the resource URI logic in the if body, i.e.

   
>+    if (scheme.Equals(NS_LITERAL_CSTRING("resource"))) {
>+       // baseURI is a resource URI, let's resolve it first.

so that both branches could share the same mSubstitutions.Put call.  But no biggie.


>diff --git a/netwerk/test/unit/test_bug580508.js b/netwerk/test/unit/test_bug580508.js
>+
>+    // When we ask for the alias, we should not get the resource://
>+    // URI that we registered it for but the original file URI.
>+    let greFileSpec = ioService.newURI("modules/", null,
>+                                       resProt.getSubstitution("gre")).spec;

I had to jump into the source to understand that resProt.getSubstitution("gre")) works even when you haven't called setSubstitution("gre"), because it falls back to checking for "resource:gre".  Yet another feature of our (completely undocumented?) resource:// protocol.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/a2246039f70d

Comment 9

[Daniel Holbert [:dholbert]]

FWIW, this is suspected of causing xpcshell tinderbox orange:

> TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/mozilla-central-fedora-debug-u-xpcshell/build/xpcshell/tests/test_chrome/unit/test_resolve_uris.js | test failed (with xpcshell return code: 0), see following log:
> TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/mozilla-central-fedora-debug-u-xpcshell/build/xpcshell/tests/test_chrome/unit/test_resolve_uris.js | Should have registered a handler for type 'resource'
>TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/mozilla-central-fedora-debug-u-xpcshell/build/xpcshell/tests/test_chrome/unit_ipc/test_resolve_uris_ipc.js | test failed (with xpcshell return code: 0), see following log:
>TEST-UNEXPECTED-FAIL | ../unit/test_resolve_uris.js | Should have registered a handler for type 'resource'
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280797617.1280799708.15858.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280795519.1280797252.5964.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280798909.1280800248.18001.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280795508.1280797978.8970.gz

dolske is currently investigating to determine the best course of action, I believe.

Comment 10

[Justin Dolske [:Dolske]]

Bah. Test failures in test_chrome\unit\test_resolve_uris.js (and test_resolve_uris_ipc.js, looks to be the same issue).

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280795519.1280797252.5964.gz&fulltext=1#err0

...
Testing type 'override'
TEST-PASS | ../unit/test_resolve_uris.js | [do_run_test : 110] resource://foo/foo-override/override-me.xul == resource://foo/foo-override/override-me.xul
Testing type 'resource'
[Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIResProtocolHandler.resolveURI]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: ../unit/test_resolve_uris.js :: do_run_test :: line 104"  data: no]
TEST-UNEXPECTED-FAIL | ../unit/test_resolve_uris.js | Should have registered a handler for type 'resource'

Looks like the call to .resolveURI is failing when given "resource://foo/".

There are a couple warnings earlier in the output (or at the same time but buffering is messing with things?):

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file e:/builds/moz2_slave/mozilla-central-win32-debug/build/netwerk/protocol/res/nsResProtocolHandler.cpp, line 425


I can reproduce on my Windows 7 box.

Comment 11

[Justin Dolske [:Dolske]]

(I backed this out while we investigate)

Comment 12

[Justin Dolske [:Dolske]]

Ok, so when the test is setting up it looks to be triggering nsChromeRegistryChrome::CheckForNewChrome() which goes through this path, triggering the warnings mentioned above: when calling ResolveURI() it goes into GetSubstitution(), which goes into NS_GetSpecialDirectory(), which fails because nsDirectoryService::Get() doesn't know what to do with "resource:foo".

Not quite sure yet if this is a real failure, or if the test just happened to work before. The "foo" chrome registration looks a little odd, I'm not sure what it's actually supposed to be pointing to.

Comment 13

[Josh Matthews [:jdm]]

The purpose of that test was simply to be a sanity test for url resolution from the content process.

Comment 14

[Justin Dolske [:Dolske]]

The manifest the test is loading is:

  resource foo resource://foo/foo-resource/
  content foo resource://foo/foo-content/
  locale foo foo resource://foo/foo-locale/
  skin foo foo resource://foo/foo-skin/
  override chrome://good-package/content/override-me.xul resource://foo/foo-override/override-me.xul

(http://mxr.mozilla.org/mozilla-central/source/chrome/test/unit/data/test_resolve_uris.manifest)

I don't understand the first line; according to the manifest docs (https://developer.mozilla.org/en/chrome_registration#resource) the format is "resource namespace path", but the path here is a resource:// URI [is that even legal?], and it seems to be recursively pointing to itself.

So, seems to me this test is bogus?

Changing the first line to "resource foo ." makes the test work.

Comment 15

[Justin Dolske [:Dolske]]

Attachment: test fix (WIP)

This _almost_ fixes the test in question:

TEST-UNEXPECTED-FAIL | ../unit/test_resolve_uris.js | d:\mozilla-central\obj\_tests\xpcshell\test_chrome\unit\data == file:///d:/mozilla-central/obj/_tests/xpcshell/test_chrome/unit/data/

What's the right way to convert a nsIFile path to a file:// URI?

Comment 16

[Justin Dolske [:Dolske]]

Attachment: test fix v.2

Comment 17

[Justin Dolske [:Dolske]]

(blocking+ since this is needed for landing Weave)

Comment 18

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/e6b5e9bafbdf
http://hg.mozilla.org/mozilla-central/rev/141be33171c6