Last Comment Bug 487638 - status bar blames wrong resource when downloading slow responding resource
: status bar blames wrong resource when downloading slow responding resource
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla1.9.2a1
Assigned To: Christian :Biesinger (don't email me, ping me on IRC)
:
Mentors:
http://analytics.nfshost.com/case3.html
: 487984 (view as bug list)
Depends on:
Blocks: 241257
  Show dependency treegraph
 
Reported: 2009-04-09 09:37 PDT by bnkuhn
Modified: 2009-07-28 10:52 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (11.47 KB, patch)
2009-07-14 23:36 PDT, Christian :Biesinger (don't email me, ping me on IRC)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Review
patch v2 (11.50 KB, patch)
2009-07-28 03:58 PDT, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details | Diff | Review
patch v2, merged to trunk (11.48 KB, patch)
2009-07-28 05:19 PDT, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details | Diff | Review
additional patch to nsBrowserStatusFilter (689 bytes, patch)
2009-07-28 05:21 PDT, Christian :Biesinger (don't email me, ping me on IRC)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Review
complete patch (12.17 KB, patch)
2009-07-28 07:41 PDT, Christian :Biesinger (don't email me, ping me on IRC)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Review
fix memory leaks (2.09 KB, patch)
2009-07-28 10:39 PDT, Christian :Biesinger (don't email me, ping me on IRC)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Review

Description bnkuhn 2009-04-09 09:37:38 PDT
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.
Comment 1 Tyler Downer [:Tyler] 2009-04-10 08:30:20 PDT
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).
Comment 2 bnkuhn 2009-04-10 10:43:14 PDT
I have now repeated this on Mac, XP and Linux with trunk/safe mode/new profile.
Comment 3 bmccann 2009-04-14 10:12:02 PDT
*** Bug 487984 has been marked as a duplicate of this bug. ***
Comment 4 bmccann 2009-04-14 10:14:52 PDT
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
Comment 5 bnkuhn 2009-06-16 13:13:23 PDT
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.
Comment 6 Christian :Biesinger (don't email me, ping me on IRC) 2009-06-16 13:27:22 PDT
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.
Comment 7 bnkuhn 2009-06-16 13:36:03 PDT
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 ;)
Comment 8 Christian :Biesinger (don't email me, ping me on IRC) 2009-07-14 23:36:48 PDT
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.
Comment 9 Boris Zbarsky [:bz] 2009-07-24 22:31:08 PDT
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).
Comment 10 Christian :Biesinger (don't email me, ping me on IRC) 2009-07-28 03:53:04 PDT
(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.
Comment 11 Christian :Biesinger (don't email me, ping me on IRC) 2009-07-28 03:58:20 PDT
Created attachment 391051 [details] [diff] [review]
patch v2
Comment 12 Christian :Biesinger (don't email me, ping me on IRC) 2009-07-28 05:19:31 PDT
Created attachment 391060 [details] [diff] [review]
patch v2, merged to trunk
Comment 13 Christian :Biesinger (don't email me, ping me on IRC) 2009-07-28 05:21:46 PDT
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.
Comment 14 Boris Zbarsky [:bz] 2009-07-28 06:01:03 PDT
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 15 Boris Zbarsky [:bz] 2009-07-28 06:03:11 PDT
Comment on attachment 391062 [details] [diff] [review]
additional patch to nsBrowserStatusFilter

mCurrentStatusMsg is write-only after this, no?  Please remove it?
Comment 16 Christian :Biesinger (don't email me, ping me on IRC) 2009-07-28 07:41:12 PDT
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.
Comment 17 Christian :Biesinger (don't email me, ping me on IRC) 2009-07-28 09:19:10 PDT
Fixed for 1.9.2a1!

http://hg.mozilla.org/mozilla-central/rev/dcda49ff1a26
Comment 18 Christian :Biesinger (don't email me, ping me on IRC) 2009-07-28 10:39:14 PDT
Created attachment 391120 [details] [diff] [review]
fix memory leaks

need to call the destructor of nsRequestInfo
Comment 19 Boris Zbarsky [:bz] 2009-07-28 10:44:06 PDT
Comment on attachment 391120 [details] [diff] [review]
fix memory leaks

Put the opening curly on its own line, and looks good.
Comment 20 Christian :Biesinger (don't email me, ping me on IRC) 2009-07-28 10:52:36 PDT
http://hg.mozilla.org/mozilla-central/rev/7149d895e5b3

Note You need to log in before you can comment on or make changes to this bug.