481560 - tab closes and location bar clears before the window closes

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Robert Sayre]

str: 

1.) visit an SSL site
2.) close window with keyboard command (cmd-w, etc)

Comment 1

[Mike Connor [:mconnor]]

Strange.  We also unload the document and clear the location bar here.  This might be marginally more visible with the SSL UI, but isn't a regression from bug 480357.  Change the pref back to 0 and you'll still see the unload/clear behaviour before window close, so the SSL indicator is just part of a larger bit of weirdness.  We definitely close a window much slower now, in 3.0 this is basically instantaneous.  Also happens on non-SSL sites.

cc-ing dao, since he's done a bunch of tab stuff, wondering if there's a change that's responsible for this alteration of behaviour.  Suspect we're doing something like clearing the existing tab before window.close() now...

Comment 2

[Dão Gottwald [:dao]]

(In reply to comment #1)
> Suspect we're doing
> something like clearing the existing tab before window.close() now...

That's right, it was needed for cases such as dragging the last tab to a different window.

Comment 3

[Dão Gottwald [:dao]]

... but I think that can be special-cased. I'll look into that.

Comment 4

[Dão Gottwald [:dao]]

Attachment: patch

Comment 5

[Robert Sayre]

should have a perf test for this

Comment 6

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

Don't think it blocks release, but definitely wanted. Thanks for putting together a patch, Dao.

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch

test added

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Why does this behavior need to be optional (i.e. why do you need aCloseWindowFastpath)? I don't see offhand why other _beginRemoveTab callers wouldn't want this behavior.

Comment 9

[Dão Gottwald [:dao]]

It would regress bug 456002 otherwise.

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch

updated to latest trunk

Comment 11

[Dão Gottwald [:dao]]

Attachment: patch

updated to latest trunk

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 370281 [details] [diff] [review]
patch

Ah, right. Seems like we should still be able to skip some of the work in endRemoveTab in the window-closing cases, but no need to fix that here.

Comment 13

[Dão Gottwald [:dao]]

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

Comment 14

[Robert Sayre]

It looks like this patch is causing or exacerbating intermittent Linux test failures in test_Browser.js.

TEST-PASS | chrome://mochikit/content/browser/browser/fuel/test/browser_Browser.js | Checking index after moving tab
TEST-PASS | chrome://mochikit/content/browser/browser/fuel/test/browser_Browser.js | Checking event handler for tab move
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/fuel/test/browser_Browser.js | Timed out

Comment 15

[Dão Gottwald [:dao]]

That test was broken before and should have been disabled a while ago -- see bug 458688. I did that now.

Comment 16

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

Comment on attachment 370281 [details] [diff] [review]
patch

a191=beltzner

Comment 17

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fbb1c2888024

Comment 18

[Aakash Desai [:aakashd]]

verified FIXED when at https://bugzilla.mozilla.org on builds:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090604 Shiretoko/3.5pre (.NET CLR 3.5.30729) ID:20090604045218

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090604 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090604042555