Defect - MetroFx should treat termination-from-a-suspended-state as a normal shutdown

VERIFIED FIXED in Firefox 28

Status

defect
P2
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: verdi, Assigned: TimAbraldes)

Tracking

22 Branch
Firefox 29
x86
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(firefox28 verified, firefox29 verified)

Details

(Whiteboard: [beta28] feature=defect c=tbd u=tbd p=3)

Attachments

(1 attachment, 5 obsolete attachments)

I've never changed the default start up setting to show the start page when Firefox starts but after a recent update, Firefox started showing my tabs from last time.

Nightly version 22.0a1 (2013-03-12)

STR:
1. Check your settings: Settings > Options > Startup > When Nightly starts, show start page.
2. Open some tabs in metro Firefox
3. Shut the computer down
4. Start the computer
5. Start metro Firefox. You will see the start page briefly and then it will load the tabs you had open last time.

Expected result: When Firefox reopens it should show the start page.
I cannot reproduce this with the latest m-c nightly. Michael, can you re-test and see if this is still happening?
Double-checked. Still happening for me. Here's a video of what I'm seeing - http://people.mozilla.org/~mverdi/video/metro-startup.m4v
This is probably because of session restore and unclean shutdown.  The flicker of the start screen I just fixed in another bug and that should land soon.  You can also get an unclean shutown by right clicking the application in the left windows application list (windows key + shift + tab bar).
I uninstalled nightly, removed my profile and then started over. Now it works as expected.
Please re-open if you disagree
No longer blocks: metrov1triage
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
Seeing your comment just now I thought I'd double-check my machine. Using the same profile created in Comment 4 (and sync not set up) I now get my tabs restored after shutting the computer down, waiting 60 seconds and then restarting. That's using Nightly 22.0a1 (2013-03-25).

I can also reproduce this in my VM install.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Summary: Startup setting is show start page but Firefox shows my tabs from last time → Startup setting is show start page but Firefox shows my tabs from last time after rebooting my computer
Summary: Startup setting is show start page but Firefox shows my tabs from last time after rebooting my computer → Startup setting is show start page but Firefox shows my tabs from last time after rebooting my computer with Firefox open
k thanks, I updated the title to reflect that this is a problem with Firefox not shutting down properly when the OS reboots.
Depends on: 917020
Marking this bug as dependent on bug 917020. Since we're removing the option to show the start page on startup, this bug will no longer be relevant
This bug is still valid and it is to add proper shutdown support when the computer is rebooting via listening for the broadcast windows message.

p=2
Summary: Startup setting is show start page but Firefox shows my tabs from last time after rebooting my computer with Firefox open → Defect - Startup setting is show start page but Firefox shows my tabs from last time after rebooting my computer with Firefox open
Whiteboard: [triage] feature=defect c=tbd u=tbd p=0
Summary: Defect - Startup setting is show start page but Firefox shows my tabs from last time after rebooting my computer with Firefox open → Defect - Unclean shutdown (process termination) invokes session restore
Whiteboard: [triage] feature=defect c=tbd u=tbd p=0 → feature=defect c=tbd u=tbd p=0
Summary: Defect - Unclean shutdown (process termination) invokes session restore → Defect - Support proper shutdown when the browser is suspended and the system reboots
Whiteboard: feature=defect c=tbd u=tbd p=0 → [release28] feature=defect c=tbd u=tbd p=0
Assignee: nobody → ally
Assignee: ally → nobody
Assignee: nobody → tabraldes
Blocks: metrov1it20
No longer blocks: metrov1backlog
Status: REOPENED → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [release28] feature=defect c=tbd u=tbd p=0 → [block28] feature=defect c=tbd u=tbd p=2
Whiteboard: [block28] feature=defect c=tbd u=tbd p=2 → [block28] feature=defect c=tbd u=tbd p=3
Blocks: metrov1it21
No longer blocks: metrov1it20
Whiteboard: [block28] feature=defect c=tbd u=tbd p=3 → [beta28] feature=defect c=tbd u=tbd p=3
When metroFx gets suspended, our MetroApp::OnSuspending [1] function gets called. At this point, our process has 5s to flush its session state to disk or be terminated. Once metroFx is suspended, there is no guarantee that it will get to execute any additional code. In particular, if the user closes metroFx from the suspended state (bug 922657), or if the machine reboots (bug 850372), or if Windows terminates the process to free up resources (bug 949107), metroFx does not execute any additional code. Currently, metroFx treats any termination from a suspended state as a crash, but it needs to treat termination from a suspended state as a normal shutdown.

I'm morphing this bug to track the underlying issue rather than one of the symptoms, and will mark the dependent bugs as duplicates.

The fix seems to be to move more of our "normal shutdown" logic into our handling of process suspension. In particular, whatever mechanism we use to detect crashes should be disabled when our process gets suspended.

[1] https://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroApp.cpp?rev=7c8790d032b5#137
Summary: Defect - Support proper shutdown when the browser is suspended and the system reboots → Defect - MetroFx should treat termination-from-a-suspended-state as a normal shutdown
Duplicate of this bug: 922657
Duplicate of this bug: 949107
Posted patch Patch v1 (obsolete) — Splinter Review
This seems to work... but it couldn't be _that_ easy, could it?

I'll test whether this patch breaks session restore in cases I can think of. If it seems not to break anything I'll r?
Posted patch Patch v2 (obsolete) — Splinter Review
From what I can tell, this does the trick.

This patch changes browser/metro/SessionStore.js to do the following:
  - Use the existence of sessionstore.js rather than sessionstore.bak as an indication that the previous session crashed
  - Observe suspend_process_notification and clear out the sessionstore files on suspend
  - Observe resume_process_notification and re-create sessionstore.js on resume
Attachment #8346244 - Attachment is obsolete: true
Attachment #8346898 - Flags: review?(mbrubeck)
Comment on attachment 8346898 [details] [diff] [review]
Patch v2

This breaks session restore in the case where the app was terminated by the OS but never closed by the user.  It also prevents us from even offering a "restore session" button, since the data is gone.

I don't want to regress this case, since it's something that can make Firefox appear to unpredictably throw away user state for no visible reason.  I'd rather err (as we currently do) on the side of restoring state than on the side of losing it.

If we're going to delete the user's session, I want to make sure it's in response to an *explicit* user action.  It looks like we'll need to check the PreviousExecutionState on startup to determine that correctly.
Attachment #8346898 - Flags: review?(mbrubeck) → review-
Sounds like we'll have to update [1] and [2] to read the PreviousExecutionState, then update all of the execution paths in [2] to pass the PreviousExecutionState as a command-line arg. Somewhere we'll have to actually read the arg and affect SessionStore's behavior based on its value.

Will investigate more tomorrow.

[1] https://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/FrameworkView.cpp?rev=e20c9dd37a86#349
[2] https://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroContracts.cpp?rev=7c8790d032b5#158
No longer blocks: 922657
Posted patch Patch v3 (obsolete) — Splinter Review
This looks more like the approach we want to take. I'm still testing the various scenarios, but in the ones I've tested so far this seems to do what we want.
Attachment #8346898 - Attachment is obsolete: true
Attachment #8349641 - Flags: review?(mbrubeck)
Comment on attachment 8349641 [details] [diff] [review]
Patch v3

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

This looks good overall, but I want to make sure _shouldRestore is set correctly in each case:

::: browser/metro/components/SessionStore.js
@@ +68,5 @@
>          let timeout = Services.prefs.getIntPref("browser.sessionstore.resume_from_crash_timeout");
> +        let previousExecutionState = Services.metro.previousExecutionState;
> +        this._shouldRestore = (delta < (timeout * 60000))
> +                           && (previousExecutionState == 0
> +                               || previousExecutionState == 3);

Please add comments with the equivalent ApplicationExecutionState name for each number.

I think the logic here should be slightly different, and should maybe be a switch statement that explicitly lists each state:

For state 0 (NotRunning), I think we should have behave differently in "crash" and "non-crash" cases.  If shutdown was unclean (i.e. if sessionstore.bak exists AND resume_from_crash_timeout has not expired) then restore the session.  Otherwise, follow the MSDN guideline for NotRunning, which is "Display initial UI."

For state 3 (Terminated), I think we should restore the session always.  We shouldn't check the resume_from_crash_timeout in this case, since this is not a crash.

For state 4 (ClosedByUser), MSDN claims that we should restore the session as of Windows 8.1.  Frankly I think this is a weird decision by Microsoft, and they are not totally clear about it, so if we decide we should start a fresh session instead then I will not object.

We should never see 1 (Running) or 2 (Suspended) here; perhaps we should log an error if we see any unexpected state.

I would also be in favor of getting rid of the resume_from_crash_timeout pref (and instead keep the session around for an unlimited amount of time after a crash), but I suppose we can decide that in a separate bug.
Attachment #8349641 - Flags: review?(mbrubeck) → review-
Posted patch Patch v4 (obsolete) — Splinter Review
This incorporates the previous review comments and adds some comments trying to help explain each case.

I haven't tested this yet; just wanted to get the changes up first.
Attachment #8349641 - Attachment is obsolete: true
Attachment #8349723 - Flags: review?(mbrubeck)
Posted patch Patch v5 (obsolete) — Splinter Review
Fixed the log message for the 'Suspended' case.

Testing looks good so far; metroFx is restoring when I expect it to and starting fresh when I expect it to
Attachment #8349723 - Attachment is obsolete: true
Attachment #8349723 - Flags: review?(mbrubeck)
Attachment #8349761 - Flags: review?(mbrubeck)
Comment on attachment 8349723 [details] [diff] [review]
Patch v4

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

r=mbrubeck with some minor tweaks below.

::: browser/metro/components/SessionStore.js
@@ +61,1 @@
>          this._sessionFileBackup.remove(false);

If a backup file is found, set a flag like "shutdownWasUnclean = true"...

@@ +72,5 @@
> +            let delta = Date.now() - this._lastSessionTime;
> +            let timeout =
> +              Services.prefs.getIntPref(
> +                  "browser.sessionstore.resume_from_crash_timeout");
> +            this._shouldRestore = (delta < (timeout * 60000));

...and here you should set "this._shouldRestore = (shutdownWasUnclean && delta < ...)".

This will prevent us from restoring incorrectly after a clean shutdown and reboot.

@@ +85,5 @@
> +          // 2 == Suspended
> +          case 2:
> +            // We should never encounter this situation
> +            Components.utils.reportError("SessionRestore.init called with "
> +                                       + "previous execution state 'Running'");

Copy/paste error - 'Running' is wrong here.

We could replace case 1 and case 2 with a "default" case and a generic error message.

@@ +101,5 @@
> +          case 4:
> +            // ClosedByUser indicates that the user performed a "close" gesture
> +            // on our tile. This presumably happened while our process was
> +            // suspended, or else we wouldn't have detected the presence of
> +            // _sessionFile. We should act as if the browser closed normally.

I don't think this comment is correct -- it's _sessionFileBackup that indicates whether the shutdown was unclean, not _sessionFile.  We can reach this case whether the app was closed while running or closed while suspended.

The logic, however, appears correct.
Attachment #8349723 - Attachment is obsolete: false
Attachment #8349761 - Flags: review?(mbrubeck) → review+
Posted patch Patch v6Splinter Review
Review comments addressed, carrying forward r+
Attachment #8349723 - Attachment is obsolete: true
Attachment #8349761 - Attachment is obsolete: true
Attachment #8349781 - Flags: review+
Tested locally:
  - Ran mochitest-metro
  - Tested functionality

Setting checkin-needed since this can land with other changes but I won't be able to do any follow-up that might be necessary (backouts, etc)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/76f9e33252b6
Keywords: checkin-needed
Whiteboard: [beta28] feature=defect c=tbd u=tbd p=3 → [beta28] feature=defect c=tbd u=tbd p=3[fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/76f9e33252b6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [beta28] feature=defect c=tbd u=tbd p=3[fixed-in-fx-team] → [beta28] feature=defect c=tbd u=tbd p=3
Target Milestone: --- → Firefox 29
Comment on attachment 8349781 [details] [diff] [review]
Patch v6

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Present since initial landing in bug 750903 (and present before that in elm).
User impact if declined: In various cases, metroFx will recover a previous session instead of starting a fresh session.
Testing completed (on m-c, etc.): This has been on m-c since December 19th
Risk to taking this patch (and alternatives if risky): Low-risk. This is a metro-only change.
String or IDL/UUID changes made by this patch: This patch changes the UUID of nsIWinMetroUtils because it adds a |previousExecutionState| member to that interface.
Attachment #8349781 - Flags: approval-mozilla-aurora?
Comment on attachment 8349781 [details] [diff] [review]
Patch v6

It's Aurora so the UUID change should be OK.  Approving.
Attachment #8349781 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flagging this for verification. Tim, under which circumstances should Metro Firefox restore a previous session and under which circumstances should it start a new session with your changes?
Flags: needinfo?(tabraldes)
Keywords: verifyme
Assuming metroFx is open and running...

     Sequence                            Expected behavior
Crash>Activate                           Restore session
Crash>Reboot>Activate                    Restore session
Suspend>Activate                         Restore session
Suspend>Close>Activate                   Fresh session
Suspend>Reboot>Activate                  Fresh session
Suspend>Terminate>Activate               Restore session
Suspend>Terminate>Close>Activate         Fresh session
Suspend>Terminate>Close>Reboot>Activate  Fresh session
Suspend>Terminate>Reboot>Activate        Fresh session
Close>Activate                           Fresh session
Close>Reboot>Activate                    Fresh session
Reboot>Activate                          Fresh session

Notes:
  - "Terminate" means that the app was successfully suspended and that, due to resource constraints, Windows terminated the process. This is not the same as a crash and is not the same as using "End Task." To cause this to happen, you must suspend metroFx and then launch an app that consumes all remaining memory (I use testlimit.exe from the sysinternals suite)
  - "Crash" can be accomplished by using the "Crash Me" addon
  - After "Close" you might have to wait 10s or else we might restore the session. This is actually in line with MSFT design docs and shouldn't be regarded as a bug (it's crazy, I know)
  - "Suspend" means to put metroFx in the background (so it's not visible on any screen) and wait the 5s or so for it to be suspended

The rule of thumb is this: If the user closed the app or restarted (regardless of what state the app was in), we should start a fresh session. Otherwise we should restore.

I should also point out that I don't like the "fresh session" behavior because it can lead to data loss if a user moves her/his session from desktopFx to metroFx and then closes metroFx. There's a discussion going on in the metro mailing list about whether we should change that behavior.
Flags: needinfo?(tabraldes)
Depends on: 957501
OS: Windows 8 Metro → Windows 8.1
Tested on latest Nightly and latest Aurora builds.
Using a dirty profile I'm still able to reproduce this issue. I was able to verify the fix only with clean profiles.

> There's a discussion going on in the metro mailing list about whether we should change that behavior.
Did this behavior changed?
Flags: needinfo?(tabraldes)
(In reply to Cornel Ionce [QA] from comment #31)
> Tested on latest Nightly and latest Aurora builds.
> Using a dirty profile I'm still able to reproduce this issue. I was able to
> verify the fix only with clean profiles.

The dirty profile is probably experiencing bug 959396. I think the verification on a clean profile is enough. Thanks!
 
> > There's a discussion going on in the metro mailing list about whether we should change that behavior.
> Did this behavior changed?

No, the discussion is still ongoing.
Flags: needinfo?(tabraldes)
Marking this verified fixed based on comment 32.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.