Closed Bug 919532 Opened 11 years ago Closed 11 years ago

Session does not restore after restarting in safemode

Categories

(Firefox :: Session Restore, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
Firefox 27
Tracking Status
firefox24 --- unaffected
firefox25 + verified
firefox26 + verified
firefox27 + verified

People

(Reporter: bmaris, Assigned: ttaubert)

References

()

Details

(Keywords: dataloss, regression)

Attachments

(2 files)

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.
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.
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
(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.
Flags: needinfo?(bogdan.maris)
Marking this as critical due to data loss (loss of session).
Severity: normal → critical
Keywords: dataloss
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.
(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.
Flags: needinfo?(bogdan.maris)
(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.
Status: NEW → ASSIGNED
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.
Attachment #812511 - Flags: review?(mnoorenberghe+bmo)
Blocks: 294260
(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?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
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.
Attachment #813683 - Flags: review?(dteller)
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.
Attachment #813683 - Flags: review?(dteller) → review+
(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.
Attachment #813683 - Flags: checkin+
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!
Flags: needinfo?(dteller)
I still see a r?, what's the status of that?
Flags: needinfo?(dteller)
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.
Attachment #813683 - Flags: approval-mozilla-beta?
Attachment #813683 - Flags: approval-mozilla-aurora?
Attachment #812511 - Flags: review?(mnoorenberghe+bmo) → review+
Attachment #813683 - Flags: approval-mozilla-beta?
Attachment #813683 - Flags: approval-mozilla-beta+
Attachment #813683 - Flags: approval-mozilla-aurora?
Attachment #813683 - Flags: approval-mozilla-aurora+
Whiteboard: [leave open] → [leave open][checkin-needed-aurora]
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
(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.
Flags: needinfo?(ttaubert)
Flags: needinfo?(ttaubert)
Whiteboard: [leave open]
Keywords: verifyme
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Verified fixed FF 25b8 Win 7, Ubuntu 13.04, Mac OS X 10.8.4.
Blocks: 912975
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?
Flags: needinfo?(ttaubert)
That must be bug 928630. Thanks for verifying!
Flags: needinfo?(ttaubert)
Verified with Fx 27 build of 20131101
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: