[Session Restore] Discriminate more precisely between new profile and catastrophic crash

RESOLVED FIXED in Firefox 34

Status

()

Firefox
Session Restore
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Yoric, Assigned: smacleod, Mentored)

Tracking

unspecified
Firefox 34
Points:
3
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js][good second bug])

Attachments

(1 attachment)

Bug 1035557 fixes an issue in which we failed to discriminate between a new profile and a catastrophic crash. However, now that bug 883609 has landed, by tweaking slightly the loading code, we can find this information much more precisely.

We need to:
- tweak `SessionFile.read` to return one more field `freshProfile` that is `true` if none of the files we attempt to read even exist, or `false` if at least one exists;
- tweak `nsSesionStartup._onSessionFileRead` to use this information if `checkpoints` is `null`, by setting `this._previousSessionCrashed` to `false` if `freshProfile` is `true`, or by using `this._initialState` as we currently do if `freshProfile` is `false`.
Depends on: 1035557
(Assignee)

Comment 1

3 years ago
Marco, please add this to the current iteration
Assignee: nobody → smacleod
Status: NEW → ASSIGNED
Iteration: --- → 34.1
Points: --- → 3
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Added to Iteration 34.1
QA Whiteboard: [qa?]
Flags: needinfo?(mmucci)
Hi Steven, can you mark this bug as [qa+] or [qa-] for verification.
Flags: needinfo?(smacleod)

Updated

3 years ago
Iteration: 34.1 → 34.2
QA Whiteboard: [qa?] → [qa+]
QA Contact: cornel.ionce

Updated

3 years ago
Flags: needinfo?(smacleod)
(Assignee)

Comment 4

3 years ago
Created attachment 8471079 [details] [diff] [review]
Patch - Use the existence of any session files to determine if a profile crashed or is fresh
Attachment #8471079 - Flags: review?(ttaubert)
Comment on attachment 8471079 [details] [diff] [review]
Patch - Use the existence of any session files to determine if a profile crashed or is fresh

Review of attachment 8471079 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/SessionFile.jsm
@@ +240,5 @@
>          parsed: null
>        };
>      }
>  
> +    result["noFilesFound"] = noFilesFound;

Maybe result.noFilesFound = ...? That's a little more common in the code base.
Attachment #8471079 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 6

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/8ce45b343da9
Target Milestone: --- → Firefox 34

Comment 7

3 years ago
https://hg.mozilla.org/mozilla-central/rev/8ce45b343da9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Iteration: 34.2 → 34.3
QA Whiteboard: [qa+]
Flags: qe-verify+
Is there any proper way to manually test this fix?
Flags: needinfo?(smacleod)
(Assignee)

Comment 9

3 years ago
(In reply to Cornel Ionce [QA] from comment #8)
> Is there any proper way to manually test this fix?

The way this was implemented it shouldn't actually result in a behaviour change, it just makes the code more explicit about why we do things. I don't think we need to verify this.

Sorry about the confusion!
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(smacleod)
You need to log in before you can comment on or make changes to this bug.