Closed Bug 648018 Opened 11 years ago Closed 11 years ago

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

Categories

(Thunderbird :: Account Manager, defect)

defect
Not set
normal

Tracking

(blocking-thunderbird5.0 needed)

RESOLVED FIXED
Thunderbird 5.0b1
Tracking Status
blocking-thunderbird5.0 --- needed

People

(Reporter: Usul, Assigned: standard8)

References

Details

(Keywords: regression)

Attachments

(2 files, 3 obsolete files)

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]
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
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.
Blocks: 549045
(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/ ..
No longer blocks: 549045
Depends on: 549045
I just tested it. I saved the attached file as isp/foobar.com, entered "foo@foobar.com" 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.
Status: NEW → RESOLVED
Closed: 11 years ago
No longer depends on: 549045
Resolution: --- → INVALID
Attached file foobar.com.xml
"jar:file:///Users/dul/Miramar.app/Contents/MacOS/omni.jar!/isp/mozilla.com.xml"
Assignee: nobody → bugzilla
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Status: REOPENED → ASSIGNED
Keywords: regression
> 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.
(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.
blocking-thunderbird5.0: --- → needed
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 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
Attached patch The fix (obsolete) — Splinter Review
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.
OS: Mac OS X → All
Hardware: x86 → All
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
Attached patch The fix v2 (obsolete) — Splinter Review
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.
Attachment #528953 - Attachment is obsolete: true
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.)
(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 ;-)
Attached patch The fix v2a (obsolete) — Splinter Review
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.
Attachment #529057 - Attachment is obsolete: true
Attachment #529076 - Flags: review?(ben.bucksch)
> 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);
});
> 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.
(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.
> > 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.
Attached patch The fix v3Splinter Review
Updated to address the comments.
Attachment #529076 - Attachment is obsolete: true
Attachment #529076 - Flags: review?(ben.bucksch)
Attachment #529278 - Flags: review?(ben.bucksch)
Comment on attachment 529278 [details] [diff] [review]
The fix v3

Perfect, thank you! :-) r=BenB
Attachment #529278 - Flags: review?(ben.bucksch) → review+
Checked in: http://hg.mozilla.org/comm-central/rev/23be2a8ad627
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a4
Depends on: 710295
Summary: ISP files aren't read anymore → ISP files aren't read from harddisk anymore since the omni.jar change
You need to log in before you can comment on or make changes to this bug.