625257 - Undo Close Tab does not work for not-yet-loaded tab

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[u279076]

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

If you close a tab before it finishes loading completely it is not restored, contrary to the above testcase.

Here are the results given different tab states at closing:
1. Connecting - New Tab in Recently Closed Tabs, opens a blank new tab
2. Loading - Page Title in Recently Closed Tabs, opens a blank new tab

Steps to reproduce:
1. CTRL/CMD+N to open a new tab
2. Navigate to www.apple.com
3. CMD/CTRL+W to close the tab
4. History > Recently Close Tabs
5. Click the entry for the tab

Comment 1

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

Oh hey that's a fun regression.

This should block something. IANAD but at this point I'd block .x but I'd be ok putting this into a release if it means we can ship faster. It's hidden enough.

Comment 2

[u279076]

This regressed between B6 and B7.  I'm working to narrow this down further.

Comment 3

[u279076]

This regressed between 20101130 and 20101201.  I'll see if I can get a changelog.

Comment 4

[u279076]

Nothing stands out to me as a regressing changeset.

Comment 5

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

Can you paste the changelog so that someone else can look, then?

I think this is something we can and should fix in a follow up release. Pretty edge.

Comment 6

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

See also: bug 625398 (very similar maybe dupe). Mentioning so I remember to check that when this gets fixed.

Comment 7

[Luke Iliffe (Harlequin99)]

I haven't verified the regression range in comment 3 but based on the dates given here are the changesets (taken from the firefox-4.0b8pre.en-US.win32.txt files). 

20101130030317 837d7b346a64
20101201030318 d48a0e23feec

The corresponding pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=837d7b346a64&tochange=d48a0e23feec

Comment 8

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

Almost definitely a regression from bug 608037 based on that range.

Comment 9

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

Mehdi volunteered to look at this next. Go Mehdi!

Comment 10

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #0)
> https://litmus.mozilla.org/show_test.cgi?id=11343
> 
> If you close a tab before it finishes loading completely it is not restored,
> contrary to the above testcase.
> 
> Here are the results given different tab states at closing:
> 1. Connecting - New Tab in Recently Closed Tabs, opens a blank new tab
> 2. Loading - Page Title in Recently Closed Tabs, opens a blank new tab
> 
> Steps to reproduce:
> 1. CTRL/CMD+N to open a new tab
> 2. Navigate to www.apple.com
> 3. CMD/CTRL+W to close the tab
> 4. History > Recently Close Tabs
> 5. Click the entry for the tab

I am unable to reproduce the first case with a 2011-01-13 build on Mac and Windows. "New Tab" shows up in Recently Closed Tabs and it opens the tab and finishes loading. Is there an easy way to replicate the second case or to fake a slow connection?

Comment 11

[u279076]

Slow connection is not necessary...I was able to reproduce this fairly easily and repeatedly on a 25Mbps cable connection.

Comment 12

[u279076]

You might have more luck if you try for a domain which you have never loaded before.

Comment 13

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

Quick look when I reproduced this...

Looking at the closed tab data when reproducing this shows that the tab data only has one entry: about:blank (presumably the transient entry). Mehdi is looking into it further.

(In reply to comment #8)
> Almost definitely a regression from bug 608037 based on that range.

Doesn't seem likely based on what's happening but not going to rule it out completely.

Comment 14

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch v1 with test.

Fix as per discussion with zpao.

We found a reliable way to reproduce this bug:
1. Open a new tab and wait 15 seconds for a session store to kick in.
2. Type in a URL, hit enter.
3. When the title shows up in the tab, immediately close the tab.

At that point, trying to restore the closed tab simply loads about:blank.

Anthony: I'm not sure if we hit the exact case you were running into, could you try running the builds in http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-4f987aacbeaf to see if you can still reproduce the bug?

Comment 15

[u279076]

(In reply to comment #14)
> Anthony: I'm not sure if we hit the exact case you were running into, could you
> try running the builds in
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-4f987aacbeaf
> to see if you can still reproduce the bug?

Hmmm...just tried the Linux build on Ubuntu 10.10 and the tab doesn't even list in Recently Closed Tabs now using the STR.

Comment 16

[u279076]

I retried this using the same build on a new profile this morning and everything works as expected:

* a recently closed New Tab can be restored
* a recently closed Loading tab can be restored
* a recently closed Loaded tab can be restored

I'm inclined to say FIXED, but I'll leave that up to you guys.

Comment 17

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

It's still a legitimate bug, even if it's pretty hard to hit. Mehdi can still repro consistently and I can still hit it following his updated STR. His patch makes sense, pending some in-person test changes.

Comment 19

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch v2 with much cleaner test.

Refactored the event listening code, the test seems much easier to understand now.

Comment 20

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

Comment on attachment 505280 [details] [diff] [review]
Patch v2 with much cleaner test.

>+function test() {
>+  waitForExplicitFinish();
>+
>+  // We'll be waiting for session stores, so we speed up their rate to ensure
>+  // we don't timeout.
>+  Services.prefs.setIntPref("browser.sessionstore.interval", 2000);
>+  // Prevent undoCloseTab() from removing a blank tab. (Bug 343895)
>+  Services.prefs.setBoolPref("browser.tabs.autoHide", true);

If you use ss.undoCloseTab directly then you shouldn't need to worry about setting that pref.

>+
>+  gBrowser.addTabsProgressListener(tabsListener);
>+
>+  waitForSaveState(test_bug625257_1);
>+  tab = gBrowser.addTab();
>+
>+  gBrowser.addEventListener("load", onLoad, true);

I'm pretty sure this listener will get load events for wrong things (like the favicon), so instead listen from tab.linkedBrowser. You'd then need to re-add the listener after undoCloseTab (which returns the tab object).

>+let tabsListener = {
>+  onLocationChange: function onLocationChange(aBrowser) {
>+    gBrowser.removeTabsProgressListener(tabsListener);
>+    is(state++, 0, "should be the first listener event");

Please seperate the state++ from the comparison.

>+
>+    // Since we are running in the context of tabs listeners, we do not
>+    // want to disrupt other tabs listeners.
>+    executeSoon(function() {
>+      gBrowser.removeTab(tab);
>+    });
>+  },
>+
>+  onStateChange: function onStateChange(aBrowser) { },
>+  onLinkIconAvailable: function onLinkIconAvailable(aBrowser, aIconURL) { }

You can just leave these off.

>+};
>+
>+function onTabClose(aEvent) {
>+  let uri = aEvent.target.location;
>+
>+  is(state++, 1, "should only remove tab at this point");
>+  gBrowser.tabContainer.removeEventListener("TabClose", onTabClose, true);
>+
>+  executeSoon(undoCloseTab);

You'll need to make this slightly more complicated here to reattach the listener. In fact we might load before you can reattach the listener...

Otherwise looks good.

Comment 21

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch v3, changed load listeners.

Cleaned up tiny test bits and the listeners as per zpao's review.
Attaching the load listener right after calling undoCloseTab seemed to be alright.
With the fix unapplied, the load event for about:blank was also caught as expected.

Running through tryserver to make sure this test does not break other sessionstore stuff, then will check in once tree looks good.

Comment 22

[Mehdi Mulani [:mmm] (I don't check this)]

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

Comment 23

[u279076]

(In reply to comment #22)
> http://hg.mozilla.org/mozilla-central/rev/a01589063ec0

Is there a try-server build I can use to verify, or should I simply wait for the next nightly?

Comment 24

[Mehdi Mulani [:mmm] (I don't check this)]

(In reply to comment #23)
> (In reply to comment #22)
> > http://hg.mozilla.org/mozilla-central/rev/a01589063ec0
> 
> Is there a try-server build I can use to verify, or should I simply wait for
> the next nightly?

For the checked in patch, I only ran a linux-opt build and on a different parent changeset. Unless you can grab mozilla-central builds, I would wait for the nightly.

Comment 25

[u279076]

Verified using Minefield nightly from 2011-01-22.