630723 - Deferred session restore doesn't work with browser.session_restore.resume_from_crash = false

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

After downloading the addon-sdk, creating a stub add-on with cfx init, and then running it with cfx run, the profile used to use it is not recovering session restore data.

more info:
Session restore works within the session, and the data appears to be correctly saved to sessionstore.js. The .bak file created on startup is not being updated.

Also interestingly to note is that it works if SS is set to "Show my windows and tabs from last time"

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Ok so this setting is only changed when running cfx; packaged add-ons with cfx does not change them.

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Adding a "this should block" comment because it should. That pref should have nothing to do with the ability to restore the session after a normal quit.

I thought I had done this at some point in bug 588482 but that's obviously not the case.

Patch coming shortly.

Anthony, can you make sure we have a litmus test for this?

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

The problem is that we were still bailing out if !resumeFromCrash && !autoResume - it didn't take into account the change due to deferred session restore.

Comment 4

[Dietrich Ayala (:dietrich)]

Comment on attachment 509162 [details] [diff] [review]
Patch v0.1

is resumeFromCrash unused in that code now? if so remove it. if not, move it to where it's used.

r=me, but do a try run before checking in to ensure we're not defeated the awesome fragility of session restore.

Comment 5

[u279076]

(In reply to comment #2)
> Anthony, can you make sure we have a litmus test for this?

Paul, please review:
https://litmus.mozilla.org/show_test.cgi?id=15077

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #5)
> (In reply to comment #2)
> > Anthony, can you make sure we have a litmus test for this?
> 
> Paul, please review:
> https://litmus.mozilla.org/show_test.cgi?id=15077

I actually meant the normal quitting case (same pref value and expected outcome though). Testing the crash case is also important though, so I would keep that.

Comment 7

[u279076]

(In reply to comment #6) 
> I actually meant the normal quitting case (same pref value and expected outcome
> though). Testing the crash case is also important though, so I would keep that.

Paul, added another test for the normal-quit scenario; please review:
https://litmus.mozilla.org/show_test.cgi?id=15078

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #7)
> Paul, added another test for the normal-quit scenario; please review:
> https://litmus.mozilla.org/show_test.cgi?id=15078

hmmm. Let's actually avoid mentioning crashing & the previous litmus case about crashing. Things get a little weird because we end up reading an old session from disk. I'll make sure to file that.

(In reply to comment #4)
> r=me, but do a try run before checking in to ensure we're not defeated the
> awesome fragility of session restore.

Try did fine.

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/d95c8766b4a8 with the changes requested. I also filed bug 632267 about the old session thing I mentioned in comment 8.

Comment 10

[u279076]

(In reply to comment #8)
> hmmm. Let's actually avoid mentioning crashing & the previous litmus case about
> crashing. Things get a little weird because we end up reading an old session
> from disk. I'll make sure to file that.

That's not really a valid justification for NOT having a testcase.  On the contrary, it's the exact reason we should have a testcase.  Or maybe I missed something in what you said? What's your reasoning why we shouldn't have a testcase for the "crash" scenario?

Comment 11

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Well, this bug is not about crashing. So I don't think the testcase should - because that's testing something different (I know I originally said otherwise, but I was wrong)

If we have a test written for the crash case that says we show the last last session, then fine. But it's just testing that a bug exists. And then we'll have to mark it failing / delete it later. We already have the bug on file so we know it's a problem.

Comment 12

[u279076]

(In reply to comment #11)
> Well, this bug is not about crashing. So I don't think the testcase should -
> because that's testing something different (I know I originally said otherwise,
> but I was wrong)

This statement confuses me because the bug is about the following pref:
browser.session_restore.resume_from_crash

Does that not imply crashing?

Comment 13

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #12)
> (In reply to comment #11)
> > Well, this bug is not about crashing. So I don't think the testcase should -
> > because that's testing something different (I know I originally said otherwise,
> > but I was wrong)
> 
> This statement confuses me because the bug is about the following pref:
> browser.session_restore.resume_from_crash
> 
> Does that not imply crashing?

No it doesn't imply crashing. I'm sorry that wasnt clear.

That pref was affecting normal quit -> restore previous session use. A value of false was causing us to disable restore previous session following a normal quit, when it shouldnt have any effect unless the session crashed.

Comment 14

[u279076]

Verified fixed and test cases have been revised.