179498 - memory leak on reloading an image

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrew Schultz]

originally reported in bug 174760 with linux CVS trunk build 20021108

Reloaded car2.jpg from the URL (locally) about 12 times.
Valgrind came up with this:

676 bytes in 13 blocks are possibly lost
 malloc (vg_clientfuncs.c:100)
 __builtin_new (nsAppRunner.cpp:178)
 operator new(unsigned) (vg_clientfuncs.c:139)
 NS_NewHTMLImageElement() (nsHTMLImageElement.cpp:181)
 nsImageDocument::CreateSyntheticDocument() (nsImageDocument.cpp:329)
 nsImageDocument::StartDocumentLoad() (nsImageDocument.cpp:265)
 nsContentDLF::CreateDocument() (nsContentDLF.cpp:435)
 nsContentDLF::CreateInstance() (nsContentDLF.cpp:233)

and
1040 bytes in 13 blocks are possibly lost
 malloc (vg_clientfuncs.c:100)
 __builtin_new (nsAppRunner.cpp:178)
 operator new(unsigned) (vg_clientfuncs.c:139)
 nsScriptSecurityManager::GetCodebasePrincipal() (nsScriptSecurityManager.cpp:1744)
 nsDocument::GetPrincipal() (nsDocument.cpp:915)
 GlobalWindowImpl::GetPrincipal() (nsGlobalWindow.cpp:895)
 nsScriptSecurityManager::GetPrincipalAndFrame() (nsScriptSecurityManager.cpp:1876)
 nsScriptSecurityManager::GetSubjectPrincipal() (nsScriptSecurityManager.cpp:1895)

the first chunk looks more relevant, but both have lucky number 13.

hopefully this component is appropriate.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> NS_NewHTMLImageElement() (nsHTMLImageElement.cpp:181)

This looks bogus; we release the pointer at the end of the function (though
switching this code to nsCOMPtr may be a good idea in any case).

> nsScriptSecurityManager::GetCodebasePrincipal() (nsScriptSecurityManager.cpp:1744)

That looks bogus-ish as well; we release it properly in the nsDocument
destructor....

Comment 2

[Andrew Schultz]

the nsHTMLImageElement is ADDREF'd once at nsHTMLImageElement:955 and then again
with this stack:
#3  nsGenericHTMLContainerElement::AppendChildTo(nsIContent*, int, int) (
    this=0x8770f08, aKid=0x86aa7c8, aNotify=0, aDeepSetDocument=0) at 
    nsGenericHTMLElement.cpp:4069
#4  nsImageDocument::CreateSyntheticDocument() (this=0x87a4158)
    at nsImageDocument.cpp:359

It is RELEASE'd only once (nsImageDocument.cpp:362), hence the memory leak.

Comment 3

[Harshal Pradhan]

This bug is bad. Pretty big leak.

Valgrind is telling the truth. But its telling only a very small part of the
truth. The image element and the priciple that it showed are small fish. I'll
attach logs that show we are leaking a whole lot of stuff from all over the
place for every nsImageDocument that we construct. As such, to hit this there is
no need to reload the same image, even the first load leaks.

It looks like there is a cycle involving nsImageDocument and the imgRequest,
probably through ImageListener. I dont know enough about imglib to be able to
specify exacty all the links in the cycle; but we end up leaking a ton stuff
from imglib and layout, along with all the stuff that the document owns. 

Explicitly dropping the documents ref to the img nsImageDocument::EndLayout()
appears to successfully break the cycle without causing any other problems.

Patch/leaklogs in a moment. 

Comment 4

[Harshal Pradhan]

Attachment: fix as described above (also kill a unused warning while i'm here)

Comment 5

[Harshal Pradhan]

Attachment: before/after leak logs

Logs collected by doing simply:
* Start up mozilla
* Load http://foo/bar.jpg
* Shutdown mozilla.

The first log is from before the patch, the second after.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, good catch....

One thought (and this would test your ImageListener hypothesis, which seems to
me to be the mosst likely one).  The document may want to have the imgIRequest
later so it can get image info out (eg for enhancements to page info).  But once
the ImageListener has done its OnStopRequest, it does not need the mDocument
ref.  So maybe we should null out mDocument in ImageListener::OnStopRequest? 
Does that also fix the leak?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I just traced the ImageListener ownership, and it _looks_ like that's only owned
by the URILoader, which releases it in OnStopRequest...

Comment 8

[Harshal Pradhan]

Yep, the cycle is not through ImageListener.

Looks to me like it goes something like this:

ImageDocument > imgRequestProxy > GalleyContext(nsPresContext)
                                        |> nsEventStateManager > ImageDocument

Does that sound correct? If it is correct, do we have a better way of breaking
that cycle than my patch above? Who would be the right person to review this?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yeah, that's correct.  :(  The problem is basically that the document ends up
owning the prescontext through this mechanism... :(

The esm has this bizarre "[OWNER], but doesn't need to be." comment on
mDocument... is that an option here?  Could the prescontext drop the ESM ref
when its container is set to null, maybe?

Of course we can also just use the patch we already have, but then any other
object owned by the document which holds a ref to an imgIRequest could cause
problems (eg there are thoughts to put those in the style system's storage
structs, which are owned by the doc in the end, etc.)

Also, if we ever make imgIRequest hold a ref to a document (instead of a
prescontext) the whole thing becomes even wose... :(

ccing some people who should be aware of this... I'd say that dbaron or bryner
are the best reviewers for this stuff.

One other option is to put the nulling out in
nsImageDocument::SetScriptGlobalObject or something along those lines (see
nsDocument).

Comment 10

[Harshal Pradhan]

Attachment: alternative patch (do the nulling out in SetScriptGlobalObject)

I like bz's SetScriptGlobalObject idea better than the earlier patch (I don't
know enough about the esm yet to say if it needs the document ref or not.)

BTW, nsHTMLImageElement itself explicity drops its ref to its imgRequest in
OnStopDecode(), so this sort of thing seem to have precedent.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 109010 [details] [diff] [review]
alternative patch (do the nulling out in SetScriptGlobalObject)

r/sr=me....

We still have a problem in that eventually we will want to have the style
system holding refs to imgIRequest objects... I guess we'll just have to make
sure we clear our all that stuff carefully.  ;)

Comment 12

[Harshal Pradhan]

Taking bug, since I have the patch.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 109010 [details] [diff] [review]
alternative patch (do the nulling out in SetScriptGlobalObject)

Nice catch! r/sr=jst

Comment 14

[Boris Zbarsky [:bzbarsky]]

patch checked in.

Comment 15

[Harshal Pradhan]

Thanks bz!

Comment 16

[Harshal Pradhan]

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

Comment 17

[Christian Hamacher]

Confirming fixed on Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.3a)
Gecko/20021214

After loading a huge set of different JPGs, either in tabs or windows, an
closing them again, memory consumption now goes back down to its original value.

Harshal, thanks a lot - you are my hero :-) 

This bug was a royal pain in the nether regions!

Comment 18

[Harshal Pradhan]

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Comment 20

[Mrten]

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

Comment 21

[Dvir]

This bug seems to have regressed in the nightlies of 1.4. Memory load goes
drastically higher when many images loaded/unloaded, just like it was before
this bug was fixed.

I tested this with the 17/4 build, and also with a recent Phoenix build.