401423 - progress meter on status bar doesn't appear for loads targetted to new tabs

Status: RESOLVED WORKSFORME

(Firefox :: Tabbed Browser, defect)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Summary: progress meter doesn't appear for loads targetted to new tabs

This is broken in both branch and trunk builds.

Build ID: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102704 Minefield/3.0a9pre

Summary: 

URL: data:text/html,<a href="http://mozilla.org" target="_blank">mozilla</a>

Steps to Reproduce:

1. Load the URL (data:text/html,<a href="http://mozilla.org" target="_blank">mozilla</a>)
2. Click on mozilla

Expected Results:

Browser shows progress-status in its progress meter

Actual Results:

We don't get any progress/loading status

Comment 1

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

Not a blocker, but we'd take a fix. Might re-evaluate if we end up making the progress indicator more prominent.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Is this a regression?

Comment 3

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #2)
> Is this a regression?

Amazingly, no; this has been broken forever, where forever=as far back as the 1.8 branch: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11

Works in at least the latest Opera and Safari versions.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Was it broken in 1.8.0?  What about 1.8.1.0?

Does it depend on whether new tabs show in the background or not?

Comment 5

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

From an old IRC log from around when this was filed:
<gavin_> hmm, seems like a valid bug
<stephend> it's weird, yeah
<gavin_> we unhide the progressmeter when we get a STATE_START event from the progresslistener
<stephend> btw, this has been around for ever
<gavin_> looks like it's not getting it for new-tab loads
<gavin_> triggered by target="_blank" links

I think this might be a tabbrowser progresslistener bug.

Comment 6

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

Specifically, I think the tabbrowser's progress listener gets STATE_START before the new tab is selected, so the currentTab == mTab check at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/tabbrowser.xml&rev=1.260#395 is false and we don't end up calling the browser's onStateChange, which is what unhides the progress bar (http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.957&mark=3475#3449).

Comment 7

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

I was wrong, this doesn't look related to tabbrowser. The code at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.957&mark=3475#3449 doesn't run because gProgressCollapseTimer is non-null, because we got a STATE_STOP from the tab's about:blank load.

Comment 8

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

(In reply to comment #7)
> The code at
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.957&mark=3475#3449
> doesn't run [...] because we got a STATE_STOP from the tab's about:blank load.

Which would be fine, if we received a STATE_START for that same about:blank load that would show the progress meter. Not sure why we're not getting it.

Comment 10

[Steve England [:stevee]]

Works: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060603
Minefield/3.0a1 ID:2006060305

Broken: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1)
Gecko/20060604 Minefield/3.0a1 ID:2006060404

Checkins to module PhoenixTinderbox between 2006-06-03 04:00 and 2006-06-04
05:00 :
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-06-03+04&maxdate=2006-06-04+05&cvsroot=%2Fcvsroot

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Looks like that the patch on bug 331938 has caused this issue?

Comment 12

[Simon Bünzli]

Attachment: use gProgressMeterPanel.collapsed instead of gProgressCollapseTimer

I see two alternatives for how to fix this issue. This one removes the slightly fragile connection between gProgressCollapseTimer and gProgressMeterPanel.collapsed and uses the latter to determine whether the progress bar should be shown...

Comment 13

[Simon Bünzli]

Attachment: don't set a timer when the progress bar is already hidden

... while this patch prevents the timer from being set when it's not needed at all.

Comment 14

[Simon Bünzli]

The third alternative would be teaching tabbrowser.xml to not dispatch multiple STATE_STOP changes in a row (which it might do since bug 331938 and which causes gProgressCollapseTimer being set when it's not needed at all).

Trying to enforce such a contract seems however to be even more fragile than what we already do (and would keep doing with the second patch). The core issue is BTW that we use a single progress listener for whichever tab is currently active (all others are "muted") which necessarily causes indeterminate behavior when switching tabs...

Comment 15

[Simon Bünzli]

Comment on attachment 308451 [details] [diff] [review]
use gProgressMeterPanel.collapsed instead of gProgressCollapseTimer

Would probably be even cheaper to just always uncollapse it.

Comment 16

[Simon Bünzli]

Comment on attachment 308452 [details] [diff] [review]
don't set a timer when the progress bar is already hidden

And here we should probably also make sure that a gProgressCollapseTimer hasn't been set already.

Comment 17

[Simon Bünzli]

Would have to go through the possible code path alternatives once again. Not this month, though.

Comment 18

[Wayne Mery (:wsmwk)]

can you still reproduce?

WFM using current nightly.

Comment 19

[Phoenix]

Resolved per whiteboard and Comment 18