21162 - Throbber animation doesn't stop

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect, P3)

Description

[Scott Furman]

1) Go to http://www.slashdot.org/
2) Right-mouse on "News for Nerds. Stuff that Matters"
3) Select 'view image' from context menu

Result: Image loads, but throbber never shuts off

Comment 1

[Scott Furman]

Think this has something to do with images and Expires: headers

Comment 2

[Scott Furman]

Happens much more frequently on the Mac

Comment 3

[Scott Furman]

I have a fix for this cache/throbber-animation-doesnt-stop bug.  A bonus is that
the fix also seems to result in a MASSIVE performance improvement, at least on
Windows.  I was wondering if sdagley and pavlov could try this patch and tell
me:

  + Does it cure the Mac throbber-not-stopping ?
  + Does it cure the Linux crash ?

For bonus points, I could use a code review and then I'll prod chofmann to let
me check in.

The problem had to do with the way OnDataAvailable notifications were generated
when they originated from the memory cache.  Since the memory cache is not a
source of async events like the file and socket transports, I chose to generate
an initial OnDataAvailable event for the first chunk of data.  When the consumer
read the data from the input stream, I generated the next OnDataAvailable and so
on.  This effectively throttles the event rate and provides a source of "async"
events.

The problem was that the image library would refuse to consume any data because
it already held a requested image in its internal cache.  Since the image lib
never called Read() on the memory cache stream, no OnDataAvailable's were fired
after the first one and no OnStopRequest() was sent, so from the standpoint of
the load group, the request was active indefinitely.

The fix is to trigger each subsequent OnDataAvailable not when the downstream
consumer calls Read on the input stream, but when the prior OnDataAvailable
event is fired.

Comment 4

[Scott Furman]

Attachment: Proposed fix patch

Comment 5

[Scott Furman]

FWIW, this change only affects caching and, at the moment, the cache manager DLL
isn't even loaded unless you flip the browser.cache.enabled pref.

Comment 6

[Steve Dagley]

Works on my Mac build.  Seems to be faster than yesterday's built too but I
don't remember what site were were testing images on to repeat.  That and I'm on
a DSL connection at home rather than the 'big pipe' at the Netscape campus.

Comment 7

[Bill Law]

Don asked me to take a look at this (since I've wrestled with similar throbber
related problems in the past) to make sure it won't mess up anything in the
throbber/status bar front-end code.

I don't have any concerns on that front.  It appears we'll not be seeing any
sequence of notifications that we don't already under different circumstances.

I don't feel qualified to review the code changes, though.

Comment 8

[Steve Dagley]

I don't know if it is cache related but if I go to www.macsurfer.com, click on
one of the links on that page, then hit back to return to www.macsurfer.com
sometimes the page is truncated.  The throbber stops, there's no error and the
scroll bar is proportioned for the truncated page.  Doesn't happen all the time
though.

Comment 9

[Steve Dagley]

There could still be a problem...  I just had a case where going back to a cached

page (www.macsurfer.com) did not kill the throbber or indicate the page was

finsihed loading although it looked complete.

Comment 10

[Scott Furman]

I wasn't able to reproduce this on NT.  sdagley and I tried to make this happen
on Mac again for a few minutes and we weren't able to.

Comment 11

[Scott Furman]

Today's my last day of work.  If I don't get a code review now, the new cache
owner will need to take care of checking this patch in.  I'm in the Mountain
View office if someone wants me to come over and explain the diffs.

Comment 12

[Scott Furman]

Brendan, if you want to review and then check in the patch, that would be cool.
This really should go in for M12.

Comment 13

[Scott Furman]

Brendan, if you want to review and then check in the patch, that would be cool.
This really should go in for M12.

Comment 14

[Brendan Eich [:brendan]]

sdagley kindly checked the patch in.  Now to see what remains (rjc bug on
warren's list?) to get the cache enabled.

/be

Comment 15

[leger]

Bulk move of all Cache (to be deleted component) bugs to new Networking: Cache
component.

Comment 16

[Tom Everingham]

original bug verified working on NT4.0 2000011108 and OS8.6 2000011008

Comment 17

[Loco]

This one is funky-I just came to bugzilla to verify a bug and saw that after I
hit <Commit> the throbber didn't stop throbbing.  This is not happening in
normal pages (i.e. they load and stop throbbing) but it does in Bugzilla when
committing info in a bug.

Comment 18

[Loco]

By the way, it seems to stop throbbing when you click on something (i.e. I
clicked on scrll bar and throbber stopped throbbing).

Comment 19

[Loco]

Could this be at all related to bug # 55975?
http://bugzilla.mozilla.org/show_bug.cgi?id=55975

Comment 20

[John Morrison]

Tsk, tsk, Dan. Morphing a year-old bug. The specific case that you are
referring to is being handled as bug 39310 (although someone hijacked that
bug too, from an image loading example into a bugzilla-loading bug).

-> FIXED

Comment 21

[John Morrison]

Ding!