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

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Serge Gautherie (:sgautherie)]

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

[Mark Banner (:standard8)]

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

[Rain [:rain1] (inactive)]

FWIW, bug 465633 didn't really create the file, it did an mv from migration.jsm.

Comment 3

[Rain [:rain1] (inactive)]

so yeah, I think bug 525238 is the regressing bug here.

Comment 4

[Ben Bucksch (:BenB)]

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", "");

Comment 5

[Ben Bucksch (:BenB)]

Similarly pref("mail.smtpservers", "");

Comment 6

[Mark Banner (:standard8)]

(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.

Comment 7

[Ben Bucksch (:BenB)]

(Personally, I believe that every pref we use - no mater how obscure, nor matter if dynamically changed - should have a default pref.)

Comment 8

[Serge Gautherie (:sgautherie)]

(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.

Comment 9

[Ben Bucksch (:BenB)]

> > 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.)

Comment 10

[Serge Gautherie (:sgautherie)]

(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

[Robert Kaiser]

(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.

Comment 12

[Ben Bucksch (:BenB)]

> never ever throw uncaught or otherwise fail fatally.

We do neither here.

Comment 13

[Ben Bucksch (:BenB)]

> 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

[Robert Kaiser]

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. ;-)

Comment 16

[Ben Bucksch (:BenB)]

Thanks, _Tsk_

Comment 17

[Ben Bucksch (:BenB)]

Attachment: Fix, v1

Comment 18

[Ben Bucksch (:BenB)]

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".

Comment 19

[David :Bienvenu]

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...

Comment 20

[Ben Bucksch (:BenB)]

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

[Mark Banner (:standard8)]

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

Comment 22

[Ben Bucksch (:BenB)]

Thanks.
Commited as <http://hg.mozilla.org/comm-central/rev/1f508d2cf905>

Comment 23

[Serge Gautherie (:sgautherie)]

V.Fixed, per "Linux comm-central-trunk debug test mochitests-2/5" builds