Last Comment Bug 196797 - [FIXr]Make imgRequestProxy hold a weak ref to the decoder observer
: [FIXr]Make imgRequestProxy hold a weak ref to the decoder observer
Status: VERIFIED FIXED
: mlk
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86 Linux
: P1 normal (vote)
: mozilla1.4alpha
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
: Terri Preston
Mentors:
Depends on:
Blocks: 83774
  Show dependency treegraph
 
Reported: 2003-03-10 19:19 PST by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2003-04-23 15:06 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Surpsingly many callers already Canceled the request properly... (14.59 KB, patch)
2003-03-10 19:21 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
jst: superreview+
Details | Diff | Review
I missed a spot; this patch is in addition to the other one (941 bytes, patch)
2003-03-12 19:30 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
jst: superreview+
Details | Diff | Review
per some comments on irc.... (15.41 KB, patch)
2003-03-16 22:23 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
pavlov: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2003-03-10 19:19:28 PST
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 [:bz] (Out June 25-July 6) 2003-03-10 19:21:15 PST
Created attachment 116827 [details] [diff] [review]
Surpsingly many callers already Canceled the request properly...
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2003-03-10 19:22:17 PST
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 [:bz] (Out June 25-July 6) 2003-03-10 19:27:36 PST
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, jst@mozilla.com) 2003-03-11 17:01:34 PST
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 [:bz] (Out June 25-July 6) 2003-03-12 19:30:39 PST
Created attachment 117042 [details] [diff] [review]
I missed a spot; this patch is in addition to the other one
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2003-03-12 19:31:36 PST
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, jst@mozilla.com) 2003-03-12 23:10:15 PST
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 [:bz] (Out June 25-July 6) 2003-03-16 22:23:10 PST
Created attachment 117451 [details] [diff] [review]
per some comments on irc....
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2003-03-17 10:12:37 PST
Checked in.
Comment 10 Chris Petersen 2003-04-23 15:06:51 PDT
Verified per last comments

Note You need to log in before you can comment on or make changes to this bug.