Closed Bug 297155 Opened 15 years ago Closed 12 years ago

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

Categories

(SeaMonkey :: Tabbed Browser, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ajschult784, Assigned: neil)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
forgot to mention that this also affects a firefox build
Flags: blocking1.8b3?
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.
Component: History: Session → Tabbed Browser
Keywords: regression
OS: Linux → All
QA Contact: history.session → tabbed-browser
I see this in Firefox 1.0.4, so this isn't a regression from bug 292971...
No longer depends on: 292971
ya, this is a really old bug
Flags: blocking1.8b3?
Attachment #186479 - Flags: superreview?(jag)
Attachment #186479 - Flags: review?(jag)
Comment on attachment 186479 [details] [diff] [review]
Does forcing tabbed mode hurt perf?

r+sr=jag
Attachment #186479 - Flags: superreview?(jag)
Attachment #186479 - Flags: superreview+
Attachment #186479 - Flags: review?(jag)
Attachment #186479 - Flags: review+
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.
Attachment #186479 - Flags: approval1.8b3?
Attachment #186479 - Flags: approval1.8b3? → approval1.8b3+
Looks like this hurt Tp a bit.
By around 6 ms on btek. Acceptable?
Assignee: nobody → neil.parkwaycc.co.uk
Attachment #186479 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #186923 - Flags: superreview?(jag)
Attachment #186923 - Flags: review?(jag)
We can't fix the bug another way that doesn't hurt Tp?
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.
(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 ;-)
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.
> 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 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 :-)
Attachment #186923 - Flags: superreview?(jag)
Attachment #186923 - Flags: superreview+
Attachment #186923 - Flags: review?(jag)
Attachment #186923 - Flags: review+
Comment on attachment 186923 [details] [diff] [review]
Tabbed mode with cleanup

We think this version won't hurt Tp so much.
Attachment #186923 - Flags: approval1.8b3?
Attachment #186923 - Flags: approval1.8b3? → approval1.8b3+
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
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.
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...)?
> 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.
(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?
Marking FIXED per Neil on IRC
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Product: Core → SeaMonkey
Duplicate of this bug: 129282
You need to log in before you can comment on or make changes to this bug.