status bar blames wrong resource when downloading slow responding resource

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Networking
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: bnkuhn, Assigned: Biesinger)

Tracking

Trunk
mozilla1.9.2a1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090409 Minefield/3.6a1pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090409 Minefield/3.6a1pre

If an image or script is requested from a server that is slow to respond, the status bar in Firefox says that it is still downloading the previous resource, which has actually finished downloading.  This leads to users blaming the previous resource for slow loading pages.

I believe this happens because Firefox doesn't update the status bar until it starts receiving data from the slow server.  It should however update the status bar as soon at it makes the request for a new resource.

I've created example pages here:
http://analytics.nfshost.com/case3.html  (slow image)
http://analytics.nfshost.com/case3b.html (slow script)

Reproducible: Always

Steps to Reproduce:
1. Find URLs for two scripts; one that loads quickly and one that takes a long time for the server to start sending back data.
2. Insert both scripts into a page (the quick one right before the slow one).
3. Open the page in Firefox and notice that the status bar says it's waiting on the first script for a long time; it will only start blaming the second script once its server starts sending back data.
Actual Results:  
Status bar blames the previous resource until the current resource's server starts sending back data.

Expected Results:  
Status bar should blame the current resources as soon as it makes the request for it.

I'm able to reproduce this with Firefox 3 on Windows, Linux and Mac every time.  I've only tested the nightly build on Mac.

By "blame", I mean the status bar says something like, "waiting on www.google-analytics.com..." when it's actually waiting on another resource.

I'm a member of the Google Analytics team, and this is a major issue for us.  We often get blamed for slowing down pages (in Firefox) that are actually being slowed down by other resources.
Component: General → Networking
Product: Firefox → Core
QA Contact: general → networking
Version: unspecified → Trunk
You have only tried trunk on Mac? Does Safe Mode or a new profile make a difference? (Probably won't, but good to make sure).
(Reporter)

Comment 2

9 years ago
I have now repeated this on Mac, XP and Linux with trunk/safe mode/new profile.

Updated

9 years ago
Duplicate of this bug: 487984

Comment 4

9 years ago
I had opened a bug for this as well, which I've now closed as being a duplicate of this bug.  Here's another example page: http://www.benmccann.com/test/firefox-status-bar-lies.html

Updated

9 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 5

8 years ago
I'm going to poke around the source a bit to see how difficult of a fix this might be.  If anyone has any advice on where to find the pertinent code, please let me know.
Ah right. I was going to look at it too, but if you want to, go ahead.

The code flow is basically this:
nsHttpChannel::OnTransportStatus calls mProgressSink->OnStatus
that's:
nsDocLoader::OnStatus - this function creates the "Transferring data from ..." string
it calls:
nsBrowserStatusFilter::OnStatusChange (this class is a mess, btw)
which calls, sooner or later,
XULBrowserWindow.onStatusChange in browser.js

The problem is, I guess, that after the google-analytics file finishes loading, there are no new status events, so the status bar doesn't change.
(Reporter)

Comment 7

8 years ago
Thanks for the info.  I'm only looking into it because I couldn't find someone to work on that was already experienced hacking Firefox.  I have zero experience with the code base, so this should be interesting ;)
Assignee: nobody → cbiesinger
Blocks: 241257
Created attachment 388650 [details] [diff] [review]
Patch

This patch does the following:
- Makes nsDocLoader store the last-sent status message for each request in the corresponding nsRequestInfo, and keeps a flag to indicate whether the request is done
- When a request finishes, it looks for a random request that is not done and has a non-empty status, and if it finds one, sends that status message to the listeners
- To make this all work, the patch also changes imagelib so that status and state messages sent for images are sent with the imgIRequest as the request and not the channel. This is necessary because the channel is not part of the loadgroup for images, only the imgIRequest is.
Attachment #388650 - Flags: superreview?(bzbarsky)
Attachment #388650 - Flags: review?(bzbarsky)
Status: NEW → ASSIGNED
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
Comment on attachment 388650 [details] [diff] [review]
Patch

How often do the notifications come in?  Would it make sense to cache the progress event sink for the channel?  Or do we never know when it might change?

Returning PR_FALSE in imgLoader::LoadImage is certainly wrong.  I suggest NS_ERROR_OUT_OF_MEMORY instead.

I'd prefer a method on nsRequestInfo that returns the nsIRequest*.

I assume when all requests finish we'll change the status from somewhere else, so don't have to worry about those cases?

r+sr=bzbarsky, with the nits picked (esp. the incorrect return value).
Attachment #388650 - Flags: superreview?(bzbarsky)
Attachment #388650 - Flags: superreview+
Attachment #388650 - Flags: review?(bzbarsky)
Attachment #388650 - Flags: review+
(In reply to comment #9)
> (From update of attachment 388650 [details] [diff] [review])
> How often do the notifications come in?  Would it make sense to cache the
> progress event sink for the channel?  Or do we never know when it might change?

Yeah, it can change at any time, especially when extensions are involved...

> Returning PR_FALSE in imgLoader::LoadImage is certainly wrong.  I suggest
> NS_ERROR_OUT_OF_MEMORY instead.

Oops. Thanks.

> I'd prefer a method on nsRequestInfo that returns the nsIRequest*.

Done.

> I assume when all requests finish we'll change the status from somewhere else,
> so don't have to worry about those cases?

That's the responsibility of the callee, in case of Firefox browser.js, which displays the Done message. It gets an onStateChange message for that purpose.
Created attachment 391051 [details] [diff] [review]
patch v2
Attachment #388650 - Attachment is obsolete: true
Created attachment 391060 [details] [diff] [review]
patch v2, merged to trunk
Attachment #391051 - Attachment is obsolete: true
Created attachment 391062 [details] [diff] [review]
additional patch to nsBrowserStatusFilter

This patch is needed for an edge case... (I hit that sometimes when clicking reload on the testcase here)

Basically, when nsBrowserStatusFilter has a pending status change, it needs to compare the new status to the pending status instead of the current status, because otherwise the (now incorrect) pending status will be sent to the UI.
Attachment #391062 - Flags: superreview?(bzbarsky)
Attachment #391062 - Flags: review?(bzbarsky)
One other note.  The imagelib behavior will still be screwy if the imgRequestProxy is canceled before the underlying channel is done loading but while it has other proxies.  We'll keep delivering the channel's status notifications with that canceled proxy as the request.  Is that ok?
Comment on attachment 391062 [details] [diff] [review]
additional patch to nsBrowserStatusFilter

mCurrentStatusMsg is write-only after this, no?  Please remove it?
Attachment #391062 - Flags: superreview?(bzbarsky)
Attachment #391062 - Flags: superreview+
Attachment #391062 - Flags: review?(bzbarsky)
Attachment #391062 - Flags: review+
Created attachment 391078 [details] [diff] [review]
complete patch

This is the two previous patches as a single file. For nsBrowserStatusFilter, I did it this way instead which seems better (avoids calls to onStatusChange when not needed):
-    if (!mCurrentStatusMsg.Equals(aMessage)) {
+    if (mStatusIsDirty || !mCurrentStatusMsg.Equals(aMessage)) {

I think the image behaviour isn't terrible even when the imgIRequest gets cancelled... I think it's good to still have the status bar updates when the image is still loading elsewhere on that page. But I could also change the code to check the request's status before forwarding onStatus/onProgress, if you want me to.
Attachment #391060 - Attachment is obsolete: true
Attachment #391062 - Attachment is obsolete: true
Attachment #391078 - Flags: superreview?(bzbarsky)
Attachment #391078 - Flags: review?(bzbarsky)
Attachment #391078 - Flags: superreview?(bzbarsky)
Attachment #391078 - Flags: superreview+
Attachment #391078 - Flags: review?(bzbarsky)
Attachment #391078 - Flags: review+
Fixed for 1.9.2a1!

http://hg.mozilla.org/mozilla-central/rev/dcda49ff1a26
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Created attachment 391120 [details] [diff] [review]
fix memory leaks

need to call the destructor of nsRequestInfo
Attachment #391120 - Flags: superreview?(bzbarsky)
Attachment #391120 - Flags: review?(bzbarsky)
Attachment #391120 - Flags: superreview?(bzbarsky)
Attachment #391120 - Flags: superreview+
Attachment #391120 - Flags: review?(bzbarsky)
Attachment #391120 - Flags: review+
Comment on attachment 391120 [details] [diff] [review]
fix memory leaks

Put the opening curly on its own line, and looks good.
http://hg.mozilla.org/mozilla-central/rev/7149d895e5b3
You need to log in before you can comment on or make changes to this bug.