608223 - Tab groups not restored after Force Quit / Crash

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[aravindm]

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

Comment 1

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

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.

Comment 2

[aravindm]

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.

Comment 3

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

(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?

Comment 4

[juan becerra [:juanb]]

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.

Comment 5

[Dietrich Ayala (:dietrich)]

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".

Comment 6

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

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...)

Comment 7

[Ian Gilman [:iangilman]]

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?

Comment 8

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

(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)

Comment 9

[Kevin Hanes]

Sounds like something we should try and get into b8. Marking as such.

Comment 10

[Raymond Lee [:raymondlee]]

(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.

Comment 11

[Kevin Hanes]

Beta8 has 1 bug left on it, moving our blockers to b9

Comment 12

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 13

[Kevin Hanes]

bugspam (removing b9)

Comment 14

[Raymond Lee [:raymondlee]]

Attachment: v1

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?

Comment 15

[Ian Gilman [:iangilman]]

(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?

Comment 16

[Anthony Hughes (:ashughes) [QA]]

Based on comment 15, adding in-litmus? and assigning to myself.

Comment 17

[Ian Gilman [:iangilman]]

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.

Comment 18

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 19

[Anthony Hughes (:ashughes) [QA]]

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.

Comment 20

[Michael Yoshitaka Erlewine [:mitcho]]

softblocker = critical

Comment 21

[Raymond Lee [:raymondlee]]

(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

Comment 22

[Ian Gilman [:iangilman]]

(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!

Comment 23

[Ian Gilman [:iangilman]]

(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 24

[Ian Gilman [:iangilman]]

Comment on attachment 503967 [details] [diff] [review]
v1

Looks good!

Comment 25

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 26

[:ehsan akhgari]

http://hg.mozilla.org/mozilla-central/rev/ada1e6804b8c

Comment 27

[Tracy Walker [:tracy]]

verified with minefield build of 20110217

Comment 28

[George Carstoiu]

Added test case in Litmus: 15224 - Restore tab groups after Force Quit / Crash

Comment 29

[Anthony Hughes (:ashughes) [QA]]

(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.

Comment 30

[George Carstoiu]

The link for the newly created testcase:

https://litmus.mozilla.org/show_test.cgi?id=15224

Comment 31

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

(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.