297155 - opening a new tab while loading first page in new window breaks throbber / status bar

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, defect)

Description

[Andrew Schultz]

If I open a new tab (^t) while loading a page in a new window, the throbber
spins and the status bar says "waiting for hostname..." forever and the
progressbar gets stuck in its current position.  This only happens if the pref
is set to hide the tab bar when only one tab is open and if a second tab has
never been opened in the current window.

Steps to reproduce:
1. open a new window
2. if the home page loads to quickly, shift-reload, or open a slower page.
3. hit ctrl-t while the page is loading

even with the new (blank) tab selected the throbber spins.

This regressed between a suite build from 2005052705 (1.8b2) and 2005060102. 
bug 292971 looks like the most likely candidate in that window.

Comment 1

[Andrew Schultz]

forgot to mention that this also affects a firefox build

Comment 2

[[e-mail]]

Are you sure it's a regression? By my reading of the code, it's a bug that has
existed since tabbrowser was created; what happens is that before the tabbed
browser enters tabbed mode it doesn't track the progress of its first tab, which
means that it doesn't show the loading arrows on the original tab either.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I see this in Firefox 1.0.4, so this isn't a regression from bug 292971...

Comment 4

[Andrew Schultz]

ya, this is a really old bug

Comment 5

[[e-mail]]

Attachment: Does forcing tabbed mode hurt perf?

Comment 6

[jag (Peter Annema)]

Comment on attachment 186479 [details] [diff] [review]
Does forcing tabbed mode hurt perf?

r+sr=jag

Comment 7

[[e-mail]]

Comment on attachment 186479 [details] [diff] [review]
Does forcing tabbed mode hurt perf?

Seeking approval for this as a perf test only - if we go with tabbed mode by
default we'll remove all the non-tabbed code.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks like this hurt Tp a bit.

Comment 9

[jag (Peter Annema)]

By around 6 ms on btek. Acceptable?

Comment 10

[[e-mail]]

Attachment: Tabbed mode with cleanup

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We can't fix the bug another way that doesn't hurt Tp?

Comment 12

[jag (Peter Annema)]

We could fix just the throbber, and maybe the status bar, but there are other
tab specific things that we're currently not keeping track of in non-tabbed mode
that we'd have to start keeping track of. Simply doing that will increase Tp a bit.

Comment 13

[[e-mail]]

(In reply to comment #11)
>We can't fix the bug another way that doesn't hurt Tp?
The test is simplistic and actually hurts Tp more than it should...
of course the proof of attachment 186923 [details] [diff] [review] is in the eating ;-)

Comment 14

[jag (Peter Annema)]

Besides, Tp is only affected for those who never use tabs and have chosen the
tab bar to never be shown.

If you open tabs or if you always show the tab bar then in your day to day
browsing you already get this small Tp hit. In fact, with this patch people who
start with the tab bar visible will get a slightly better Txul and Ts because
we're avoiding hooking things up only to tear 'em down so we can hook them up
"tabbed mode" style.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Besides, Tp is only affected for those who never use tabs and have chosen the
> tab bar to never be shown.

That sounds reasonable, but that's not the default, right? So why is btek affected?

Comment 16

[jag (Peter Annema)]

Comment on attachment 186923 [details] [diff] [review]
Tabbed mode with cleanup

>Index: tabbrowser.xml
>===================================================================

>@@ -713,38 +710,26 @@
>               return;
>             }
> 
>             var i;
> 
>             // Security says okay, now ask content policy
>+            for (i = 0; i < this.browsers.length; i++) {
>+              if (this.browsers[i].contentDocument == event.originalTarget.ownerDocument) {
>+                if (contentPolicy.shouldLoad(nsIContentPolicy.TYPE_IMAGE,
>+                                             uri, origURI, event.target,
>+                                             safeGetProperty(event.target, "type"),
>+                                             null) != nsIContentPolicy.ACCEPT)
>+                  return;
> 
>+                this.mTabListeners[i].setIcon(href);
>+                if (this.browsers[i] == this.mCurrentBrowser) {
>+                  for (i = 0; i < this.mProgressListeners.length; i++) {
>+                    var p = this.mProgressListeners[i];
>+                    if (p && 'onLinkIconAvailable' in p)
>+                      p.onLinkIconAvailable(href);
>+                  }
>                 }
>+                break;
>               }
>             }
>           ]]>

I foresee subtle bugs in tabbrowser's future :-)

Comment 17

[[e-mail]]

Comment on attachment 186923 [details] [diff] [review]
Tabbed mode with cleanup

We think this version won't hurt Tp so much.

Comment 18

[Robert Kaiser]

BTW, just as a note, data from brad tinderbox (from the time when the first
patch was in) shows that untabbed mode seems to be leaking as hell, while the
switch to tabbed mode decreased leaks quite a lot:
untabbed mode (before checkin / after backout) - RLk:743KB, Lk:4.25MB
tabbed mode (when patch was in) - RLk:228B, Lk:294KB

Comment 19

[jag (Peter Annema)]

Interesting find on the leaks. With this second checkin, RLk:743KB, Lk:4.25MB ->
RLk:228B, Lk:294KB. That's quite nice!

Somehow this second checkin managed to bring btek Tp up 10 ms instead of the 6
ms from last time. Then again, we went down 10 ms when Neil backed out the
simple patch. I wonder what got checked in that's causing this change to have a
larger impact... Or perhaps the graph is lying to us about there being two
distinct jumps on the 20th?

I wonder how much of that 10 ms then is us freeing memory 'coz we stopped
leaking something big.

Comment 20

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050913 Firefox/1.4
ID:2005091305

I don't think this is quite fixed yet - when browser.tabs.autoHide is set to
true and a browser window is opened, the first tab is still broken.

Steps to Reproduce:

1. Set browser.tabs.autoHide to true.
2. Open a new window.
3. Start a URL load in the current, hidden tab.
4. While this is loading, hit Ctrl+T.

Actual results: The throbber on top of the favicon is not spinning and the tab
title is "(Untitled") instead of "Loading..."

Is this sort of thing affected by profile condition (i.e. extensions, dirty
RDFs, etc...)?

Comment 21

[Andrew Schultz]

> I don't think this is quite fixed yet (in Firefox)

That seems reasonable considering the patch was only for SeaMonkey.  File a new
bug in Firefox:Tabbed Browser (if one doesn't already exist in that component).
 Firefox devs might just port the patch from here or might want to do it
differently.

Comment 22

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #21)
>
> That seems reasonable considering the patch was only for SeaMonkey.  File a new
> bug in Firefox:Tabbed Browser (if one doesn't already exist in that component).
> Firefox devs might just port the patch from here or might want to do it
> differently.

*egg on face*

Bug 292865 is filed against Firefox:Tabbed Browser - in fact you yourself
pointed out that this bug was filed against Core:Tabbed Browser.

If the Seamonkey patch were ported to toolkit and presented for r+sr and
approval1.8b5?, would it be taken? Or is it too late for that sort of change?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Marking FIXED per Neil on IRC