Currently the previousCheckpoints promise has the following behaviour: resolve(null): if - The file does not exist - Reading fails - JSON.parse(...) fails reject(exception): if - JSON.parse(...) passes, but with a non object result (e.g. the checkpoints file contains "1234") resolve({Checkpoints}): otherwise The only place that mentions rejecting in error cases (not just resolving null) is a single test which checks this behavior [1]. We should probably either have the promise |resolve(null)| in all error cases, or reject in all error cases. The way SessionStore consumes this API, we really don't care about errors, and would prefer to just have the Crash Monitor make its best guess about crashes. Maybe we should have additional API which allows consumers to be dumb about these errors, and switch this promise to reject. e.g: |CrashMonitor.reachedCheckpoint("<checkpoint-name>").then(bool)| which would always resolve to a boolean. [1] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/crashmonitor/test/unit/test_invalid_file.js

(In reply to Steven MacLeod [:smacleod] from comment #0) > The way SessionStore consumes this API, we really don't care about errors, > and would prefer to just have the Crash Monitor make its best guess about > crashes. Maybe we should have additional API which allows consumers to be > dumb about these errors, and switch this promise to reject. e.g: > > |CrashMonitor.reachedCheckpoint("<checkpoint-name>").then(bool)| which would > always resolve to a boolean. I really like this idea. This way sessionstore and other future clients don't have to know about how the CM works. It would mean that for checkPoints=null, reachedCheckPoint() would always resolve to 'true'. I would even go as far as to remove the previousCheckPoints promise and hide implementation detail by default. If there ever is another client that wants to know more we can go back and think about it again.

Hey Steven, can I take this bug?

(In reply to Roy Li from comment #2) > Hey Steven, can I take this bug? Yup! I've assigned you the bug.

It looks like previousCheckpoints has already been changed to return a Task with the desired functionality.

(In reply to Roy Li from comment #4) > It looks like previousCheckpoints has already been changed to return a Task > with the desired functionality. Yes, it appears that one part of this bug has been fixed. The case where the JSON parses to a non object now resolves with null[1] instead of rejecting with an error. The other piece of this bug though is introducing the simpler API: (In reply to Steven MacLeod [:smacleod] from comment #0) > The way SessionStore consumes this API, we really don't care about errors, > and would prefer to just have the Crash Monitor make its best guess about > crashes. Maybe we should have additional API which allows consumers to be > dumb about these errors, and switch this promise to reject. e.g: > > |CrashMonitor.reachedCheckpoint("<checkpoint-name>").then(bool)| which would > always resolve to a boolean. I still think having a simpler API that consumers can use would be nice. The being said, I've realized that the way we're consuming the current API in Session Store[2], we actually do take into account whether previousCheckpoints resolves null vs was written but doesn't contain our checkpoint. If previousCheckpoints is null we're assuming the file doesn't exist - if the session files also don't exist we assume a fresh profile. Tim, Yoric, it looks like Session Store wouldn't be able to make use of the new simpler API itself, do you think it's still worth fixing this? Should we try to roll some sort of fresh profile detection into the crash monitor itself too? Roy, it might turn out we won't fix this bug, or it might be morphed a little bit. I'll leave you assigned to this but we should probably wait to hear back from Tim/Yoric before moving forward. Feel free to pick up other bugs in the mean time. [1] http://mxr.mozilla.org/mozilla-central/source/toolkit/components/crashmonitor/CrashMonitor.jsm?rev=1b532d05bccf#121 [2] http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/nsSessionStartup.js?rev=04ae87ed4a41#168

Well, I believe that Session Store could be slightly refactored (to assume that it's a new profile iff none of the sessionstore.* files can be found), but what would be the interest?

(In reply to Steven MacLeod [:smacleod] from comment #5) > Yes, it appears that one part of this bug has been fixed. The case where the > JSON parses to a non object now resolves with null[1] instead of rejecting > with an error. That's great, consistency is nice. > I still think having a simpler API that consumers can use would be nice. Yes, the current API is weird. > The being said, I've realized that the way we're consuming the current API > in Session Store[2], we actually do take into account whether > previousCheckpoints resolves null vs was written but doesn't contain our > checkpoint. If previousCheckpoints is null we're assuming the file doesn't > exist - if the session files also don't exist we assume a fresh profile. True, we do want different behaviors on upgrade. We could extend the API to provide a method that tells us whether there was a file at all... This is a little flawed though. SessionStore uses the presence of |session.state| to tell whether it's an upgrade or a crash where the file couldn't be written - i.e. not a shutdown crash but a regular crash which probably occurs more often. The API is thus only usable for sessionstore that can use extra information to tell *why* checkpoints=null. Maybe we should start handling checkpoints=null implies that previousSessionCrashed=true - that means use CrashMonitor.reachedCheckpoint("checkpoint").then(...). We can let |session.state| override this status when it exists? So the flow would probably be: 1) if (noFilesFound) then previousSessionCrashed=false, stop. => There's nothing to restore anyway. 2) if (stateFlagPresent) then previousSessionCrashed=(session.state != STATE_RUNNING_STR), stop. => The CM could still report that we crashed in case we imported an old session after crashing but then the crash doesn't affect our data anymore if it was overridden manually. 3) previousSessionCrashed=CM.reachedCheckpoint("sessionstore-final-state-write-complete"). Yet another idea would be to let the CrashManager return reachedCheckpoint=true in case of an upgrade. The CM could handle E_FILE_NOT_FOUND as "upgrade" when we detect that the profile hasn't been used with a version that shipped the CM yet? There must be some code that enables us to check it, we have a toolbar that asks you to reset your profile if you haven't used it in like 60 days. Should of course do the same for a new profile. Maybe that's stuff for another bug though.

resetting assignee at the request of Roy.