Closed
Bug 648018
Opened 14 years ago
Closed 14 years ago
ISP files aren't read from harddisk anymore since the omni.jar change
Categories
(Thunderbird :: Account Manager, defect)
Thunderbird
Account Manager
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)
2.13 KB,
application/xml
|
Details | |
7.92 KB,
patch
|
BenB
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
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•14 years ago
|
||
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.
Reporter | ||
Comment 3•14 years ago
|
||
(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/ ..
Updated•14 years ago
|
Comment 4•14 years ago
|
||
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.
Updated•14 years ago
|
Comment 5•14 years ago
|
||
Keywords: regression,
regressionwindow-wanted
Reporter | ||
Comment 6•14 years ago
|
||
"jar:file:///Users/dul/Miramar.app/Contents/MacOS/omni.jar!/isp/mozilla.com.xml"
Assignee: nobody → bugzilla
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Reporter | ||
Updated•14 years ago
|
Status: REOPENED → ASSIGNED
Reporter | ||
Updated•14 years ago
|
Keywords: regression
Comment 7•14 years ago
|
||
> 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.
Assignee | ||
Comment 8•14 years ago
|
||
(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
Comment 9•14 years ago
|
||
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?
Reporter | ||
Comment 10•14 years ago
|
||
(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
Assignee | ||
Comment 11•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Comment 12•14 years ago
|
||
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
Assignee | ||
Comment 13•14 years ago
|
||
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
Comment 14•14 years ago
|
||
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.)
Assignee | ||
Comment 15•14 years ago
|
||
(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 ;-)
Assignee | ||
Comment 16•14 years ago
|
||
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)
Comment 17•14 years ago
|
||
> 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•14 years ago
|
||
> 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.
Assignee | ||
Comment 19•14 years ago
|
||
(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•14 years ago
|
||
> > 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.
Assignee | ||
Comment 21•14 years ago
|
||
Updated to address the comments.
Attachment #529076 -
Attachment is obsolete: true
Attachment #529076 -
Flags: review?(ben.bucksch)
Attachment #529278 -
Flags: review?(ben.bucksch)
Comment 22•14 years ago
|
||
Comment on attachment 529278 [details] [diff] [review]
The fix v3
Perfect, thank you! :-) r=BenB
Attachment #529278 -
Flags: review?(ben.bucksch) → review+
Assignee | ||
Comment 23•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago → 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a4
Updated•13 years ago
|
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.
Description
•