Closed Bug 608223 Opened 9 years ago Closed 9 years ago

Tab groups not restored after Force Quit / Crash

Categories

(Firefox Graveyard :: Panorama, defect, P3)

defect

Tracking

(blocking2.0 betaN+)

VERIFIED FIXED
Firefox 4.0b10
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: aravind, Assigned: raymondlee)

References

Details

(Whiteboard: [session-store-testday][softblocker])

Attachments

(1 file, 2 obsolete files)

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b8pre) Gecko/20101028 Firefox/4.0b8pre

Steps to reproduce : 
1. Create a new profile
2. Open around 50 sites
3. Go to Panorama View (Ctrl + E)
4. Create 2 new tabs groups.
5. Drag and drop few tabs in both the groups.
6. Force quit Minefield
7. Start Minefield

Actual results : 
Tab groups which are created before force quit are not restored. Newly created tab groups are lost with all the tabs moved to Main tab group or Sometimes shows a blank tab group with all the tabs in Main group. 

Expected results :
Tab groups should be restored with their respective tabs after session restore.

Able to reproduce this issue in Mac OSX 10.5.8, Windows platforms and Linux (Ubuntu 9.10).

Screencast of the issue :
http://www.screencast-o-matic.com/create?step=info&sid=1288345266&itype=choose
We don't save session state immediately for most actions. That would cause a number of issues. So we have a timer and will save state when needed but no sooner than 15s (by default) after the previous save.

We've deemed this interval good enough to restore a usable state without causing slowdowns in browsing.

So you're absolutely correct that crashing immediately after doing something might not get saved. In this case that new data is your tab groups. In other cases it might be the tab you just opened or some text you wrote in a form.

I'd be willing to bet things would be restored if you let Firefox sit for ~20s after creating your groups and setting them up, then crashed. If that's not the case, or if quitting normally doesn't cause group data to be saved, that's a legitimate issue.
OS: Mac OS X → All
Hardware: x86 → All
Summary: Tab groups not restored after Session store → Tab groups not restored after Force Quit / Crash
When i am trying to force quit the browser, it is not saving the tab group sets while restoring firefox. All the tabs are opened in single tab group. Other tab group sets are deleted.
(In reply to comment #2)
> When i am trying to force quit the browser, it is not saving the tab group sets
> while restoring firefox. All the tabs are opened in single tab group. Other tab
> group sets are deleted.

This is after waiting the ~20s before force quitting?
I'm able to reproduce this problem on a Linux machine with 10.10 64bit (hardware), and the latest nightly. I opened all the news headlines (from bookmarks menu) in tabs, which were about 70. It took a while to load and it slowed down the browser. When it was a little bit responsive I went in to Panorama, and created a couple of groups and dragged tabs on to them.

I waited a couple of minutes, and force quit the application.

When it came back only the main group shows. The tab candy instructional video came back, too (even though I had closed it prior to force quitting). The tabs that I had dragged onto those two groups disappeared with them.

Able to reproduce.
blocking2.0: --- → ?
Blocking. We should not let users do these management tasks if we're not ready to persist them. I think this kind of falls into a category of dataloss bug that I'm seeing a lot of recently, more like "state loss".
blocking2.0: ? → betaN+
From the session restore side, we call saveStateDelayed from setWindowValue and setTabValue. So we should be saving the value within the next ~15s.

Ian, I would assume Panorama is calling those methods immediately and not on a timer. Any chance they are throwing when you call them? (I think setWindowValue can but it really shouldn't be...)
Panorama sets values to sessionstore immediately when they change. I suppose it's possible that routine could be throwing, but then it wouldn't work when you quit normally, and it does. 

However, upon looking through the code it appears we're not updating a tab's parent group when it gets added; that may not get saved until the application quits. That definitely needs to get fixed. 

Another thing we don't do until application quit is saving out the thumbnail cache. I'm more on the fence about whether we should change that to periodic saves... it's a lot of data churn, and losing some thumbnails isn't that big a deal (though I suppose it could be more so when we start indefinitely delaying the load of pages in background groups).

The "first-run" video is controlled by both a pref and some information in sessionstore; if either of them are absent, the video will show. It looks like the sessionstore side is being updated immediately... is there a known issue with prefs not being remembered after a crash?

At any rate, sounds like this is a tab candy issue... shall we switch the component?
(In reply to comment #7)
> Panorama sets values to sessionstore immediately when they change. I suppose
> it's possible that routine could be throwing, but then it wouldn't work when
> you quit normally, and it does. 

True. I didn't really suspect sessionstore throwing since it can really only happen if you try to set data on a window that isn't tracked yet.

> However, upon looking through the code it appears we're not updating a tab's
> parent group when it gets added; that may not get saved until the application
> quits. That definitely needs to get fixed. 

This sounds like the problem...

> Another thing we don't do until application quit is saving out the thumbnail
> cache. I'm more on the fence about whether we should change that to periodic
> saves... it's a lot of data churn, and losing some thumbnails isn't that big a
> deal (though I suppose it could be more so when we start indefinitely delaying
> the load of pages in background groups).

Should be a different bug, but I don't think that it needs to be done more often (right now anyway). Regardless, I would imagine you could keep churn to a minimum (only need to update for tabs that have changed)
Component: Session Restore → TabCandy
QA Contact: session.restore → tabcandy
Sounds like something we should try and get into b8. Marking as such.
Blocks: 597043
Priority: -- → P3
Assignee: nobody → raymond
(In reply to comment #8)
> (In reply to comment #7)
> > Panorama sets values to sessionstore immediately when they change. I suppose
> > it's possible that routine could be throwing, but then it wouldn't work when
> > you quit normally, and it does. 
> 
> True. I didn't really suspect sessionstore throwing since it can really only
> happen if you try to set data on a window that isn't tracked yet.
> 
> > However, upon looking through the code it appears we're not updating a tab's
> > parent group when it gets added; that may not get saved until the application
> > quits. That definitely needs to get fixed. 
> 
> This sounds like the problem...
> 

I've checked the code and we actually do call sessionStore.setTabValue when a tab item is moved from one group to another or a new tab item is created.

GroupItem_add() => TabItem_save() => Storage_saveTab() => sessionStore.setTabValue()

There might be something else which doesn't save the session store after the sessionStore.setTabValue() is called and the 15seconds delay.
Whiteboard: [session-store-testday]
Blocks: 598154
Beta8 has 1 bug left on it, moving our blockers to b9
No longer blocks: 597043
bugspam (moving b9 to b10)
Blocks: 608028
bugspam (removing b9)
No longer blocks: 598154
Status: NEW → ASSIGNED
Whiteboard: [session-store-testday] → [session-store-testday][softblocker]
Attached patch v1 (obsolete) — Splinter Review
The problem is that the "experienced_first_run" pref isn't saved to file before a force quit and crash happens.

The patch saves preferences to file when "experienced_first_run" pref is set to true. Also, stores the "tabview-visibility" to session store when entering and existing the tabview UI so when a crash happens, we can still restore on next start.

I am not sure how to write a test to stimulate, restart Fx and check the "experienced_first_run" pref.  @Ian: any suggestions?
Attachment #503429 - Flags: review?(ian)
(In reply to comment #14)
> I am not sure how to write a test to stimulate, restart Fx and check the
> "experienced_first_run" pref.  @Ian: any suggestions?

Doesn't seem like the sort of thing you can automate. Probably just need to flag it with "litmus" for manual testing, unless anyone else in this thread has suggestions?
Based on comment 15, adding in-litmus? and assigning to myself.
Flags: in-litmus?(anthony.s.hughes)
Comment on attachment 503429 [details] [diff] [review]
v1

>+  VISIBILITY_IDENTIFIER: "tabview-visibility",

Seems unfortunate to now have two copies of this string constant. Can UI grab the constant from the browser's TabView?

>+      this._sessionstore =
>+        Cc["@mozilla.org/browser/sessionstore;1"].
>+          getService(Ci.nsISessionStore);

So far we've kept all of our storage access in storage.js... seems like we should continue to do so.

Otherwise looking good.
Attachment #503429 - Flags: review?(ian) → review-
Attached patch v1 (obsolete) — Splinter Review
Attachment #503429 - Attachment is obsolete: true
Attachment #503967 - Flags: review?(ian)
Turns out I had already created a testcase for this scenario:
https://litmus.mozilla.org/show_test.cgi?id=13857

It does not test the performance scenario of "50 tabs" but it does test the core "restoring tab groups" functionality.  Please let me know if this is good enough.
softblocker = critical
Severity: normal → critical
Severity: critical → normal
(In reply to comment #19)
> Turns out I had already created a testcase for this scenario:
> https://litmus.mozilla.org/show_test.cgi?id=13857
> 
> It does not test the performance scenario of "50 tabs" but it does test the
> core "restoring tab groups" functionality.  Please let me know if this is good
> enough.

The "50" tabs scenario is not necessary because I can replicate the bug with just few tabs.  However, rhere would be some small changes to the testcase when the patch lands.

Steps to Perform:
1. Open a new Firefox profile
2. Set Preferences > General > When Firefox Starts to "Show windows and tabs"
3. Open various websites in multiple tabs
4. Open the Panorama view and create 2 more tabgroups by dragging tabs
5. Kill Firefox (Force to quite Firefox)
6. Start Firefox

Expected Results:
* When Firefox restarts, the Panorama view should be opened automatically and you should restore to the tabgroup you were last viewing
* You should be able to see all the tabgroups in the panorama view
* You should be able to display one of the other tabgroups
(In reply to comment #19)
> Turns out I had already created a testcase for this scenario:
> https://litmus.mozilla.org/show_test.cgi?id=13857
> 
> It does not test the performance scenario of "50 tabs" but it does test the
> core "restoring tab groups" functionality.  Please let me know if this is good
> enough.

Should be fine, thanks!
(In reply to comment #22)
> (In reply to comment #19)
> > Turns out I had already created a testcase for this scenario:
> > https://litmus.mozilla.org/show_test.cgi?id=13857
> > 
> > It does not test the performance scenario of "50 tabs" but it does test the
> > core "restoring tab groups" functionality.  Please let me know if this is good
> > enough.
> 
> Should be fine, thanks!

... with Raymond's updated steps, of course :)
Comment on attachment 503967 [details] [diff] [review]
v1

Looks good!
Attachment #503967 - Flags: review?(ian) → review+
Attachment #503967 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/ada1e6804b8c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b10
verified with minefield build of 20110217
Status: RESOLVED → VERIFIED
Added test case in Litmus: 15224 - Restore tab groups after Force Quit / Crash
Flags: in-litmus?(anthony.s.hughes) → in-litmus+
(In reply to comment #28)
> Added test case in Litmus: 15224 - Restore tab groups after Force Quit / Crash

George, can you please provide a link to the testcase. Zpao or Ian should review it to make sure it makes sense as per the patch.
The link for the newly created testcase:

https://litmus.mozilla.org/show_test.cgi?id=15224
(In reply to comment #30)
> The link for the newly created testcase:
> 
> https://litmus.mozilla.org/show_test.cgi?id=15224

I think it makes sense from my side.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.