63750 - network reloading of 1x1 image (depends on libpron landing)

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[pnunn]

This was reported in another bug, #58535.

I am opening this bug to keep bug#58535 from
mutating. 

See comments added by Tomi & Frank.
-pn

Comment 1

[pnunn]

Attachment: simplest test, given by Tomi in bug#58535

Comment 2

[pnunn]

In fact, the simplest test reduces to:
<IMG src="http://images.slashdot.org/greendot.gif" width="80%" height=1>

The critical part is the specifing one dimen by percentage and the other
by an absolute value.
-p

Comment 3

[pnunn]

In the debugger I see a loop of loading the
image and then immediately unloading the image.
I'm guessing the cleanup for the FrameImageLoader
in triggering the IL_DestroyImage removes the needed
image from the image cache, just as its needed for
layout reflow or drawing. 

The fact that the dimensions of the image are dependent
on the window (because it is a percentage) seems to be 
a factor in creating this loop.
-------------------------------------------------
IL_DestroyImage(_IL_ImageReq * 0x05659050) line 1119
ImageRequestImpl::~ImageRequestImpl() line 190 + 12 bytes
ImageRequestImpl::`scalar deleting destructor'(unsigned int 0x00000001) + 15 
bytes
ImageRequestImpl::Release(ImageRequestImpl * const 0x0565b4f0) line 271 + 154 
bytes
nsFrameImageLoader::StopImageLoad(nsFrameImageLoader * const 0x0565f6c0, int 
0x00000001) line 273 + 18 bytes
nsPresContext::StopLoadImage(nsPresContext * const 0x05627b90, void * 
0x013942ec, nsIFrameImageLoader * 0x0565f6c0) line 1157
nsHTMLImageLoader::StartLoadImage(nsIPresContext * 0x05627b90) line 249
nsHTMLImageLoader::GetDesiredSize(nsIPresContext * 0x05627b90, const 
nsHTMLReflowState * 0x0012d898, nsHTMLReflowMetrics & {...}) line 479
nsImageFrame::GetDesiredSize(nsIPresContext * 0x05627b90, const 
nsHTMLReflowState & {...}, nsHTMLReflowMetrics & {...}) line 322 + 23 bytes
nsImageFrame::Reflow(nsImageFrame * const 0x013942ec, nsIPresContext * 
0x05627b90, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0x00000000) line 362
nsLineLayout::ReflowFrame(nsIFrame * 0x013942ec, nsIFrame * * 0x0012e444, 
unsigned int & 0x00000000, nsHTMLReflowMetrics * 0x00000000, int & 0x00000000) 
line 921
nsBlockFrame::ReflowInlineFrame(nsBlockReflowState & {...}, nsLineLayout & 
{...}, nsLineBox * 0x013943d0, nsIFrame * 0x013942ec, unsigned char * 
0x0012d9bc) line 4363 + 29 bytes
nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState & {...}, nsLineLayout & 
{...}, nsLineBox * 0x013943d0, int * 0x0012e038, unsigned char * 0x0012de80, int 
0x00000000, int 0x00000001) line 4247 + 28 bytes
nsBlockFrame::DoReflowInlineFramesAuto(nsBlockReflowState & {...}, nsLineBox * 
0x013943d0, int * 0x0012e038, unsigned char * 0x0012de80, int 0x00000000, int 
0x00000001) line 4181 + 42 bytes
nsBlockFrame::ReflowInlineFrames(nsBlockReflowState & {...}, nsLineBox * 
0x013943d0, int * 0x0012e038, int 0x00000001, int 0x00000000) line 4126 + 32 
bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineBox * 0x013943d0, int 
* 0x0012e038, int 0x00000001) line 3260 + 29 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}) line 2949 + 27 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x01394214, nsIPresContext * 
0x05627b90, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0x00000000) line 1740 + 15 bytes
nsBlockReflowContext::DoReflowBlock(nsHTMLReflowState & {...}, nsReflowReason 
eReflowReason_Incremental, nsIFrame * 0x01394214, const nsRect & {...}, int 
0x00000001, int 0x00000000, int 0x00000001, nsMargin & {...}, unsigned int & 
0x00000000) line 562 + 36 bytes
nsBlockReflowContext::ReflowBlock(nsIFrame * 0x01394214, const nsRect & {...}, 
int 0x00000001, int 0x00000000, int 0x00000001, nsMargin & {...}, unsigned int & 
0x00000000) line 332 + 50 bytes
nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & {...}, nsLineBox * 
0x01394288, int * 0x0012eb70) line 3879 + 56 bytes
nsBlockFrame::ReflowLine(nsBlockReflowState & {...}, nsLineBox * 0x01394288, int 
* 0x0012eb70, int 0x00000001) line 3142 + 23 bytes
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & {...}) line 2949 + 27 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x0139418c, nsIPresContext * 
0x05627b90, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0x00000000) line 1740 + 15 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x0139418c, nsIPresContext * 
0x05627b90, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 
0x00000000, int 0x00000000, unsigned int 0x00000000, unsigned int & 0x00000000) 
line 693 + 31 bytes
CanvasFrame::Reflow(CanvasFrame * const 0x013d5af4, nsIPresContext * 0x05627b90, 
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 
0x00000000) line 306
nsBoxToBlockAdaptor::Reflow(nsBoxLayoutState & {...}, nsIPresContext * 
0x05627b90, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0x00000000, int 0x00000000, int 0x00000000, int 0x00001fd1, int 
0x0000120c, int 0x00000001) line 868
nsBoxToBlockAdaptor::DoLayout(nsBoxToBlockAdaptor * const 0x01394120, 
nsBoxLayoutState & {...}) line 525 + 52 bytes
nsBox::Layout(nsBox * const 0x01394120, nsBoxLayoutState & {...}) line 1002
nsScrollBoxFrame::DoLayout(nsScrollBoxFrame * const 0x013d5c0c, nsBoxLayoutState 
& {...}) line 379
nsBox::Layout(nsBox * const 0x013d5c0c, nsBoxLayoutState & {...}) line 1002
nsContainerBox::LayoutChildAt(nsBoxLayoutState & {...}, nsIBox * 0x013d5c0c, 
const nsRect & {...}) line 593 + 16 bytes
nsGfxScrollFrameInner::LayoutBox(nsBoxLayoutState & {...}, nsIBox * 0x013d5c0c, 
const nsRect & {...}) line 1029 + 17 bytes
nsGfxScrollFrameInner::Layout(nsBoxLayoutState & {...}) line 1112
nsGfxScrollFrame::DoLayout(nsGfxScrollFrame * const 0x013d5b64, nsBoxLayoutState 
& {...}) line 1037 + 15 bytes
nsBox::Layout(nsBox * const 0x013d5b64, nsBoxLayoutState & {...}) line 1002
nsBoxFrame::Reflow(nsBoxFrame * const 0x013d5b2c, nsIPresContext * 0x05627b90, 
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 
0x00000000) line 789
nsGfxScrollFrame::Reflow(nsGfxScrollFrame * const 0x013d5b2c, nsIPresContext * 
0x05627b90, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0x00000000) line 741 + 25 bytes
nsContainerFrame::ReflowChild(nsIFrame * 0x013d5b2c, nsIPresContext * 
0x05627b90, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, int 
0x00000000, int 0x00000000, unsigned int 0x00000000, unsigned int & 0x00000000) 
line 693 + 31 bytes
ViewportFrame::Reflow(ViewportFrame * const 0x013d5ab8, nsIPresContext * 
0x05627b90, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0x00000000) line 546
nsHTMLReflowCommand::Dispatch(nsHTMLReflowCommand * const 0x056596c0, 
nsIPresContext * 0x05627b90, nsHTMLReflowMetrics & {...}, const nsSize & {...}, 
nsIRenderingContext & {...}) line 146
PresShell::ProcessReflowCommands(int 0x00000001) line 5106
ReflowEvent::HandleEvent() line 4989
HandlePLEvent(ReflowEvent * 0x0565af20) line 5003
PL_HandleEvent(PLEvent * 0x0565af20) line 576 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00ac70d0) line 509 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x065903fc, unsigned int 0x0000c0c7, unsigned int 
0x00000000, long 0x00ac70d0) line 1054 + 9 bytes
USER32! 77e71820()
00ac70d0()

------------------------------------------
I am only seeing this when I press reload on simplified
page. 

Since I will be out of cube until March, I will cc: 
people that may see this problem.
-p

Comment 4

[pnunn]

Attachment: even simpler test

Comment 5

[pnunn]

ah, yes. and to get started, put 
breaks in :
IL_DestroyImage(), mozilla/modules/libimg/src/ilclient.cpp
IL_GetImage(), mozilla/modules/libimg/src/if.cpp

somewhere where you can watch the url string to monitor
which image is requested and then immediately destroyed.

-p

Comment 6

[Keyser Sose]

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

Comment 7

[Jerry Baker]

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

Comment 8

[Jesse Ruderman]

I think this is a dup of bug 52798 by way of bug 58645.

Comment 9

[Phil Peterson]

topperf keyword. expedia.com now using this, which causes performance
measurement to be artificially high.

Comment 10

[Marc Attinasi]

Temporarily taking to investigate...

Comment 11

[Gagan]

cc'ing pavlov

Comment 12

[Marc Attinasi]

This is a nasty feedback loop between the nsImageFrame::UpdateImageFrame method
and the nsImageFrame::Reflow method. Basically, the problem is that
UpdateImageFrame is called when the image is loaded, and it causes a reflow,
which causes the ImageLoader to start loading the image again (killing the old
loader) which ends up notifying the image frame again, which causes another
reflow and so on and so on. I believe that this only happens on reload because
of a subtle timing issue: if the image is loaded fast enough, then the reflow
does not complete before the next image notification somes in, hence the cycle.

I have a fix that breaks the cycle (because it prevents the reflows of the image
frame if the size is not dependent on the intrinsic size of the image), but I am
afraid that it is not really the correct fix for this bug, but just masks the
bigger problem of the ImageLoader and ImageFrame being just plain evil.

Aside from revamping the entire image loader / image frame interaction, probably
the next best fix is to prevent the notification callback for the image load
when we know that we do NOT need the intrinsic size of the image. In other
words, squelch the image loader callback when the intrinsic size is irrelevant.
I'll try it out with some various testcases...

Comment 13

[John Morrison]

By the way, this is also leaking quite a bit [according to purify],
particularly from here. (note: il_get_container does a PR_Calloc of an
il_container; Purify doesn't show it, but the actual allocation is

478     /* There's no existing matching container.  Make a new one. */
479     if (!ic) {
480         ic = PR_NEWZAP(il_container);
481         if (!ic)
482             return NULL;

But, pavlov may have changed all this in his branch).

[W] MLK: Memory leak of 53928 bytes from 504 blocks allocated in PR_Calloc
        Distribution of leaked blocks
        Allocation location
            calloc         [msvcrt.DLL]
            PR_Calloc      [prmem.c:43]
                }
                
                PR_IMPLEMENT(void *) PR_Calloc(PRUint32 nelem, PRUint32 elsize)
             => {
                #if defined (WIN16)
                    return PR_MD_calloc( (size_t)nelem, (size_t)elsize );
                
            IL_GetImage    [if.cpp:1908]
                
                    ic = il_get_container(img_cx, cache_reload_policy, image_url,
                                          background_color, img_cx->dither_mode,
             =>                           req_depth, req_width, req_height);
                    if (!ic)
                    {
                        ILTRACE(0,("il: MEM il_container"));
            ImageRequestImpl::Init(void *,char const*,nsIImageRequestObserver *,
                                   UINT const*,UINT,UINT,UINT,ilINetContext *) 
                                   [nsImageRequest.cpp:260]
            ImageGroupImpl::GetImage(char const*,nsIImageRequestObserver *,
                                     UINT const*,UINT,UINT,UINT) 
                                     [nsImageGroup.cpp:282]
            nsFrameImageLoader::Init(nsIPresContext *,nsIImageGroup *,nsString
const&,UINT const*,nsSize const*,nsIFrame *,nsImageAnimation,(*)(nsIPresContext
*,nsIFrameImageLoader *,nsIFrame *,void *,UINT),void *,void *)
[nsFrameImageLoader.cpp:183]
            nsPresContext::StartLoadImage(nsString const&,UINT const*,nsSize
const*,nsIFrame *,(*)(nsIPresContext *,nsIFrameImageLoader *,nsIFrame *,void
*,UINT),void *,void *,nsIFrameImageLoader * *) [nsPresContext.cpp:1141]
            nsHTMLImageLoader::StartLoadImage(nsIPresContext *)
[nsHTMLImageLoader.cpp:217]
            nsHTMLImageLoader::GetDesiredSize(nsIPresContext *,nsHTMLReflowState
const*,nsHTMLReflowMetrics&) [nsHTMLImageLoader.cpp:478]
            nsImageFrame::GetDesiredSize(nsIPresContext *,nsHTMLReflowState
const&,nsHTMLReflowMetrics&) [nsImageFrame.cpp:359]

Comment 14

[Stuart Parmenter]

this doesn't happen with the new imagelib

Comment 15

[Marc Attinasi]

Pavlov, should I even bother trying to fix this, or is it all obsolete because 
of the work you are doing?

Comment 16

[Stuart Parmenter]

don't worry about fixing it.  As soon as I check in and turn on the new code in
the iamge frame, element, etc, these will all go awaay.

Comment 17

[Marc Attinasi]

Pavlov, got a bug I can depend this one on?

Comment 18

[Stuart Parmenter]

feel free to mark it depending on 66967... unfortuantly, this bug doesn't have a
whole lot of info in it.  please give me a call if you have any questions.

Comment 19

[Marc Attinasi]

Marking dependent on pavlov's new imagelib stuff

Comment 20

[Marc Attinasi]

Attachment: Patch to fix the flood of notifications that serve no purpose and make us go really bonkers when the image is in the cache

Comment 21

[Scott Collins]

I see ... since the image is in the cache, we're all set, we don't need to be
told that we have the size and go through the extra trouble because the
size-as-cached is exactly right.  This seems good, and a good thing to check in
for the interim.  Best case, libpr0n obsoletes it quickly; worst case you've
improved performance while libpr0n landing is stalled indefinitely.  sr=scc.

Comment 22

[Marc Attinasi]

CC'ing karnaze - is this your problem? While you are at it, can you review the
patch? Thanks.

Comment 23

[Cathleen]

updated bug summary and depend list, to reflect dependency on get new image lib
landing.

(clearing out extra details to the dependency tree viewed by bug 71688, per
chofmann :-) )

Comment 24

[karnaze (gone)]

r=karnaze

Comment 25

[Marc Attinasi]

Mozilla 0.8.1 - checking in when the tree is clean...

Comment 26

[karnaze (gone)]

Moving to mozilla0.9

Comment 27

[Marc Attinasi]

Fix checked in.

Comment 28

[Terri Preston]

Verified fix checked into lxr.mozilla.org

Comment 29

[Jerry Baker]

Mass removing self from CC list.

Comment 30

[Jerry Baker]

Now I feel sumb because I have to add back. Sorry for the spam.