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

VERIFIED FIXED in mozilla1.4alpha

Status

()

Core
ImageLib
P1
normal
VERIFIED FIXED
15 years ago
14 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({mlk})

Trunk
mozilla1.4alpha
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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....
Priority: -- → P1
Summary: Make imgRequestProxy hold a weak ref to the decoder observer → [FIX]Make imgRequestProxy hold a weak ref to the decoder observer
Target Milestone: --- → mozilla1.4alpha
Created attachment 116827 [details] [diff] [review]
Surpsingly many callers already Canceled the request properly...
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?
Attachment #116827 - Flags: superreview?(jst)
Attachment #116827 - Flags: review?(pavlov)
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 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.
Attachment #116827 - Flags: superreview?(jst) → superreview+
Created attachment 117042 [details] [diff] [review]
I missed a spot; this patch is in addition to the other one
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....
Attachment #117042 - Flags: superreview?(jst)
Attachment #117042 - Flags: review?(pavlov)
Comment on attachment 117042 [details] [diff] [review]
I missed a spot; this patch is in addition to the other one

sr=jst
Attachment #117042 - Flags: superreview?(jst) → superreview+
Created attachment 117451 [details] [diff] [review]
per some comments on irc....
Attachment #116827 - Attachment is obsolete: true
Attachment #117042 - Attachment is obsolete: true

Updated

15 years ago
Attachment #117451 - Flags: review+
Summary: [FIX]Make imgRequestProxy hold a weak ref to the decoder observer → [FIXr]Make imgRequestProxy hold a weak ref to the decoder observer
Attachment #117042 - Flags: review?(pavlov)
Attachment #116827 - Flags: review?(pavlov)
Checked in.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 10

14 years ago
Verified per last comments
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.