Closed
Bug 558607
Opened 15 years ago
Closed 15 years ago
mailnewsMigrator.js logs an exception on "gPrefs.getCharPref("mail.accountmanager.accounts")"
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
VERIFIED
FIXED
Thunderbird 3.1b2
People
(Reporter: sgautherie, Assigned: BenB)
References
()
Details
Attachments
(1 file)
(deleted),
patch
|
Bienvenu
:
review+
standard8
:
superreview+
|
Details | Diff | Splinter Review |
Example:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1269723109.1269728839.28780.gz&fulltext=1
WINNT 5.2 comm-central-trunk debug test mochitests on 2010/03/27 13:51:49
+
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1270949484.1270949858.31090.gz&fulltext=1
Linux comm-central-trunk debug test mochitests-3/5 on 2010/04/10 18:31:24
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1270944164.1270946304.24412.gz&fulltext=1
WINNT 5.2 comm-central-trunk debug test mochitests-2/5 on 2010/04/10 17:02:44
+
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.3a5pre) Gecko/20100410 SeaMonkey/2.1a1pre] (comm-central-trunk-win32/1270896177)
{
-- Exception object --
+ QueryInterface (function) 3 lines
+ message (string) 'Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getCharPref]'
+ result (number) 2147549183
+ name (string) 'NS_ERROR_UNEXPECTED'
+ filename (string) 'file:///builds/slave/comm-central-trunk-linux-debug-unittest-mochitests-3/build/seamonkey/modules/mailnewsMigrator.js'
+ lineNumber (number) 70
+ columnNumber (number) 0
+ location (object) JS frame :: file:///builds/slave/comm-central-trunk-linux-debug-unittest-mochitests-3/build/seamonkey/modules/mailnewsMigrator.js :: MigrateServerAuthPref :: line 70
+ inner (object) null
+ data (object) null
+ initialize (function) 3 lines
}
I would guess 'mail.accountmanager.accounts' pref is not defined for the mochitest harness!?
http://mxr.mozilla.org/comm-central/search?string=mail.accountmanager.accounts&case=1
Comment 1•15 years ago
|
||
Wrong regression bug - bug 525238 doesn't touch anything to do with mailnewsMigrator.js. You want bug 465633 which is the only thing that touched that file (it created it!).
Comment 2•15 years ago
|
||
FWIW, bug 465633 didn't really create the file, it did an mv from migration.jsm.
Comment 3•15 years ago
|
||
so yeah, I think bug 525238 is the regressing bug here.
Assignee | ||
Comment 4•15 years ago
|
||
What's the bug here? MigrateServerAuthPref() catches the exception. So, where's the problem? The exception log output?
You could, in the migrator, check first whether the pref exists, but I would recommend *against* it. 99.9% of the cases, it exists, and making an extra check costs more time. In contrast, an exception is more expensive when thrown, but the try/catch has almost no cost when there's no exception. So, unexpected cases like "no account" can be handled fine by exceptions (and the logException *is* useful for us developers and should do no harm for release builds).
So, what about mochitests? As Serge said, why doesn't it have any accounts defined? Maybe it should? If it shouldn't, a workaround to silence this would be to define an empty pref("mail.accountmanager.accounts", "");
Assignee | ||
Comment 5•15 years ago
|
||
Similarly pref("mail.smtpservers", "");
Comment 6•15 years ago
|
||
(In reply to comment #4)
> So, what about mochitests? As Serge said, why doesn't it have any accounts
> defined? Maybe it should? If it shouldn't, a workaround to silence this would
> be to define an empty pref("mail.accountmanager.accounts", "");
Mochitests is primarily a browser based function. You could essentially treat it as a fresh SeaMonkey profile where the user hasn't started up mailnews yet (or at all), and yes some SM users do use it like that.
I suspect in the TB case by the time we've got that far started, we've also created some kind of accounts so we wouldn't see it on a fresh profile - but maybe we would.
I'm kinda on the fence as to what we should do. An empty pref might be the best/clearest way though I'm not really sure.
Assignee | ||
Comment 7•15 years ago
|
||
(Personally, I believe that every pref we use - no mater how obscure, nor matter if dynamically changed - should have a default pref.)
Reporter | ||
Comment 8•15 years ago
|
||
(In reply to comment #4)
> What's the bug here? [...] The exception log output?
Exactly.
> So, what about mochitests? [...] a workaround to silence this would
> be to define an empty pref("mail.accountmanager.accounts", "");
That's what I was suggesting, but I don't know this code nor what the best solution is.
(In reply to comment #6)
> Mochitests is primarily a browser based function. You could essentially treat
> it as a fresh SeaMonkey profile where the user hasn't started up mailnews yet
That's exactly what I assume is happening.
Assignee | ||
Comment 9•15 years ago
|
||
> > What's the bug here? [...] The exception log output?
> Exactly.
Note a mere log output (even of an exception) is not a bug.
(But I agree, we should probably just add the default prefs.)
Reporter | ||
Comment 10•15 years ago
|
||
(In reply to comment #9)
> Note a mere log output (even of an exception) is not a bug.
To me, it is in itself...
Comment 11•15 years ago
|
||
(In reply to comment #8)
> (In reply to comment #4)
>
> > What's the bug here? [...] The exception log output?
>
> Exactly.
Actually, the bug is the mochitest failing. The exception log output might explain well why it does, though. ;-)
(In reply to comment #7)
> (Personally, I believe that every pref we use - no mater how obscure, nor
> matter if dynamically changed - should have a default pref.)
I don't agree 100% here. IMHO, there are prefs that can go without a default value, but in all those cases, we should be able to deal with that situation and never ever throw uncaught or otherwise fail fatally. If we don't have a default value, there must be a reasonable case where it isn't set and that case should never be fatal.
Assignee | ||
Comment 12•15 years ago
|
||
> never ever throw uncaught or otherwise fail fatally.
We do neither here.
Assignee | ||
Comment 13•15 years ago
|
||
> Actually, the bug is the mochitest failing. The exception log output might
> explain well why it does, though. ;-)
FWIW, that's not true. The exception shown here can't possibly cause the test to fail (unless the testsuite is broken and by failing on all output), because the exception is not propagated and contained.
I haven't seen any evidence that links a test failure to the exception.
Comment 14•15 years ago
|
||
If there's, as you say, no test failure here, I'm not sure why I'm CCed here, so I'm taking myself off here. I got bigger fish to fry. I know Serge settles for nothing else than perfection, but I'm not daring to go that far myself. ;-)
Assignee | ||
Comment 16•15 years ago
|
||
Thanks, _Tsk_
Assignee: nobody → ben.bucksch
No longer blocks: SmTestFail
Component: Testing Infrastructure → Backend
QA Contact: testing-infrastructure → backend
Assignee | ||
Comment 17•15 years ago
|
||
Attachment #438705 -
Flags: superreview?(bugzilla)
Attachment #438705 -
Flags: review?(bienvenu)
Assignee | ||
Comment 18•15 years ago
|
||
I added the |if (!accountKey)|, because it's needed not only here, but I have IIRC seen freaky pref where the user has "account1,,account2".
Assignee | ||
Updated•15 years ago
|
Summary: [SeaMonkey] mochitests-*: mailnewsMigrator.js fails on "gPrefs.getCharPref("mail.accountmanager.accounts")" → [SeaMonkey] mochitests-*: mailnewsMigrator.js logs an exception on "gPrefs.getCharPref("mail.accountmanager.accounts")"
Assignee | ||
Updated•15 years ago
|
Summary: [SeaMonkey] mochitests-*: mailnewsMigrator.js logs an exception on "gPrefs.getCharPref("mail.accountmanager.accounts")" → mailnewsMigrator.js logs an exception on "gPrefs.getCharPref("mail.accountmanager.accounts")"
Comment 19•15 years ago
|
||
Comment on attachment 438705 [details] [diff] [review]
Fix, v1
this looks OK to me, but I can't verify that it fixes the SM mochitests...
Attachment #438705 -
Flags: review?(bienvenu) → review+
Assignee | ||
Comment 20•15 years ago
|
||
FWIW, simpler reproduction of the bug:
Start TB from the (command prompt) console using ./thunderbird -no-remote -P newprofile
You'll see the exception output on the console.
Comment 21•15 years ago
|
||
Comment on attachment 438705 [details] [diff] [review]
Fix, v1
I can't see anything that relies on the prefs being missing on first use (there shouldn't be anyway), so sr=Standard8
Attachment #438705 -
Flags: superreview?(bugzilla) → superreview+
Assignee | ||
Comment 22•15 years ago
|
||
Thanks.
Commited as <http://hg.mozilla.org/comm-central/rev/1f508d2cf905>
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 23•15 years ago
|
||
V.Fixed, per "Linux comm-central-trunk debug test mochitests-2/5" builds
Status: RESOLVED → VERIFIED
Target Milestone: --- → Thunderbird 3.1b2
You need to log in
before you can comment on or make changes to this bug.
Description
•