Closed Bug 558607 Opened 10 years ago Closed 10 years ago

mailnewsMigrator.js logs an exception on "gPrefs.getCharPref("mail.accountmanager.accounts")"

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Thunderbird 3.1b2

People

(Reporter: sgautherie, Assigned: BenB)

References

()

Details

Attachments

(1 file)

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
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!).
Blocks: 465633
No longer blocks: 525238
FWIW, bug 465633 didn't really create the file, it did an mv from migration.jsm.
so yeah, I think bug 525238 is the regressing bug here.
Blocks: 525238
No longer blocks: 465633
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", "");
Similarly pref("mail.smtpservers", "");
(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.
(Personally, I believe that every pref we use - no mater how obscure, nor matter if dynamically changed - should have a default pref.)
(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.
> > 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.)
(In reply to comment #9)
> Note a mere log output (even of an exception) is not a bug.

To me, it is in itself...
(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.
> never ever throw uncaught or otherwise fail fatally.

We do neither here.
> 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.
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. ;-)
Duplicate of this bug: 559015
Thanks, _Tsk_
Assignee: nobody → ben.bucksch
No longer blocks: SmTestFail
Component: Testing Infrastructure → Backend
QA Contact: testing-infrastructure → backend
Attached patch Fix, v1Splinter Review
Attachment #438705 - Flags: superreview?(bugzilla)
Attachment #438705 - Flags: review?(bienvenu)
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".
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")"
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 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+
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 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+
Thanks.
Commited as <http://hg.mozilla.org/comm-central/rev/1f508d2cf905>
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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.