735573 - "Failed to setup pref service" on first-run from ResetAndReadUserPrefs

Status: VERIFIED FIXED

(Core :: Preferences: Backend, defect, P3)

Description

[Serge Gautherie (:sgautherie)]

(Moved from bug 734883 comment 15.)

UseDefaultPrefFile() code is:
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/Preferences.cpp#586
{
593   rv = NS_GetSpecialDirectory(NS_APP_PREFS_50_FILE, getter_AddRefs(aFile));
594   if (NS_SUCCEEDED(rv)) {
595     rv = ReadAndOwnUserPrefFile(aFile);
596     // Most likely cause of failure here is that the file didn't
597     // exist, so save a new one. mUserPrefReadFailed will be
598     // used to catch an error in actually reading the file.
599     if (NS_FAILED(rv)) {
600       rv2 = SavePrefFileInternal(aFile);
601       NS_ASSERTION(NS_SUCCEEDED(rv2), "Failed to save new shared pref file");
602     }
603   }
604   
605   return rv;
}

Startup crash tracking does:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/xre/nsXREDirProvider.cpp#736
{
742     nsresult rv = mozilla::Preferences::ResetAndReadUserPrefs();
743     if (NS_FAILED(rv)) NS_WARNING("Failed to setup pref service.");
}

At each first startup of a new profile, the latter warning is reported.
I assume the startup crash tracking feature is then implicitly disabled for that startup.

Should rv be set to NS_OK when NS_SUCCEEDED(rv2)?
Should we (re)try to read the new file?
...

Comment 1

[Matthew N. [:MattN]]

(In reply to Serge Gautherie (:sgautherie) from comment #0)
> (Moved from bug 734883 comment 15.)
> 
> UseDefaultPrefFile() code is:
> http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/
> Preferences.cpp#586
> {
> 593   rv = NS_GetSpecialDirectory(NS_APP_PREFS_50_FILE,
> getter_AddRefs(aFile));
> 594   if (NS_SUCCEEDED(rv)) {
> 595     rv = ReadAndOwnUserPrefFile(aFile);
> 596     // Most likely cause of failure here is that the file didn't
> 597     // exist, so save a new one. mUserPrefReadFailed will be
> 598     // used to catch an error in actually reading the file.
> 599     if (NS_FAILED(rv)) {
> 600       rv2 = SavePrefFileInternal(aFile);
> 601       NS_ASSERTION(NS_SUCCEEDED(rv2), "Failed to save new shared pref
> file");
> 602     }
> 603   }
> 604   
> 605   return rv;
> }

Getting rid of rv2 and setting the return value of SavePrefFileInternal to rv would get rid of the warning for new profiles.  I'm not sure why we are keeping the return values separate here.

> Startup crash tracking does:
> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/xre/
> nsXREDirProvider.cpp#736
> {
> 742     nsresult rv = mozilla::Preferences::ResetAndReadUserPrefs();
> 743     if (NS_FAILED(rv)) NS_WARNING("Failed to setup pref service.");
> }
> 
> At each first startup of a new profile, the latter warning is reported.
> I assume the startup crash tracking feature is then implicitly disabled for
> that startup.

The startup crash count isn't incremented on the first run in a new profile anyways since it needs toolkit.startup.recent_crashes to be set from the previous startup.  This warning actually isn't specific to startup crash tracking, it's just that no warning was reported before since it was part of the profile-do-change observer.

> Should rv be set to NS_OK when NS_SUCCEEDED(rv2)?
I think we could replace rv2 with rv.

> Should we (re)try to read the new file?

I think it's fine without since this was probably failing before in Observe.

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Document UseDefaultPrefFile() and fix its nsresult value [Checked in: Comment 4]

Untested (yet).


(In reply to Matthew N. [:MattN] from comment #1)

> > Should rv be set to NS_OK when NS_SUCCEEDED(rv2)?

By code inspection, that should be enough to fix ResetAndReadUserPrefs(), which is the only caller of this case.

> I think we could replace rv2 with rv.

I can do it if you think that is wanted, but ftb I assumed rv2 had been used on purpose.

Comment 3

[Mozilla RelEng Bot]

Try run for d7cd48e5d178 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d7cd48e5d178
Results (out of 223 total builds):
    exception: 2
    success: 178
    warnings: 28
    failure: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d7cd48e5d178

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 605832 [details] [diff] [review]
(Av1) Document UseDefaultPrefFile() and fix its nsresult value
[Checked in: Comment 4]

https://hg.mozilla.org/mozilla-central/rev/2cec1f79a141

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 605832 [details] [diff] [review]
(Av1) Document UseDefaultPrefFile() and fix its nsresult value
[Checked in: Comment 4]

[Approval Request Comment]
Regression caused by (bug #): (revealed by) Bug 294260.
User impact if declined: Not sure, but wrong interaction (with a 'WARNING') with new feature at least on all our boxes.
Testing completed (on m-c, etc.): Try, this comment 4.
Risk to taking this patch (and alternatives if risky): (Very) Low.
String changes made by this patch: None.

Comment 6

[Matthew N. [:MattN]]

(In reply to Serge Gautherie (:sgautherie) from comment #5)
> User impact if declined: Not sure, but wrong interaction (with a 'WARNING')
> with new feature at least on all our boxes.

This is for first-run only and release builds don't output NS_WARNINGs.

Comment 7

[Serge Gautherie (:sgautherie)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=10300331&tree=Firefox&full=1
Rev3 Fedora 12 mozilla-central debug test mochitests-3/5 on 2012-03-22 19:00:02 PDT for push 2cec1f79a141

V.Fixed

Comment 8

[Alex Keybl [:akeybl]]

Comment on attachment 605832 [details] [diff] [review]
(Av1) Document UseDefaultPrefFile() and fix its nsresult value
[Checked in: Comment 4]

[Triage Comment]
Given Comment 6, this doesn't appear necessary for Aurora 13.