490040 - Reattaching a lone tab into another window causes an empty window to be added to Recently Closed Windows

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

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

STR:
0. Look at Recently Closed Windows menu
1. Open 2 windows (A & B). Make sure window B only has one tab.
2. Drag the lone tab from window B to window A
3. Recently Closed Windows now has a menu entry for (Untitled).

Expected Behavior:
No new entry is added to the menu.

Proposed Solution:
One possibility is to just ignore empty windows in session store, though this could possibly cause issues with the win/linux need for a non-popup window.

Comment 1

[Simon Bünzli]

(In reply to comment #0)
> One possibility is to just ignore empty windows in session store

This would also fix Ctrl+N, Ctrl+W, Ctrl+Shift+N from reopening a useless blank window. I'd however not consider blank windows with a non-empty list of recently closed tabs "empty".

Comment 2

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

(In reply to comment #1)
> I'd however not consider blank windows with a non-empty list of
> recently closed tabs "empty".

So it turns out that the lone tab window has a _closedTabs entry for the tab that gets detached. So I think this is really just a side effect of that larger problem.

I'm not sure if there's a bug on file for that, or if we even consider that a bug.

Comment 3

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

CC'ing Mano since he hopefully has a pretty good understanding of the tab detach code...

Rephrasing what I said in comment #2, the problem here is I think part of a larger problem that we're firing "TabClose" when we detach a tab (either detaching into a new window or into another). SessionStore is listening for "TabClose" and so that detached tab gets added to our "Recently Closed Tabs" list.

Right now that's just a minor annoyance. But if we want to use the recently closed tabs as a determinant of an "empty" window, then it becomes critical.

So I'm not sure if there's a way we can fire a different event ("TabDetached" perhaps?), or if we have to work around this, or something else...

Thoughts?

Comment 4

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

Attachment: Patch v0.1 (WIP)

So with the patch from bug 491431 applied (assuming gavin continues thinking that one is ok), this is relatively simple. It also happens to fix the broader problem covered in comment #3.

(In reply to comment #1)
> I'd however not consider blank windows with a non-empty list of
> recently closed tabs "empty".

Right now this get's handled poorly (IMO). Those windows are restorable, but it restores to a blank page from which you can restore closed tabs. I would say that it's enough of an edge case that it's not a big deal, but I feel like the majority of cases people are going to have some closed tabs before dragging out.

I guess we could open the last closed tab when the window gets restored with no open tabs, but that's not really "expected" behavior.

Comment 5

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

(In reply to comment #4)
> Right now this get's handled poorly (IMO). Those windows are restorable, but it
> restores to a blank page from which you can restore closed tabs. I would say
> that it's enough of an edge case that it's not a big deal, but I feel like the
> majority of cases people are going to have some closed tabs before dragging
> out.
> 
> I guess we could open the last closed tab when the window gets restored with no
> open tabs, but that's not really "expected" behavior.

An idea that's been suggested in IRC - merge the recently closed tabs list from the closing window with the open window. I like the idea, but it's going to need us adding timestamps to closed tabs (not a big deal, but something to note). It's not a perfect solution, but perhaps the most appropriate.

Comment 6

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

Ok, so it seems like the solution that seems least bad is to just get rid of the window when it's "closed" via detaching the final tab. I'm fine with that, Beltzner & others seemed fine with that, so that's what I'm going to do. If we think there's really going to be contention around that, we can make it a pref (which when set to true would just allow us to store those windows, but wouldn't make any special attempts - essentially the behavior of the patch as it is right now)

Comment 7

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

Attachment: Patch v0.2

Updated to reflect my previous comments & tests.

Comment 8

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

Comment on attachment 377042 [details] [diff] [review]
Patch v0.2

Realized that this still had some debug code in there. I'd still appreciate a fly-by review.

Comment 9

[Simon Bünzli]

Comment on attachment 377042 [details] [diff] [review]
Patch v0.2

Please make sure to not leave debugging code in the test, either. And please drive in the patch for bug 491431 first...

>+          // aEvent.detail determines if the tab was closed by dragging to a different window

AFAICT this won't be specific to "dragging" but rather to "moving" (through whatever means).

>+      // if the window is eligible for restoration, save to undo list

Nit: Why not state the criteria for (or better instead of) "eligible for restoration"?

>+  let obs = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);

Nit: We usually call services by their initials: os

>+  let wm = Cc["@mozilla.org/appshell/window-mediator;1"].getService(Ci.nsIWindowMediator);

Nit: Unused?

>+  function testWithState(state, shouldBeAdded, callback) {

Nit: JavaScript arguments have an "a" prefix.

>+      ss.setWindowState(theWin, JSON.stringify(state), true);
>+
>+        let observer = {

Nit: Indentation.

>+              is(closedWindowCount == curClosedWindowCount + 1, shouldBeAdded,

Failures will be easier to debug if you compare |closedWindowCount| with |curClosedWindowCount + (shouldBeAdded ? 1 : 0)|.

>+  let state_3 = {

Nit: Unused?

>+  testWithState(state_0, true, function() {

For clarity, I'd prefer having the shouldBeAdded boolean as a property of the state.

Comment 10

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

(In reply to comment #9)
> (From update of attachment 377042 [details] [diff] [review])
> Please make sure to not leave debugging code in the test, either. And please
> drive in the patch for bug 491431 first...

Yea, my mistake. I realized that I still had debugging code in there as soon as I attached it. I also realized I had some other cruft in there which you mentioned. Sorry about that.

> >+          // aEvent.detail determines if the tab was closed by dragging to a different window
> 
> AFAICT this won't be specific to "dragging" but rather to "moving" (through
> whatever means).

Comment fixed

> >+      // if the window is eligible for restoration, save to undo list
> 
> Nit: Why not state the criteria for (or better instead of) "eligible for
> restoration"?

This was sort of left over from before it was really determined what "eligible" meant. I updated the comment to make it clearer.

> >+              is(closedWindowCount == curClosedWindowCount + 1, shouldBeAdded,
> 
> Failures will be easier to debug if you compare |closedWindowCount| with
> |curClosedWindowCount + (shouldBeAdded ? 1 : 0)|.

Makes sense

> >+  let state_3 = {
> 
> Nit: Unused?

Using it now. I wanted another state that was slightly different (in case the definition of "eligible" changed to include windows with extData, there would already be a test case ready)

> >+  testWithState(state_0, true, function() {
> 
> For clarity, I'd prefer having the shouldBeAdded boolean as a property of the
> state.

I updated the code so that testWithState takes an object as such:
{ shouldBeAdded: true, windowState: { windows: [....} }

Thanks for looking at this. I don't know how busy you are but I'll be asking for real review on a new patch as soon as I take care of bug 491431.

Comment 11

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

Attachment: Patch v0.3

Finished this up now that bug 491431 is ready to land.

Comment 12

[Simon Bünzli]

Comment on attachment 377523 [details] [diff] [review]
Patch v0.3

Looking good. Only thing we might to reconsider:

>+      if (!(winData.tabs.length == 1 &&
>+            winData.tabs[0].entries.length == 0)) {

AFAICT it could be that winData comes directly through the API and winData.tabs.length might thus be 0. Additionally, I'd prefer not to negate the whole condition, so why not something along the lines of

> if (winData.tabs.length > 1 || winData.tabs.length == 1 && winData.tabs[0].entries.length > 0)

(with proper indentation, of course)? r+=me with this changed.

Comment 13

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

Attachment: Patch v1.0 (for checkin)

Changed as requested in comment #12. I also changed the comment directly above to reflect the change in the if logic

Comment 14

[Dão Gottwald [:dao]]

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

Comment 15

[Dão Gottwald [:dao]]

backed out

TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/sessionstore/test/browser/browser_490040.js | That window should be restorable - Got 1, expected 2

Comment 16

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

Investigating. It seems like a win/linux vs osx problem :(

Comment 17

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

So, locally on Windows this test is passing & one of the private browsing ones  from bug 394759 is failing. That _looks_ due to timing issues with a popup window, but it shouldn't be (test is running in "load" events). Running in OSX with all the ifdefs in sessionstore off, all the tests pass as is.

Still looking...

Comment 18

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

Attachment: Patch v1.1

Just changed how we're running the test event... It seemed to work locally. Try passed these tests but not some unrelated tests... so i'm running it again.

Comment 19

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

How'd those tests go, zpao?

Comment 20

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

X other presumably unrelated tests failed. Didn't have a chance to look at latest try results yet

Comment 21

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

Attachment: Patch v1.2 (if bug 493823 applied)

If patch for bug 493823 is applied first, this will go in cleanly. Making new patch for the more likely case of use not putting that patch in first.

In local cases test from bug 394759 were failing, most likely due to timing issues, and so the window was still in the "empty" state when we were closing it, treating it like a tab detaching window. So the quick (and slightly dirty) fix was to addTab before closing. Granted this test wasn't failing on Try, but oh well.

Comment 22

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

Attachment: Patch v1.2

Try was still failing tests, but it was consistently video tests and not these. So if these tests are still failing, I'm at a loss and it won't make it tonight. (See comment for previous attachment about why there are changes to browser_394759.js)

That said, I'm relatively confident about this. I played with it actually built in Windows and it behaved in real use as the test is supposed to mimic.

Comment 23

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

Sucks, but not going to hold the release for this. Would take it if we can figure out why it's pooching tests.

Comment 24

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

I can't make tests fail locally with this or make tryserver fail these tests, so I think it's ok to go back in. The original check-in failed only on tests inside this patch and I'm pretty sure it was directly related to the use of setTimeout, which has been taken out.

Simon, can you take a quick look at the changes to make sure it's ok? The comment in bug_394759.js may be a bit excessive, but it's a test so I figured it was ok.

Comment 25

[Simon Bünzli]

Comment on attachment 378781 [details] [diff] [review]
Patch v1.2

>         window.gBrowser.addEventListener("load", function(aEvent) {
>+          // the window _should_ have state with a tab of url, but it doesn't
>+          // always happend before window.close(). addTab ensure we don't treat
>+          // this window as a stateless window
>+          window.addTab();

Could it be that you're picking up the load event for about:blank before the actual URL is loaded? If so, check for that, return and just wait for the next load event. Of course, it would help if you tried to load a URL that actually resolves...

(Besides: window.addTab isn't a function.)

Comment 26

[Nochum Sossonko [:Natch]]

I think you want | theWin.gBrowser.selectedTab.linkedPanel.addEventListener |.

Comment 27

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

Attachment: Patch v1.3 (for checkin)

(In reply to comment #25)
> Could it be that you're picking up the load event for about:blank before the
> actual URL is loaded? If so, check for that, return and just wait for the next
> load event. Of course, it would help if you tried to load a URL that actually
> resolves...

Possible, but I thought that's what the second load listener was for. Also, I tried it with a page that actually resolves and the problem was still there. Again, that test only seemed to happen locally & on Windows.

Comment 28

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

Paul was gonna toss this through tryserver one more time to see if the tests passed ...

Comment 29

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

from irc "try timed out on windows with it. i ran mochitests ~8 times here and can't reproduce. linux failed a crash test (known failure), and osx failed ref test (known) and a mochitest (known)"

Comment 30

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/7c0aff415a01

Comment 31

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

This cycled green so maybe since we're not quite frozen we can still take this on 3.5?

Comment 32

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

Comment on attachment 379200 [details] [diff] [review]
Patch v1.3 (for checkin)

a191=beltzner

I think the value of fixing this outweighs the cost of the new intermittent [orange] in bug 494749, and Paul promised me that he'd work with someone like sdwilsh to see if he can figure out a way to squash that.

Comment 33

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/548172a6d389

Comment 34

[Dão Gottwald [:dao]]

The test times out every single time on branch. -> backed out

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090525 Minefield/3.6a1pre ID:20090525031110

Comment 36

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

(In reply to comment #34)
> The test times out every single time on branch. -> backed out

Turns out this is because the pref service is throwing. Bug 487059 never landed on branch :(

Comment 37

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

Attachment: Patch v1.3 (for checkin m191)

For branch. I still hit the intermittent timeouts of bug 494749 but nothing with any consistency like this was timing out.

Comment 38

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/648de96b2d71

Comment 39

[Aakash Desai [:aakashd]]

verified FIXED on Shiretoko:


Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090527 Shiretoko/3.5pre ID:20090527031214