627642 - Don't restore session into a new window if only the home page is open

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

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

The original work in bug 588482 is pretty good, but it was pretty dumb in determining if a window should be reused.

The most common case now is going to be a single tab (about:home). We can check for a single about:home tab & no extData (which makes the panorama case harder), no recently closed tabs, no history in that single tab. We could probably ignore the extData case (potential for data loss but small) so that entering panorama and pressing restore previous session doesn't suck.

With bug 592822 landing and soon bug 593421 and bug 589665, I think this is going to be a much bigger deal than it was before.

Comment 1

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

I really didn't want to set blocking? on this, but here we are. I really should have realized this was going to be a problem sooner.

Comment 2

[Philip Chee]

With no patch nobody is going to block on this.

Comment 3

[Luke Iliffe (Harlequin99)]

If you have a single app tab the default new session on startup would be the app tab and about:home. When restoring a previous session in this case it is restored to the same window.

Mozilla/5.0 (Windows NT 6.1; rv:2.0b10pre) Gecko/20110121 Firefox/4.0b10pre

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

I need a whiteboard tag for "it hurt to make this decision"

Comment 5

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

whiteboard tag added...

I'm taking this since I wrote the original code and have a good idea of how to do it and I don't like unassigned blockers. If somebody comes along looking for work, they can take it.

Comment 6

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

Attachment: Patch v0.1 (WIP)

quick patch. doesn't close open tabs and doesn't actually do anything special. it just restores into the most recent window if it can (where "can" is defined very loosely right now). That was the easiest thing to get started with.

Only restoring if home pages is harder (not too hard though). I assume in that case of just home pages open we'd want to overwrite that tab so it's exactly the window that was open at shutdown? (in other words, close all the open tab(s) and replace extdata, etc).

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
> it just restores into the most recent window if it can (where "can" is defined
> very loosely right now).

The most recent window created or used? Why not simply use the window from which the user asked to restore the session?

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 507717 [details] [diff] [review]
Patch v0.1 (WIP)

looks ok, r=me. please flag for manual testing.

Comment 9

[Dietrich Ayala (:dietrich)]

(In reply to comment #7)
> (In reply to comment #6)
> > it just restores into the most recent window if it can (where "can" is defined
> > very loosely right now).
> 
> The most recent window created or used? Why not simply use the window from
> which the user asked to restore the session?

The patch checks for the last active browser window, which presumably is the last browser window the user was in. The user could currently have the download manager open and focused.

Comment 10

[Dietrich Ayala (:dietrich)]

(In reply to comment #9)
> The user could currently have the download
> manager open and focused.

Oh, no they couldn't. End result of the patch is still the last active browser window, which is right.

Comment 11

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

(In reply to comment #10)
> (In reply to comment #9)
> > The user could currently have the download
> > manager open and focused.
> 
> Oh, no they couldn't. End result of the patch is still the last active browser
> window, which is right.

No they could on OSX. On Win/Lin you can only get into the history menu from a browser window so that's fine. Regardless, this should be doing the right thing.

(In reply to comment #8)
> Comment on attachment 507717 [details] [diff] [review]
> Patch v0.1 (WIP)
> 
> looks ok, r=me. please flag for manual testing.

You're ok with using the most recent window regardless of what pages are open? That matches what we're doing with windows that had app tabs in them. For the most part that's fine, we'll just end up with a couple extra tabs around.

I originally filed this with the intention of checking home pages, but I'm ok with not checking home pages. I just want to make sure we all are before I land it.

Comment 12

[Ian Gilman [:iangilman]]

Comment on attachment 507717 [details] [diff] [review]
Patch v0.1 (WIP)

just tried it in relation to Panorama; looks good.

Comment 13

[Ian Gilman [:iangilman]]

Comment on attachment 507717 [details] [diff] [review]
Patch v0.1 (WIP)

Oh! I hadn't tested what happens if you create new tabs and new groups before doing a restore (I took the bug's title at face value). Sure enough, it blows away whatever groups you had created in Panorama and shuffles the new tabs into the restored groups; pretty messy. It would definitely be preferable for restore to create a new window in that case. 

Changing to f- because of that. I believe this bug should be implemented as the title suggests.

Comment 14

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

After keying Ian into the ways it breaks for the worse, he's changed his mind. It doesn't look good and newly created group data is going to be all sorts of wonky.

So I'm going to break my rule of not inspecting extData, and look for panorama keys before restoring into a window (including for the whole app tabs dance). If panorama has been activated (tabview-groups exists) and there is more than 1 group (will require parsing tabview-groups) then we'll open into a new window.

I think Ian said this might mean a small change on panorama's end, but he can answer for that.

Comment 16

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

Attachment: Patch v0.2 (WIP)

So this was fun. This closes home pages that are open and in theory only if they are in a single group in panorama.

I didn't actually test the panorama case yet (and if we keep the hack in here, there's something else that should be done I think).

Comment 17

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

Attachment: Patch v0.3 (WIP)

Fixed a couple little things to work with panorama. I think I tested most of the cases now, but I'd like a bit of feedback from Ian to make sure somebody more familiar with that can poke around.

Comment 18

[Ian Gilman [:iangilman]]

Comment on attachment 509871 [details] [diff] [review]
Patch v0.3 (WIP)

The "Panorama has more than one group" detection seems to be working great. 

Restoring into the same window when there are multiple groups in sessionstore, however, often restores all of the tabs to the tab strip, not just the tabs for the current group. Sometimes the tabs are hidden properly, sometimes they're not; I'm not sure what the pattern is. At first I thought it had to do with how many tabs there were, but then I got it to repro with two groups of one tab each. 

Another thing that's broken is that the "next group" key combo doesn't work after restoring but before going into Panorama. This does work just fine if you have "restore on startup" turned on.

Another detail, maybe not a big deal, is that if you were in Panorama when you quit, you should go back into it when you restore. This works with "restore on startup", and it also works when you restore into a fresh window (because the current window has changed enough), but it doesn't work if you restore into the current window.

Comment 19

[Dietrich Ayala (:dietrich)]

Ian, are all of these things covered in tests? It'd be great if Paul could just run the Panorama test suite to know how much this patch regresses. It'd certainly be more efficient than having you review and test manually for each patch rev :)

Comment 20

[Dietrich Ayala (:dietrich)]

Comment on attachment 509871 [details] [diff] [review]
Patch v0.3 (WIP)

Canceling review request given all the Panorama failure this causes.

Comment 21

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

(In reply to comment #18)
> Comment on attachment 509871 [details] [diff] [review]
> Patch v0.3 (WIP)
> 
> The "Panorama has more than one group" detection seems to be working great. 
> 
> Restoring into the same window when there are multiple groups in sessionstore,
> however, often restores all of the tabs to the tab strip, not just the tabs for
> the current group. Sometimes the tabs are hidden properly, sometimes they're
> not; I'm not sure what the pattern is. At first I thought it had to do with how
> many tabs there were, but then I got it to repro with two groups of one tab
> each. 

I'll try to see if I can reproduce this. Reliable STR would be great though.

> Another thing that's broken is that the "next group" key combo doesn't work
> after restoring but before going into Panorama. This does work just fine if you
> have "restore on startup" turned on.

So I'm guessing this is because the keypress event listener is only attached at TabView.init, and only if there is a hidden tab.

This is likely failing for all cases where the window state is set with setWindowState and the keypress listener wasn't already attached (though it is working for the first window during normal session restore... why is that?)

Anywho, the next patch will address that. I'm just calling TabView.init() from restoreWindow. It looks likes that's safe to call even after it's already been called.

> Another detail, maybe not a big deal, is that if you were in Panorama when you
> quit, you should go back into it when you restore. This works with "restore on
> startup", and it also works when you restore into a fresh window (because the
> current window has changed enough), but it doesn't work if you restore into the
> current window.

I would say that's not a big deal. That's likely something to fix on the panorama end anyway. Whatever data we had in extData is being restored. Likely another thing where this never works with setWindowState.

Actually... surprise! calling TabView.init when I do seems to make that work, soooo win!

(In reply to comment #19)
> Ian, are all of these things covered in tests? It'd be great if Paul could just
> run the Panorama test suite to know how much this patch regresses. It'd
> certainly be more efficient than having you review and test manually for each
> patch rev :)

We have the same problem we had before when we landed bug 588482 - this is only really accessible if a session is read at startup and our test machines definitely shouldn't be doing that.

Comment 22

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

Attachment: Patch v0.4 (WIP)

Only change since v0.3 was the call to window.TabView.init()

Comment 23

[(no longer active)]

Is there a good reason that this blocks betaN?  If not, it should be moved over to final+.

Comment 24

[[not reading bugmail]]

I noticed we don't get into this state of creating a new window if you have at least 1 app tab open upon closing the browser.  

For random tabs in tab groups str:

The tab groups issue certainly don't appear to be in their last saved visible state after entering Panorama but were fine after Restore.  

For instance: 

I had 10 tabs open, 5 in one group and 5 in another and 1 app tab open.  
I moved 9 into 1 group so 1 tab was in a single group
closed panorama by selecting the tab in a single group.  
see there is only 1 app tab and 1 tab open
close the browser [X]
Reopen the browser and 1 tab + about:home tab is shown
Hit Restore and it opens in the same window
There is 1 app tab, about:home and 1 tab open
Open Panorama
The tabs are randomly placed between the groups.  
Close Panorama
Notice that Panorama seeming liked to fill up the visible tab group tab bar with enough tabs to cover the visible distance.  

Other than that, it appear random, but really it seems to automagically fill up your visible tab width then overflows the rest.

Comment 25

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

(In reply to comment #23)
> Is there a good reason that this blocks betaN?  If not, it should be moved over
> to final+.

I think the original reason was that this requires some code churn and would be better with some beta coverage. I think it would probably be fine as final though.

Comment 26

[Dietrich Ayala (:dietrich)]

I think this should be in a beta, or wait until the next release to fix it. The regression potential in the interactions between ss and panorama is high, and as the discussion so far has shown, there's potential for dataloss if we get something wrong.

Wrt to the patch - is it possible to move some of that code into a canUseExistingWindow method? megamethod ftl.

Comment 27

[Dietrich Ayala (:dietrich)]

Comment on attachment 511271 [details] [diff] [review]
Patch v0.4 (WIP)

Canceling request, Paul said on IRC that he's cleaning up a new rev of the patch.

Comment 28

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

Attachment: Patch v0.5

Broke that up so it's all easier to read & cleaned it up. I haven't checked that this is still working (it should be - it wasn't majorly changed, just cleaner).

Ian, it would still be good to get feedback from you on this, so if you're feeling well enough, it would be really helpful (since this is a betaN softblocker so if this bug doesn't happen now, it's going to miss Firefox 4)

Comment 29

[Dietrich Ayala (:dietrich)]

Comment on attachment 511778 [details] [diff] [review]
Patch v0.5

patch looks much better, thanks. r=me if there really is no way to write a test for this. given the late change and the historical fragility of this code, please work directly with someone from QA on testing this.

Comment 30

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

(In reply to comment #29)
> Comment on attachment 511778 [details] [diff] [review]
> Patch v0.5
> 
> patch looks much better, thanks. r=me if there really is no way to write a test
> for this. given the late change and the historical fragility of this code,
> please work directly with someone from QA on testing this.

Thanks for reviewing so late :) I'll work with Anthony and Ian to make sure that this gets tested.

Ian: I'm going to get started on it, but I'll followup with you to make sure we have a good set of test cases & expected results in relation to panorama.

Anthony: We'll want some litmus tests here. I know some were added in bug 588482, but we'll want to make sure we cover the additional complexity here.

I'll shoot you guys an email after I do some initial cases so we can make sure everything is covered.

Comment 31

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

Attachment: Patch v1.0 (for checkin)

Caught a missed comma. Ran SS tests to make sure there at least weren't any errors that would catch.

Comment 32

[Phil Ringnalda (:philor)]

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

Comment 33

[Phil Ringnalda (:philor)]

And back out again in http://hg.mozilla.org/mozilla-central/rev/00f2b33bcb60

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1297640995.1297642147.10100.gz

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_privatebrowsing.js | we finish with Tab View not visible
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_rtl.js | Tab View starts hidden
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_rtl.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_search.js | Tab View is hidden
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_rtl.js | Tab View is visible.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/tabview/browser_tabview_undo_group.js | Test timed out

Comment 34

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

Bah. Thanks for landing and then handling it. Sorry for the hassle.

Ian, those failures were most likely related to my use of TabView.init(). Any ideas on making this work?

Comment 35

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

Tim did some great work and found that this is most likely exposing a Panorama bug. He's filing & working on that bug.

Comment 36

[(no longer active)]

(In reply to comment #34)
> Bah. Thanks for landing and then handling it. Sorry for the hassle.
> 
> Ian, those failures were most likely related to my use of TabView.init(). Any
> ideas on making this work?

I bet this is what's kicking in here: <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-tabview.js#78>

IOW, we try to read the tabview-visibility window value from session store and show tab view when restoring a window.  This conflicts with our desire of not showing the tab view after exiting from private browsing.

One possible fix would be to use Storage.saveVisibilityData to set the tabview-visibility property to false when entering the private browsing mode (before saving the current session data), but that seems ugly to me.  Maybe Ian knows of a better fix?

Comment 37

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

Ehsan, Tim has a fix in bug 634077 already which seems to fix the failures for me. But thanks!

Comment 38

[(no longer active)]

(In reply to comment #37)
> Ehsan, Tim has a fix in bug 634077 already which seems to fix the failures for
> me. But thanks!

Great!  This is what I get for writing up something in a bugzilla tab, forgetting about it, coming back a few hours later, submitting and not reading the mid-air page.  :-)

Comment 39

[Ian Gilman [:iangilman]]

Comment on attachment 511857 [details] [diff] [review]
Patch v1.0 (for checkin)

Sorry I'm late to the party (been out sick)! I've now tried the patch, and it's looking good!

Comment 40

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

Comment on attachment 511857 [details] [diff] [review]
Patch v1.0 (for checkin)

We're in this weird softblockers are still blockers but need approval state, so can I can an A+. The blocking bug has landed.

Comment 41

[Dave Townsend [:mossop]]

Comment on attachment 511857 [details] [diff] [review]
Patch v1.0 (for checkin)

Tentative a+ but any sign of regression and this bounces. Please look into writing a mozmill test for this.

Comment 42

[(no longer active)]

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

Comment 43

[AndreiD[QA]]

Works For Me with the homepages "about:home" or "google.com" on: 
Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110216 Firefox/4.0b12pre

But the session restore window is present if cnn.com is set as homepage

Comment 44

[[not reading bugmail]]

Verified Fixed.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110217 Firefox/4.0b12pre ID:20110217030357

Comment 45

[AndreiD[QA]]

Reproduceable on (WinXP):

Mozilla/5.0 (Windows NT 5.1; rv:2.0b12pre) Gecko/20110216 Firefox/4.0b12pre

Comment 46

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

(In reply to comment #43)
> Works For Me with the homepages "about:home" or "google.com" on: 
> Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110216 Firefox/4.0b12pre
> 
> But the session restore window is present if cnn.com is set as homepage

Can you explain further? What session restore window?

Comment 47

[u279076]

Paul, can you help me identify what criteria is needed for the Litmus test?

I could write a test which simply tests "session restore from about:home", but that is generally covered but a few of our session store tests already.

Thanks

Comment 48

[AndreiD[QA]]

When killing firefox (ctr+alt+del), on restart the session restore window appears even if:
- only one tab is open with the homepage (about:home)
OR
- only one apptab is open with the homepage (about:home)

Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110220 Firefox/4.0b12pre

Comment 49

[u279076]

RE: in-litmus?

What is the desired test case here? Is it simply trying to restore a previous session with a about:home loaded in a single tab of single window? Does it involve multiple windows? Is Panorama somehow involved?

Thanks

Comment 50

[u279076]

Paul, any feedback on my previous comment?

Comment 51

[u279076]

Just noticed this very old in-litmus request. Canceling the request. Please re-nom if a test is still needed.