196797 - [FIXr]Make imgRequestProxy hold a weak ref to the decoder observer

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

This is basically to prevent reference cycles... As things stand,
nsImageDocument was only not leaking because of explicit "set the request to
null on document unload" code.  nsImageLoader was leaking (so all background
images got leaked, for example; this includes all list-style-image images in
XUL, of course).  nsImageLoadingContent would have had to put in a bunch of code
to not leak...

I think that this change is a decent way to resolve the issue -- it puts the
ownership in the caller's hands.

I don't much like the refcounting in nsTreeBodyFrame, but that code was already
pretty broken (passing a refcount-0 object through an XPCOM interface and
all...); if desired, I can muck with it a bit to store both the imgIRequest and
the listener in the cache entry....

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Surpsingly many callers already Canceled the request properly...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 116827 [details] [diff] [review]
Surpsingly many callers already Canceled the request properly...

jst, can you get to this review in the next few days?  Or are you busy?  If the
latter, who'd be a good sr for this?  peterv?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Erm, that should be:

+  NS_ASSERTION(observer, "No observer?  We're leaking!");

instead of

+  NS_ASSERTION(observer, "No observer?  We're leaking!\n");

(fixed in my tree).

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 116827 [details] [diff] [review]
Surpsingly many callers already Canceled the request properly...

- In nsImageDocument::SetScriptGlobalObject():

+    mImageRequest->Cancel(NS_ERROR_FAILURE);

I don't see any code that guarantees that mImageRequest is non-null here, a
null check seems appropriate here.


sr=jst with that change.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: I missed a spot; this patch is in addition to the other one

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 117042 [details] [diff] [review]
I missed a spot; this patch is in addition to the other one

Here's hoping for some review love from pavlov....

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 117042 [details] [diff] [review]
I missed a spot; this patch is in addition to the other one

sr=jst

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: per some comments on irc....

Comment 9

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 10

[Chris Petersen]

Verified per last comments