648018 - ISP files aren't read from harddisk anymore since the omni.jar change

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Ludovic Hirlimann [:Usul]]

1) Create an ISP config xml file.
2) put it in the ISP directory
3) Try to create an account

Account data is fetched from the ISPDB

Console says :
mail.wizardINFOfetchConfigFromDisk failed: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIChannel.open]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: chrome://messenger/content/accountcreation/util.js :: readURLasUTF8 :: line 112"  data: no]

Comment 1

[Ben Bucksch (:BenB)]

What's the exact filename and directory?

You know that you must append ".xml"?

<https://developer.mozilla.org/en/Thunderbird/Autoconfiguration#Mechanisms>
> tb-install-dir/isp/example.com.xml on the harddisk

Comment 2

[Blake Winton (:bwinton) (:☕️)]

I put a similar file in /Users/bwinton/Programming/thunderbird/objdir-src-central-default/mozilla/dist/ShredderDebug.app/Contents/MacOS/isp/mozilla.com.xml and it worked just fine for me.  Come see me, and I'll show you.

But, I want to leave this open so that we can track my fixing the documentation and adding some code to tell us which file it can't find when it can't find a file.

Later,
Blake.

Comment 3

[Ludovic Hirlimann [:Usul]]

(In reply to comment #1)
> What's the exact filename and directory?
> 
> You know that you must append ".xml"?
> 
> <https://developer.mozilla.org/en/Thunderbird/Autoconfiguration#Mechanisms>
> > tb-install-dir/isp/example.com.xml on the harddisk

mozilla.com.xml

in /Applications/Shredder.app/Content/MacOs/Content/isp/ ..

Comment 4

[Ben Bucksch (:BenB)]

I just tested it. I saved the attached file as isp/foobar.com, entered "[e-mail]" as email address, clicked Continue, and *immediately* saw imap.frooop.de as email server.

Your XML file probably is invalid and thus skipped.

INVALID.

Please attach your mozilla.com.xml file, and I can help you debugging it.

Comment 5

[Ben Bucksch (:BenB)]

Attachment: foobar.com.xml

Comment 6

[Ludovic Hirlimann [:Usul]]

"jar:file:///Users/dul/Miramar.app/Contents/MacOS/omni.jar!/isp/mozilla.com.xml"

Comment 7

[Ben Bucksch (:BenB)]

> jar:file:///Users/dul/Miramar.app/Contents/MacOS/omni.jar!/isp/mozilla.com.xml

Sorry, that doesn't help me. Please attach it.

Also, you cannot put this file in the JAR, IIRC.

Comment 8

[Mark Banner (:standard8)]

(In reply to comment #7)
> > jar:file:///Users/dul/Miramar.app/Contents/MacOS/omni.jar!/isp/mozilla.com.xml
> 
> Sorry, that doesn't help me. Please attach it.

The xml file is valid.

> Also, you cannot put this file in the JAR, IIRC.

What Ludovic should have said was that the account wizard is looking for the xml file in omni.jar in omni.jar enabled builds.

I have a rough idea of the fix this but I need to check on what we've done elsewhere first.

Comment 9

[Ben Bucksch (:BenB)]

Sorry, but I don't see the bug. It worksforme. Can somebody show me how to reproduce the problem, or describe what the problem is?

Comment 10

[Ludovic Hirlimann [:Usul]]

(In reply to comment #9)
> Sorry, but I don't see the bug. It worksforme. Can somebody show me how to
> reproduce the problem, or describe what the problem is?

In a nightly (probably works in Alpha3 too) :
1) download a known ISP xml file from live.momo/autoconfig/...
2) Add that file into the proper directory
3) Create a new account for that domain
4) see that the account information is fetched from the ispdb and not from the file

Comment 11

[Mark Banner (:standard8)]

Attachment: The fix

This should fix the issue. It switches us from using a resource URI to loading the file directly.

I've also included a unit test - whilst it copies the file to the same directory, I think its still a reasonable test to include and tests a bit more of the autoconfig code.

I've just pushed this to try server to make sure it doesn't break with packaged tests.

Comment 12

[Ben Bucksch (:BenB)]

Mark, fix looks good to me, thanks.
Do we create the resource /isp/ somewhere? If so, we should remove it.
Also, please change "Components.interfaces." to "Ci.", that saves a whole line.
With these 2 things, r=BenB

Comment 13

[Mark Banner (:standard8)]

Attachment: The fix v2

Thanks Ben. I've fixed the Components.interfaces. I'm pretty sure we don't define the /isp/ resource anywhere - that's just a bit of magic on the resource uri (and I'm pretty sure the old style account wizard would need it anyway).

I'll land this later when I get chance.

Comment 14

[Ben Bucksch (:BenB)]

Thanks. Yes, you're right, we'll still need /isp/ for movemail.
I haven't looked over the test, I assume it's OK.
(The Ci. is not fixed in "The fix v2" yet, but I trust that you'll do it.)

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #14)
> I haven't looked over the test, I assume it's OK.
> (The Ci. is not fixed in "The fix v2" yet, but I trust that you'll do it.)

Guess who forgot the hg qrefresh ;-)

Comment 16

[Mark Banner (:standard8)]

Attachment: The fix v2a

Did you want to take a quick look at the test? It basically moves the test file into place then calls fetchConfigFromDisk and checks the config this is returned.

Comment 17

[Ben Bucksch (:BenB)]

> Did you want to take a quick look at the test?

Sure.

nit: "example.invalid" suggests to me that the file is intentionally invalid. I recommend using domain "example.com" (or "example.net"), because that domain is already defined to be invalid.

important: You are using an old XML file format. Please use <http://live.mozillamessaging.com/autoconfig/v1.1/arcor.de> as base.

nit: Use explicit target filename:
file.copyTo(copyLocation, kXMLFile);

> do_timeout(0, finish_test);

Please avoid timeouts whenever possible, they make the testsuite unpredictable and very hard to work with. If one is needed, please make a comment why, and how it could be fixed without timeout.

In this case, the test should not finish before the ISP file is removed, because it could taint the next test. Such a problem would be hard to find, if it ever appears.

> var result = {}

This makes the code hard to read. If you remove the timeout, you could also pass the |result| variable directly to  finish_test() as parameters.
If you need the timeout, you could still avoid |result| by using a closure, i.e. calling
doTimeout(0, function() {
  finish_test(true, xml);
});

Comment 18

[Ben Bucksch (:BenB)]

> the test should not finish before the ISP file is removed,

Nevermind that sentence (the test doesn't finish before the ISP file is removed). Rest stays.

Comment 19

[Mark Banner (:standard8)]

(In reply to comment #17)
> important: You are using an old XML file format. Please use
> <http://live.mozillamessaging.com/autoconfig/v1.1/arcor.de> as base.

Hmm, we may want a bug for our other tests then, because I copied it from those.

> nit: Use explicit target filename:
> file.copyTo(copyLocation, kXMLFile);

Err, I am using the target filename. Unless you mean to use the raw string itself - I specifically wanted to use the constant as it is referenced in more than one location in the file, and if we change it we should only have to change it in one place.

> > do_timeout(0, finish_test);
> 
> Please avoid timeouts whenever possible, they make the testsuite unpredictable
> and very hard to work with. If one is needed, please make a comment why, and
> how it could be fixed without timeout.

I should have probably commented that the timeout is just so we get cleanly out of the async function call before cleaning up the test. Although I may rework that slightly.

Comment 20

[Ben Bucksch (:BenB)]

> > nit: Use explicit target filename:
> > file.copyTo(copyLocation, kXMLFile);

> Unless you mean to use the raw string itself

No. FYI, current code has: file.copyTo(copyLocation, "");

> the timeout is just so we get cleanly out
> of the async function call before cleaning up the test.

I don't understand. You mean "the async function call" == fetchConfigFromDisk() ? When the fetchConfigFromDisk() calls the success or error callbacks, it's finished. Anything else would be a bug.

Comment 21

[Mark Banner (:standard8)]

Attachment: The fix v3

Updated to address the comments.

Comment 22

[Ben Bucksch (:BenB)]

Comment on attachment 529278 [details] [diff] [review]
The fix v3

Perfect, thank you! :-) r=BenB

Comment 23

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/23be2a8ad627