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

VERIFIED FIXED in Firefox 4.0b10

Status

()

defect
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: ashughes, Assigned: mmm)

Tracking

({regression})

Trunk
Firefox 4.0b10
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [4b9][softblocker])

Attachments

(1 attachment, 2 obsolete attachments)

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
Whiteboard: [4b9]
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.
blocking2.0: --- → ?
This regressed between B6 and B7.  I'm working to narrow this down further.
This regressed between 20101130 and 20101201.  I'll see if I can get a changelog.
Nothing stands out to me as a regressing changeset.
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.
blocking2.0: ? → .x
See also: bug 625398 (very similar maybe dupe). Mentioning so I remember to check that when this gets fixed.
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
blocking2.0: .x → ?
Almost definitely a regression from bug 608037 based on that range.
Blocks: 608037
blocking2.0: ? → final+
Whiteboard: [4b9] → [4b9][softblocker]
Mehdi volunteered to look at this next. Go Mehdi!
Assignee: nobody → mars.martian+bugmail
(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?
Slow connection is not necessary...I was able to reproduce this fairly easily and repeatedly on a 25Mbps cable connection.
You might have more luck if you try for a domain which you have never loaded before.
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.
Posted patch Patch v1 with test. (obsolete) — Splinter Review
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/mmmulani@uwaterloo.ca-4f987aacbeaf to see if you can still reproduce the bug?
Attachment #504966 - Flags: review?(paul)
(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/mmmulani@uwaterloo.ca-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.
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.
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.
Duplicate of this bug: 604268
Refactored the event listening code, the test seems much easier to understand now.
Attachment #504966 - Attachment is obsolete: true
Attachment #505280 - Flags: review?(paul)
Attachment #504966 - Flags: review?(paul)
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.
Attachment #505280 - Flags: review?(paul) → review+
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.
Attachment #505280 - Attachment is obsolete: true
Attachment #505479 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/a01589063ec0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
(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?
Target Milestone: --- → Firefox 4.0b10
(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.
Verified using Minefield nightly from 2011-01-22.
Status: RESOLVED → VERIFIED
Blocks: 639777
You need to log in before you can comment on or make changes to this bug.