Closed Bug 630723 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 4.0b12
Tracking Status
blocking2.0 --- .x+

People

(Reporter: Felipe, Assigned: zpao)

References

Details

Attachments

(1 file)

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"
Assignee: nobody → paul
blocking2.0: --- → ?
Summary: Running a jetpack addon once permanently breaks session restore → Deferred session restore doesn't work with browser.session_restore.resume_from_crash = false
Ok so this setting is only changed when running cfx; packaged add-ons with cfx does not change them.
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?
Flags: in-litmus?(anthony.s.hughes)
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
Attached patch Patch v0.1Splinter Review
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.
Attachment #509162 - Flags: review?(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.
Attachment #509162 - Flags: review?(dietrich) → review+
(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
Flags: in-litmus?(anthony.s.hughes) → in-litmus+
blocking2.0: ? → .x
(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.
(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
(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.
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12
(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?
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.
(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?
(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.
Verified fixed and test cases have been revised.
Status: RESOLVED → VERIFIED
Blocks: 640167
You need to log in before you can comment on or make changes to this bug.