268985 - Pressing stop does not always stop image download

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Sometimes after pressing the stop button, images continue to download, even
though the throbber stops.  Clicking the "go offline" button does stop the download.

Comment 1

[Darin Fisher]

sounds more like an imagelib bug to me.  probably some loads are not being added
to a loadgroup or something.

-> imagelib

Comment 2

[Boris Zbarsky [:bzbarsky]]

Ctho, what are the steps to reproduce?  "It happens sometimes" with no
indication of when is not so helpful....

Comment 3

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #2)
> Ctho, what are the steps to reproduce?  "It happens sometimes" with no
> indication of when is not so helpful....

Visit URL, wait for a few images (but not all) to load, hit stop.  The throbber
stops, the images keep loading.  Note that the "thumbnails" at the top are the
same images as the large versions, with 'width=80'.

The thumbnails may stop painting until you scroll (this behavior doesn't seem
consistent), but if you scroll down (or look at the scrollbar widget) you'll
note that images continue to load.  The thumbnails for the images that are
downloaded after you hit stop get replaced with the upper left corner of the
image, as each one downloads (this is strange behavior - I would expect nothing,
or the whole image)

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: testcase

Comment 5

[OstGote!]

Yeah I see continues loading and replacing of the images after pressing stop.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So the basic problem is the code at
http://lxr.mozilla.org/seamonkey/source/modules/libpr0n/src/imgLoader.cpp#528

This doesn't add the imgRequestProxy to the loadgroup if we already have a
cached imgRequest for it.  The problem is that the cached imgRequest may not be
done loading yet.  We should only skip adding to the loadgroup when
OnStopRequest has already fired on that request.

Note that fixing this part will fix the visual manifestation of the bug.  It may
not fix the underlying load continuing (though if the loadgroup cancels with
NS_BINDING_ABORTED or some other error code, it should).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 165650 [details] [diff] [review]
Proposed patch

The comment explains why this change is the right thing...

Comment 9

[Darin Fisher]

Comment on attachment 165650 [details] [diff] [review]
Proposed patch

sr=darin

i'm a little concerned about the additional webprogresslistener traffic, but
that should hopefully not be too much of a problem.  we could set the
LOAD_BACKGROUND load flag on the subsequent proxy requests if it is.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Stuart says this will rebreak the bug that this code was checked in for, but I
don't see how, and the code as written is clearly wrong... jst, you've poked at
this stuff too.  Do you have any idea what's going on here?

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 271987 has been marked as a duplicate of this bug. ***

Comment 12

[Torsten Bondieck]

(In reply to comment #11)
> *** Bug 271987 has been marked as a duplicate of this bug. ***

a extra testcase ;-)

Comment 13

[Bill McGonigle (not currently reading bugmail; please contact directly)]

*** Bug 245210 has been marked as a duplicate of this bug. ***

Comment 14

[Boris Zbarsky [:bzbarsky]]

Fixed

Comment 15

[Bernd]

I believe this bug caused bug 281892. At least backing this patch out fixes the
issue there.