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

ASSIGNED
Assigned to

Status

SeaMonkey
Startup & Profiles
--
enhancement
ASSIGNED
5 years ago
a year ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

({helpwanted})

Trunk
helpwanted
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.10 affected)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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
?
(Assignee)

Updated

5 years ago
Target Milestone: --- → seamonkey2.10

Comment 1

5 years ago
Dunno, but it sounds like a good idea anyway.
(Assignee)

Updated

5 years ago
Depends on: 734886
(Assignee)

Comment 2

5 years ago
Created attachment 604985 [details] [diff] [review]
(Av1) Port |Bug 294260 - Safe Mode: Auto detect previous start-up failure and offer to start in safe mode|, non-UI part

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)
(Assignee)

Updated

5 years ago
Blocks: 36810
(Assignee)

Comment 3

5 years ago
(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?
(Assignee)

Comment 4

5 years ago
(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
(Assignee)

Comment 5

5 years ago
Created attachment 604998 [details] [diff] [review]
(Av1a) Port |Bug 294260 - Safe Mode: Auto detect previous start-up failure and offer to start in safe mode|, non-UI part

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?
(Assignee)

Comment 7

5 years ago
(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.

Comment 8

5 years ago
> +                    .getService(Ci.nsIAppStartup);
I don't think we have the Ci shortcut. in nsSuiteGlue.js
(Assignee)

Comment 9

5 years ago
Created attachment 605508 [details] [diff] [review]
(Av1b) Port |Bug 294260 - Safe Mode: Auto detect previous start-up failure and offer to start in safe mode|, non-UI part

Av1a, with comment 8 suggestion(s).
Attachment #604998 - Attachment is obsolete: true
Attachment #605508 - Flags: review?(iann_bugzilla)
Attachment #604998 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 10

5 years ago
(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...
(Assignee)

Comment 11

5 years ago
(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.
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Updated

5 years ago
Depends on: 735573
(Assignee)

Comment 15

5 years ago
(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
status-seamonkey2.10: --- → affected
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.
(Assignee)

Comment 17

5 years ago
Created 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

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 18

5 years ago
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)
(Assignee)

Comment 19

5 years ago
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-
(Assignee)

Comment 21

5 years ago
(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
(Assignee)

Updated

5 years ago
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.

Updated

5 years ago
Target Milestone: seamonkey2.11 → ---
You need to log in before you can comment on or make changes to this bug.