Open Bug 734883 Opened 8 years ago Updated 4 years ago

Port |Bug 294260 - Safe Mode: Auto detect previous start-up failure and offer to start in safe mode| to SeaMonkey

Categories

(SeaMonkey :: Startup & Profiles, enhancement)

enhancement
Not set

Tracking

(seamonkey2.10 affected)

ASSIGNED
Tracking Status
seamonkey2.10 --- affected

People

(Reporter: sgautherie, Assigned: sgautherie)

References

Details

(Keywords: helpwanted)

Attachments

(1 file, 3 obsolete files)

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1331529579.1331534365.8340.gz&fulltext=1
Linux comm-central-trunk debug test mochitests-3/5 on 2012/03/11 22:19:39
{
WARNING: Failed to setup pref service.: file /builds/slave/comm-cen-trunk-lnx-dbg/build/mozilla/toolkit/xre/nsXREDirProvider.cpp, line 742
}

All platforms, all mochitests-*.

***

Do SeaMonkey needs to port (some of) the browser/* part from
https://hg.mozilla.org/mozilla-central/rev/bcdc5493e6a1
?
Target Milestone: --- → seamonkey2.10
Dunno, but it sounds like a good idea anyway.
Depends on: 734886
This does not port:
*UI part: the 3 safeMode.* files.
 *This should be fine to do as a follow-up patch.
*browser.js delayedStartup() part.
 *See below.
And I can't test it...

***

http://mxr.mozilla.org/comm-central/search?string=delayedStartup&case=on&find=%2Fsuite%2F
nsSessionStore.js refers to delayedStartup() which SeaMonkey does not have (yet).
Should that be ported first/too?
Does this bug depends on it?
Attachment #604985 - Flags: review?(iann_bugzilla)
Blocks: 36810
(In reply to Serge Gautherie (:sgautherie) from comment #2)
> http://mxr.mozilla.org/comm-central/
> search?string=delayedStartup&case=on&find=%2Fsuite%2F
> nsSessionStore.js refers to delayedStartup() which SeaMonkey does not have
> (yet).
> Should that be ported first/too?
> Does this bug depends on it?

It looks like we need to at least add that trackStartupCrashEnd() call somewhere, for example to enable the pref and pass
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/startup/tests/browser/browser_crash_detection.js
Where would that be?
(In reply to Serge Gautherie (:sgautherie) from comment #0)
> WARNING: Failed to setup pref service.: file
> /builds/slave/comm-cen-trunk-lnx-dbg/build/mozilla/toolkit/xre/
> nsXREDirProvider.cpp, line 742

It looks like TrackStartupCrashEnd() is already called somehow, though I don't know how that happens (on SeaMonkey) :-/
http://mxr.mozilla.org/comm-central/search?string=TrackStartupCrashEnd
Av1, with added default preference value (to enable the feature).
Attachment #604985 - Attachment is obsolete: true
Attachment #604998 - Flags: review?(iann_bugzilla)
Attachment #604985 - Flags: review?(iann_bugzilla)
Could the root cause of the warning be investigated?  The warning and porting the feature should be two separate bugs.  Does this warning happen on every startup?
(In reply to Matthew N. [:MattN] from comment #6)

> Could the root cause of the warning be investigated?

I tried: could you answer comment 0, comment 2 and comment 4?

> The warning and porting the feature should be two separate bugs.

Why (= genuine question)? They were added in the same bug...

> Does this warning happen on every startup?

Yes, see comment 0.

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1331584266.1331593066.374.gz&fulltext=1
OS X 10.6 comm-central-trunk leak test build on 2012/03/12 13:31:06
is affected too.
> +                    .getService(Ci.nsIAppStartup);
I don't think we have the Ci shortcut. in nsSuiteGlue.js
Av1a, with comment 8 suggestion(s).
Attachment #604998 - Attachment is obsolete: true
Attachment #605508 - Flags: review?(iann_bugzilla)
Attachment #604998 - Flags: review?(iann_bugzilla)
(In reply to Serge Gautherie (:sgautherie) from comment #4)
> (In reply to Serge Gautherie (:sgautherie) from comment #0)
> > WARNING: Failed to setup pref service.: file
> > /builds/slave/comm-cen-trunk-lnx-dbg/build/mozilla/toolkit/xre/
> > nsXREDirProvider.cpp, line 742
> 
> It looks like TrackStartupCrashEnd() is already called somehow, though I
> don't know how that happens (on SeaMonkey) :-/
> http://mxr.mozilla.org/comm-central/search?string=TrackStartupCrashEnd

Bah, I got confused:
this is unrelated to TrackStartupCrashEnd(),
this is mozilla::Preferences::ResetAndReadUserPrefs() which fails.

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

calls
http://mxr.mozilla.org/comm-central/source/mozilla/modules/libpref/src/Preferences.cpp#359
364   sPreferences->ResetUserPrefs();
365   return sPreferences->ReadUserPrefs(nsnull);

calls
http://mxr.mozilla.org/comm-central/source/mozilla/modules/libpref/src/Preferences.cpp#395
399   if (XRE_GetProcessType() == GeckoProcessType_Content) {
400     NS_ERROR("cannot load prefs from content process");
401     return NS_ERROR_NOT_AVAILABLE;
402   }
403 
404   nsresult rv;
405 
406   if (nsnull == aFile) {
407 
408     NotifyServiceObservers(NS_PREFSERVICE_READ_TOPIC_ID);
409 
410     rv = UseDefaultPrefFile();
411     UseUserPrefFile();
412   } else {
413     rv = ReadAndOwnUserPrefFile(aFile);
414   }
415   return rv;

But the nsresult value is not reported: it would need a debugger to find out what is going on...
(In reply to Serge Gautherie (:sgautherie) from comment #10)

> http://mxr.mozilla.org/comm-central/source/mozilla/modules/libpref/src/
> Preferences.cpp#395
> 399   if (XRE_GetProcessType() == GeckoProcessType_Content) {
> 400     NS_ERROR("cannot load prefs from content process");

This error is not reported: fine.

> 401     return NS_ERROR_NOT_AVAILABLE;
> 402   }
> 403 
> 404   nsresult rv;
> 405 
> 406   if (nsnull == aFile) {
> 407 
> 408     NotifyServiceObservers(NS_PREFSERVICE_READ_TOPIC_ID);
> 409 
> 410     rv = UseDefaultPrefFile();

UseDefaultPrefFile() 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;

The assertion is not reported either, so it fails at NS_GetSpecialDirectory() or ReadAndOwnUserPrefFile().
I used to get this but after I updated my build recently I have stopped seeing this. It might be related to the bug about the chrome folder not appearing in new profiles - I think there is a default pref file too, and now that new profiles get populated the pref service doesn't warn on startup.
(In reply to neil@parkwaycc.co.uk from comment #12)

> I used to get this but after I updated my build recently I have stopped

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1331678053.1331688389.14754.gz&fulltext=1
OS X 10.5 comm-central-trunk leak test build on 2012/03/13 15:34:13

still has it.

> seeing this. It might be related to the bug about the chrome folder not
> appearing in new profiles - I think there is a default pref file too, and
> now that new profiles get populated the pref service doesn't warn on startup.

Are referring to bug 728840?
(In reply to Serge Gautherie (:sgautherie) from comment #7)
> (In reply to Matthew N. [:MattN] from comment #6)
> 
> > Could the root cause of the warning be investigated?
> 
> I tried: could you answer comment 0, comment 2 and comment 4?

To implement startup crash tracking, you just need to call TrackStartupCrashEnd when you want to define the end of startup and set the pref IIRC.  For Firefox, it is 30s after delayedStartup() which means that we also need to make sure that TrackStartupCrashEnd is called on shutdown (in case they shutdown before the 30s).  Attachment 605508 [details] [diff] still needs work because it doesn't call TrackStartupCrashEnd at the end of startup.
 
> > The warning and porting the feature should be two separate bugs.
> 
> Why (= genuine question)? They were added in the same bug...

Because porting the feature will not remove the warning.  The warning is shown because there are no prefs to read in a new profile. It's because UseDefaultPrefFile doesn't return the successful nsresult rv2 in this case at https://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/Preferences.cpp?rev=12813323739a#588

> > Does this warning happen on every startup?
> 
> Yes, see comment 0.

I believe the test machines always use a new profile for testing and so that's why you see the warning on every one.  I was talking about every startup for one profile.
Depends on: 735573
(In reply to Matthew N. [:MattN] from comment #14)

> Attachment 605508 [details] [diff] still needs work
> because it doesn't call TrackStartupCrashEnd at the end of startup.

Thanks for the explanation/confirmation:
*nsSuiteGlue.js part should be "commented out" until SeaMonkey uses a "delayedStartup()".
*For now, I need (to find) an answer to comment 3 on the SeaMonkey side.

> https://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/
> Preferences.cpp?rev=12813323739a#588

Right, that's what I was hinting at in comment 11.

Actually, I was sure I had verified (2-3 times) that Firefox was not affected, but it is (now):
https://tbpl.mozilla.org/php/getParsedLog.php?id=10044669&tree=Firefox&full=1
Rev3 Fedora 12 mozilla-central debug test mochitests-3/5 on 2012-03-13 15:55:34 PDT for push c71845b3b2a6

I filed bug 735573.

Ftr, I did
http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=41a6b83a864b
but it doesn't reproduce useful steps for this issue :-|
(But it could be done again: for Firefox...)

> I believe the test machines always use a new profile for testing and so
> that's why you see the warning on every one.  I was talking about every

Yes.

> startup for one profile.

Oh. (I can't check that.)
Assignee: nobody → sgautherie.bz
Severity: normal → enhancement
Status: NEW → ASSIGNED
Keywords: helpwanted
Summary: [SeaMonkey] "WARNING: Failed to setup pref service. [...] /toolkit/xre/nsXREDirProvider.cpp, line 742" → Port |Bug 294260 - Safe Mode: Auto detect previous start-up failure and offer to start in safe mode| to SeaMonkey
Target Milestone: seamonkey2.10 → seamonkey2.11
(In reply to Serge Gautherie from comment #13)
> (In reply to neil@parkwaycc.co.uk from comment #12)
> > I used to get this but after I updated my build recently I have stopped
> > seeing this. It might be related to the bug about the chrome folder not
> > appearing in new profiles - I think there is a default pref file too, and
> > now that new profiles get populated the pref service doesn't warn on startup.
> Are referring to bug 728840?
Indeed, the lack of the chrome folder is just one symptom of the default files not getting copied to the profile folder, and I thought that there was a default pref file that we were therefore lacking thus triggering that warning.
Av1b, with (delayed) startup part.
(Untested.)
Attachment #605508 - Attachment is obsolete: true
Attachment #606094 - Flags: review?(iann_bugzilla)
Attachment #605508 - Flags: review?(iann_bugzilla)
Comment on attachment 606094 [details] [diff] [review]
(Av1c) Port |Bug 294260 - Safe Mode: Auto detect previous start-up failure and offer to start in safe mode| non-UI part, Add a minimal delayedStartup() function

As Neil is more familiar with the safe mode code, another option would be :InvisibleSmiley both were involved with bug 573538
Attachment #606094 - Flags: review?(iann_bugzilla) → review?(neil)
Ftr,
{
[21:17]	<IanN>	sgautherie: is there an easy way to test your new code?

[21:38]	<sgautherie>	IanN: utilityOverlay.js part is the Restart item in the Help menu I think.
I guess "startup crash tracking" should trigger if you manually kill SM process within "30 seconds"...
}
Comment on attachment 606094 [details] [diff] [review]
(Av1c) Port |Bug 294260 - Safe Mode: Auto detect previous start-up failure and offer to start in safe mode| non-UI part, Add a minimal delayedStartup() function

>+// Startup Crash Tracking:
>+// Number of startup crashes that can occur before starting into safe mode automatically.
>+// (This pref has no effect if more than 6 hours have passed since the last crash.)
>+pref("toolkit.startup.max_resumed_crashes", 2);
Maybe put this near browser.sessionstore.max_resumed_crashes?

>+  // End startup crash tracking after a (30 seconds) delay to catch crashes
>+  // while restoring tabs and to postpone saving the pref to disk.
Unfortunately we need this to work for all windows, not just browsers.

>+      Components.classes["@mozilla.org/toolkit/app-startup;1"]
>+                .getService(Components.interfaces.nsIAppStartup)
>+                .restartInSafeMode(Components.interfaces.nsIAppStartup.eAttemptQuit);
Does this notify observers in the way Application.restart() does?
Attachment #606094 - Flags: review?(neil) → review-
(In reply to neil@parkwaycc.co.uk from comment #20)
> >+  // End startup crash tracking after a (30 seconds) delay to catch crashes
> >+  // while restoring tabs and to postpone saving the pref to disk.
> Unfortunately we need this to work for all windows, not just browsers.

Is there an existing common place to add this feature to?
Or which are the other windows (file)?
No longer depends on: 294260, 734886, 735573
Depends on: 294260, 734886, 735573
(In reply to Serge Gautherie from comment #21)
> (In reply to neil@parkwaycc.co.uk from comment #20)
> > >+  // End startup crash tracking after a (30 seconds) delay to catch crashes
> > >+  // while restoring tabs and to postpone saving the pref to disk.
> > Unfortunately we need this to work for all windows, not just browsers.
> Is there an existing common place to add this feature to?
nsSuiteGlue, although of course you can't use setTimeout there.
Target Milestone: seamonkey2.11 → ---
You need to log in before you can comment on or make changes to this bug.