555715 - Replace resource://app/ with resource:///

Status: VERIFIED FIXED

(MailNews Core :: Build Config, defect)

Description

[Robert Kaiser]

http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/protocol/res/src/nsResProtocolHandler.cpp#178 says that resource:/// points to the application directory and below it resource://gre/ is defined to point to the GRE directory, but I can't find any documentation that might point out that resource://app/ even exists.

This might be the reason why we see errors with those URLs in debug builds, e.g. when running xpcshell tests.

We should replace resource://app/ with resource:/// where we have it in our code.

Comment 1

[Robert Kaiser]

OK, I can confirm that doing that replacement makes the errors in xpcshell tests go away. Right now, we're throwing NS_ERROR_NOT_AVAILABLE for every Components.utils.import line using a resource://app/ URI, with a patch replacing resource://app/ with resource:/// we are not throwing any more.

Comment 2

[Robert Kaiser]

Working on a patch, not sure which component it belongs in, as it touches all of mailnews, mail, and suite.

Comment 3

[Robert Kaiser]

Attachment: simple find and replace

This patch is a simple find-and-replace pass over all of mail, mailnews, and suite. This apparently invalid URI construct is not found anywhere outside those directories.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Where are the errors happening?

I apparently explicitly added a case to mailDirService.js to deal with this in mailnews tests:
http://mxr.mozilla.org/comm-central/source/mailnews/test/resources/mailDirService.js#64

Comment 5

[Ben Bucksch (:BenB)]

The resource "resource://app/" makes sense to me, semantically.
Why not just add a resource that makes it valid (and points to the same dir as resource:///) instead of the global search/replace?

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Heh, I was wondering where we got to use the idea "app" from, and a gloda search found this:

https://bugzilla.mozilla.org/show_bug.cgi?id=463278#c9

Not sure it is the absolute genesis.

Comment 7

[Robert Kaiser]

(In reply to comment #4)
> Where are the errors happening?
> 
> I apparently explicitly added a case to mailDirService.js to deal with this in
> mailnews tests:
> http://mxr.mozilla.org/comm-central/source/mailnews/test/resources/mailDirService.js#64

Interesting. I saw it happening in non-mailnews tests as SeaMonkey loads nsSuiteGlue.js (which loads migration.jsm from mailnews) and msgAsyncPrompter.js on every startup, and those failed to load with that throw.

I agree that resource://app/ would be somewhat clearer in meaning than resource:/// but we'd need to add it to the global version, IMHO, as else we're basically loading invalid URIs (even though they seem to load OK in most cases).
And in the end, resource:/// means exactly what we mean with resource://app/.

Comment 8

[Robert Kaiser]

(In reply to comment #6)
> Heh, I was wondering where we got to use the idea "app" from, and a gloda
> search found this:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=463278#c9
> 
> Not sure it is the absolute genesis.

Hrm. OK, I take the blame, I just was wrong but everybody accepted what I said. :(

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Thunderbird might actually be establishing the "app" mapping at some point, but I think the main issue is that, as KaiRo says, we would realy need to establish it at the dawn of time with the other mappings for perfect consistency.

This very likely explains why all my resource://app/ imports from inside JS XPCOM components didn't work during the top-level evaluation.  (I think I eventually cottoned onto it, but assumed it was an intentional platform limitation...)

Thanks for tracking this down and preparing a patch!

Comment 10

[Mark Banner (:standard8)]

Attachment: Fixed for bitrot

This bitrotted already :-( Still I've fixed that and run the Thunderbird tests against it and it all seems to work fine :-) r=Standard8

Comment 11

[Robert Kaiser]

I actually had updated the patch by myself for bitrot and new additions, Standard8 agreed over IRC I can take his review for that, so I pushed it as http://hg.mozilla.org/comm-central/rev/ffe61760113a

Comment 12

[Serge Gautherie (:sgautherie)]

V.Fixed, per bug 556663 comment 0.