919532 - Session does not restore after restarting in safemode

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Bogdan Maris, Desktop Test Engineering]

Reproducible on Firefox 25 beta 1 (BuildID: 20130922030201)
Mozilla/5.0 (X11; Linux i686; rv:25.0) Gecko/20100101 Firefox/25.0
Reproducible on the latest Aurora (BuildID: 20130922004001)
Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0
Reproducible on the latest Nightly (BuildID: 20130205042010)
Mozilla/5.0 (X11; Linux i686; rv:27.0) Gecko/20100101 Firefox/27.0

Steps to reproduce:
1. Start Firefox.
2. Open 2-3 websites in new tabs.
3. Go to Help/Restart with Add-ons Disabled.
4. Click Restart.
5. Click Start in Safe Mode.

Expected results: Session is restored after the restart.

Actual results: Session is not restored with previously opened tabs.

Notes:
1. This is reproducible on Ubuntu, Mac and Windows.
2. Regression:
Bad: 24 beta 8
Good: 24 beta 7
Bad: 24 beta 6
3. Sometimes it`s reproducible after following steps 3,4,5 multiple times.
4. Attached screencast showing the issue.

Comment 1

[mjh563]

I get this regression range:

Last good nightly: 2013-06-30
First bad nightly: 2013-07-01

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cbb24a4a96af&tochange=d7553251cf43

As the reporter says, you might need to restart in safe mode several times before the bug appears.

Comment 2

[mjh563]

The first bad revision is:
changeset:   136954:086465524970
parent:      136710:52f605debfd4
user:        Tim Taubert
date:        Thu Jun 27 10:32:04 2013 -0400
summary:     Bug 887394 - Don't collect state right after startup when restoring the initial session; r=yoric

Comment 3

[mjh563]

(In reply to Bogdan Maris [QA] [:bogdan_maris] from comment #0)
> 2. Regression:
> Bad: 24 beta 8
> Good: 24 beta 7
> Bad: 24 beta 6

Are you able to reproduce this in the release version of Firefox 24?

I wasn't able to reproduce it in 24 (I tested with beta 10). And I'm pretty sure it's caused by bug 887394, which only landed in 25.

Comment 4

[mjh563]

Marking this as critical due to data loss (loss of session).

Comment 5

[Alex Keybl [:akeybl]]

Tim - since bug 887394 appears to be just a performance win, we may want to just back that out for FF25. We don't want to introduce new session restore regressions.

Comment 6

[Bogdan Maris, Desktop Test Engineering]

(In reply to mjh563 from comment #1)
> I get this regression range:
> 
> Last good nightly: 2013-06-30
> First bad nightly: 2013-07-01
> 
> Pushlog:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=cbb24a4a96af&tochange=d7553251cf43

That`s the regression I`m getting as well if I run the STR from comment 0 and don`t close Firefox. If I open Firefox with a profile then close Firefox and reopen it with the same profile then following STR I get this regression window:

Last good nightly: 2012-03-20
First bad nightly: 2012-03-21

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b972b89518c3&tochange=4bdae514b9be

(In reply to mjh563 from comment #3)
> (In reply to Bogdan Maris [QA] [:bogdan_maris] from comment #0)
> > 2. Regression:
> > Bad: 24 beta 8
> > Good: 24 beta 7
> > Bad: 24 beta 6
> 
> Are you able to reproduce this in the release version of Firefox 24?
> 
> I wasn't able to reproduce it in 24 (I tested with beta 10). And I'm pretty
> sure it's caused by bug 887394, which only landed in 25.

I can reproduce it on 24 RC and all FF 24 betas but only if I open Firefox with a profile, close Firefox, reopen it and run STR on the same profile.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Alex Keybl [:akeybl] from comment #5)
> Tim - since bug 887394 appears to be just a performance win, we may want to
> just back that out for FF25. We don't want to introduce new session restore
> regressions.

Backing out bug 887394 is not that easy, for a somewhat clean backout we'd have to revert bug 891360, bug 893009, and bug 898184 as well. While that's possible it seems a little risky to me. Another possibility would be to fix this bug by writing a custom patch that backs out bug 887394.

I tracked down what's causing us to lose data when restarting in safe-mode and think we can have an easier and safer solution. I'll attach a patch right away.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Send quit-application-requested before restarting in safe-mode

The problem is that when safe-mode was introduced by bug 294260 we forgot to send application-quit-requested before shutting down the browser. Everything that needs to collect data before shutdown will lose data when it can't collect it anymore once the shutdown has started - like SessionStore.

If you quit early enough before SessionStore has collected data once, we'll write an empty session. This in itself is another bug I'm going to file but aside from misbehaving add-ons we'll most likely never hit that in the wild as all other shutdown paths send the -requested notification before.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to Tim Taubert [:ttaubert] from comment #8)
> The problem is that when safe-mode was introduced by bug 294260 we forgot to
> send application-quit-requested before shutting down the browser. Everything
> that needs to collect data before shutdown will lose data when it can't
> collect it anymore once the shutdown has started - like SessionStore.

Hmm, I didn't think anything important should rely on quit-application-requested. But if it is required, shouldn't we ensure we always fire it at a lower level?

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #9)
> Hmm, I didn't think anything important should rely on
> quit-application-requested. But if it is required, shouldn't we ensure we
> always fire it at a lower level?

Yeah, that was my first thought, too. There might be code paths that don't want to notify before restart/shutdown for whatever reason and those could pass an argument to skip that. Maybe we don't even have any paths like that at all.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Make sure we don't lose state when quitting early without being notified

This patch ensures that when restarting shortly after startup, without being notified, we will not lose the entire session. I have a test that locally verifies this works, we just need a couple of Marionette patches landed before we can land the test.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 813683 [details] [diff] [review]
Make sure we don't lose state when quitting early without being notified

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

That looks good.
As for saving state during quit-application-requested, I suspect that this could be an optimization, to start collection while the user is busy confirming that yes, she wants to quit Firefox.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] <on summit, unavailable until October 8th> from comment #12)
> As for saving state during quit-application-requested, I suspect that this
> could be an optimization, to start collection while the user is busy
> confirming that yes, she wants to quit Firefox.

It might be. It might also be ok to do the last data collection on quit-application-granted as it looks like we will start closing windows afterwards.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/850a18dae809

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/850a18dae809

Comment 16

[Alex Keybl [:akeybl]]

Yoric, while ttaubert is out, what are your thoughts on uplifting the current patch to FF25/26? FF25 (the regressing version) will be released in a matter of weeks. Thanks!

Comment 17

[David Teller [:Yoric] - still alive but not very active]

I still see a r?, what's the status of that?

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 813683 [details] [diff] [review]
Make sure we don't lose state when quitting early without being notified

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 887394
User impact if declined: Certainty of data loss in some common cases (see comment 0).
Testing completed (on m-c, etc.): On m-c since October 7th.
Risk to taking this patch (and alternatives if risky): Small.
String or IDL/UUID changes made by this patch: No string or API change.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1e701acdecca
https://hg.mozilla.org/releases/mozilla-beta/rev/9d1f64572959

Should this still be open?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

This was backed out from beta while investigating some failures that appeared on the push including it and was re-landed after being cleared.
https://hg.mozilla.org/releases/mozilla-beta/rev/b69efba9154a

Comment 21

[David Teller [:Yoric] - still alive but not very active]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/1e701acdecca
> https://hg.mozilla.org/releases/mozilla-beta/rev/9d1f64572959
> 
> Should this still be open?

We'll wait until Tim returns from PTO to determine that.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/5ce9695a5c60

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5ce9695a5c60

Comment 24

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 25b8 Win 7, Ubuntu 13.04, Mac OS X 10.8.4.

Comment 25

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0

Verified as fixed on Windows 8 x64 and Mac OS X 10.9 on latest Aurora 26.0a2 (buildID: 20131023004005) and latest Nightly 27.0a1 (buildID: 20131022030202)
On Ubuntu 12.04 I`m getting something strange though (tested on 3 different machines):

STR:
1. Start Firefox.
2. Open 2-3 websites in new tabs.
3. Focus one tab opened at step 2.
4. Go to Help/Restart with Add-ons Disabled.
5. Click Restart.
6. Click Start in Safe Mode.

Actual Results: The focused tab is changed to about:home after the restart but the other tabs are reloaded successfully.

Notes: Sometimes the first tab is changed to about:home regarding the focus.
Any thoughts?

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

That must be bug 928630. Thanks for verifying!

Comment 27

[Tracy Walker [:tracy]]

Verified with Fx 27 build of 20131101