Closed Bug 321054 Opened 19 years ago Closed 18 years ago

fix GC hazards for loading images created by bug 241518

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Future

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: fixed1.8.1, helpwanted, Whiteboard: [patch])

Attachments

(4 files, 12 obsolete files)

IRC discussion on #developers about image loading issues here
7.51 KB, text/plain; charset=utf-8
Details
testcase showing the bug (always broken in Mozilla)
708 bytes, text/html; charset=UTF-8
Details
testcase showing the bug (regression)
729 bytes, text/html; charset=UTF-8
Details
patch
74.48 KB, patch
bzbarsky
: superreview+
Details | Diff | Splinter Review 
There are potential GC hazards created by bug 241518 -- we need to audit to make sure these don't exist.

For example, something like (untested code, may contain errors):

function foo() {
  var i = new Image();
  i.src = "http://www.example.com/slow-image.gif";
  i.onload = function(event) { document.appendChild(event.target); }
}

could potentially have the whole thing garbage-collected if nothing about the "loading" process keeps the image content node alive.
Actually, just holding on to the content node wouldn't fix it; I'd need to think more about what would.
David, I think the ephemerality of expandos on non-nodes is a real bug, not related to the dropped-on-floor-new-Image bug.  In any browser I know of, back to Netscape 3 where new Image was added first, script has to keep a ref to each new Image instance in order for loads to complete and be useful.

/be
Summary: audit for GC hazards created by bug 241518 → fix GC hazards created by bug 241518
Anything where the behavior of a script depends on when a GC runs is a bug.  In real script, GCs are relatively rare, so authors are unlikely to be able to detect and debug the problem successfully.  Garbage collection shouldn't collect things that aren't garbage.
(Excluding the influence of GC on timing, that is.)
As far as comment 0 goes, image loading does NOT hold a strong reference to the image loader observer (see bug 196797 for why).  So yes, in that case the whole thing could get garbage-collected...

I also agree with comment 4.  So we need to do something about it.  :(
(In reply to comment #5)
> As far as comment 0 goes, image loading does NOT hold a strong reference to the
> image loader observer (see bug 196797 for why).

... which wouldn't even be sufficient, as I briefly stated in comment 1, since the event listener could still be collected even if it were.

(I should probably file a separate bug on .parentNode and comment 3-comment 4.)

Bizarrely enough, the bug in my code in comment 0 was that |event.target| is the document; I had to use |this| instead.
David, I bet the event.target bustage you saw is basically bug 295340...
This is some significant work in progress.  However, I've run into major problems for both bugs:
 * for this bug, it's hard for me to figure out where to hook into the image code
 * for the XMLHttpRequest code, it's hard to figure out how to get a customized scriptable helper in external classinfo -- I really want to inherit from nsDOMClassInfo.

Also, I haven't done the legwork to call both nsMarkedJSFunctionHolder and my new nsDOMClassInfo static methods from outside libgklayout.so.
(In reply to comment #9)
>  * for the XMLHttpRequest code, it's hard to figure out how to get a customized
> scriptable helper in external classinfo -- I really want to inherit from
> nsDOMClassInfo.

I do remember working on something like that, don't remember for which use case exactly. We could make the code in http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/src/base/nsDOMClassInfo.cpp&rev=1.344#3558 pass in a nsIClassInfo created with nsDOMGenericSH::doCreate(aData) which you could use to forward certain methods to. It's not real inheritance though, what are you trying to do exactly?
FWIW, the patch in bug 323534 adds a scriptable helper (nsDOMGCParticipantSH) that does exactly what I want here as well.

Bugzilla won't let me add it as a dependency, though, since it creates a cycle.  We need separate fields for dependencies and regressions!
Attached patch work in progress (obsolete) — — Splinter Review 

        Attachment #208457 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 217210 [details] [diff] [review]
work in progress

>-class ImageEvent : public PLEvent
>+class nsImageLoadingContent::Event : public PLEvent

Note that declaration of nested classes outside of their enclosing class is already used in the tree by nsPropertyTable::PropertyList and nsFrameManagerBase::UndisplayedMap.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        work in progress (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #217210 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
Well, I haven't hit any of my assertions in a few minutes of browsing.  (And they do have a pretty good chance of firing; a leak that would suppress them would only happen for images that actually have onload/onerror handlers.)

I expect this will break *something*, though.

        Attachment #217252 -
        Attachment is obsolete: true

        Attachment #217317 -
        Flags: superreview?(jst)

        Attachment #217317 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 217317 [details] [diff] [review]
patch

All those requests, and I attached the wrong patch.

        Attachment #217317 -
        Flags: superreview?(jst)

        Attachment #217317 -
        Flags: review?(pavlov)

        Attachment #217317 -
        Flags: review?(bzbarsky)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #217317 -
        Attachment is obsolete: true

        Attachment #217318 -
        Flags: superreview?(bzbarsky)

        Attachment #217318 -
        Flags: review?(jst)

    
    

    
  
I'd also note that I don't consider this code anywhere remotely close to maintainable -- the randomly scattered RootParticipant and UnrootParticipant calls are just manual reference counting.  The only saving grace is that I've added assertions that ought to fire if things go wrong.

    
    

    
  
(In reply to comment #21)
> randomly scattered RootParticipant and UnrootParticipant calls

sorry, PreserveLoadHandlers and UnpreserveLoadHandlers calls

    
    

    
  
Comment on attachment 217337 [details]
testcase showing the bug (always broken in Mozilla)

...although this testcase doesn't work in 1.5 either, because of the i = null I needed to add to make it break on the trunk.  But in theory it really ought to break on the trunk without the i = null; I'm not sure why it doesn't.

        Attachment #217318 -
        Flags: superreview?(bzbarsky)

        Attachment #217318 -
        Flags: review?(pavlov)

        Attachment #217318 -
        Flags: review?(jst)

    
    

    
  
Comment on attachment 217318 [details] [diff] [review]
patch

       mListener = nsnull;

-      mOwner->RemoveProxy(this, NS_OK, PR_FALSE);
+      mOwner->RemoveProxy(this, NS_OK, PR_TRUE);

This won't actually do anything since mListener will be null.  You'll want to change around the comments a bit and actually test this pretty heavily.

        Attachment #217318 -
        Flags: review-

    
    

    
  
The most related changes seem to be:

revision 1.17
date: 2001/04/13 02:42:56;  author: pavlov%netscape.com;  state: Exp;  lines: +89 -59
fix for at least bugs 6074,72087,74506,75190,75180,74165,69857,75576,75326,75417,75474 r=waterson, sr=brendan

revision 1.34
date: 2002/01/15 05:23:33;  author: pavlov%netscape.com;  state: Exp;  lines: +23 -3
fixing multiple reload of same image on reload.  bug 108161 (and others) r=bryner sr=darin

Flags: blocking1.9a1?
Target Milestone: --- → Future

        Attachment #217337 -
        Attachment description: testcase showing the bug → testcase showing the bug (always broken in Mozilla)

    
    

    
  
So this patch fires a bunch of assertions for JPEG server push cases, like http://www.berkeley.edu/webcams/sproul.html , since a single nsImageLoadingContent::LoadImage call leads to many nsImageLoadingContent::OnStopDecode calls -- in other words, a single load causes multiple onload events.  (At least, that's what happens with my patch.)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        work in progress (obsolete)
        — 
          — Splinter Review
      

    


  
Here's the current state of my patch.

        Attachment #217318 -
        Attachment is obsolete: true

    
    

    
  
This patch also has the following problem:

#12 0x00ea5695 in __cxa_pure_virtual ()
    at ../../../../libstdc++-v3/libsupc++/pure.cc:55
#13 0x06a4bbde in nsCOMPtr (this=0xbf99a954, aRawPtr=0xa6ccb84)
    at ../../../dist/include/xpcom/nsCOMPtr.h:627
#14 0x06a4a3c7 in imgRequestProxy::OnStopDecode (this=0xa02b688,
    status=2147500037, statusArg=0x0)
    at /builds/leak/mozilla/modules/libpr0n/src/imgRequestProxy.cpp:440
#15 0x06a4794a in imgRequest::RemoveProxy (this=0xa6cdaf0, proxy=0xa02b688,
    aStatus=2147500037, aNotify=1)
    at /builds/leak/mozilla/modules/libpr0n/src/imgRequest.cpp:155
#16 0x06a4adfe in imgRequestProxy::Cancel (this=0xa02b688, status=2147500037)
    at /builds/leak/mozilla/modules/libpr0n/src/imgRequestProxy.cpp:200
#17 0x05db0aff in ~nsImageLoadingContent (this=0xa6ccb84)
    at /builds/leak/mozilla/content/base/src/nsImageLoadingContent.cpp:125
#18 0x05e572c3 in ~nsHTMLImageElement (this=0xa6ccb68)
    at /builds/leak/mozilla/content/html/content/src/nsHTMLImageElement.cpp:187
#19 0x05da2d5b in nsGenericElement::Release (this=0xa6ccb68)
    at /builds/leak/mozilla/content/base/src/nsGenericElement.cpp:3131
#20 0x05e571e5 in nsHTMLImageElement::Release (this=0xa6ccb68)
    at /builds/leak/mozilla/content/html/content/src/nsHTMLImageElement.cpp:191
#21 0x033266e6 in XPCJSRuntime::GCCallback (cx=0xa3cee48, status=JSGC_END)
    at /builds/leak/mozilla/js/src/xpconnect/src/xpcjsruntime.cpp:562
#22 0x05f95518 in DOMGCCallback (cx=0xa3cee48, status=JSGC_END)
    at /builds/leak/mozilla/dom/src/base/nsJSEnvironment.cpp:2208
#23 0x00c2db12 in js_GC (cx=0xa3cee48, gcflags=0)
    at /builds/leak/mozilla/js/src/jsgc.c:2254


        Attachment #218474 -
        Attachment description: patch → work in progress

    
    

    
  

      
      
      
      

        Attached patch
          
          
        work in progress (obsolete)
        — 
          — Splinter Review
      

    


  
This would have fixed the known problems with the previous patch *if* nsIMultiPartChannel::GetIsLastPart were reliable.  It's not.  If a multipart/mixed load is canceled between parts (the norm for a progressive JPEG -- leaving the page while not in the middle of a part load), the listener gets no notification of the fact that the multipart/mixed load as a whole has finished -- and that's the notification that I really need.

    
    

    
  
Comment on attachment 220990 [details] [diff] [review]
work in progress

Note that the nsDOMClassInfo changes are now on bug 206520 instead of here, since that's likely to land first.

Also, the nsHTMLTableColElement.cpp change wasn't supposed to be part of this patch.

    
    

    
  
> This would have fixed the known problems with the previous patch *if*
> nsIMultiPartChannel::GetIsLastPart were reliable.  It's not.  If a
> multipart/mixed load is canceled between parts (the norm for a progressive JPEG
> -- leaving the page while not in the middle of a part load), the listener gets
> no notification of the fact that the multipart/mixed load as a whole has
> finished -- and that's the notification that I really need.

You can't tell by inspecting the part channel's status?  (nsIRequest::GetStatus)

    
    

    
  
How could I if I don't get any notification to do so?  And there's no part channel at all since the multipart/x-mixed-replace connection closed *after* the notifications for (what in the end turned out to be) the last part were sent out.

    
    

    
  
(In any case, I think I've figured out how to restrict the problems this causes to leaking in cases where a multipart/x-mixed-replace image has a load event handler function closure, so it's not so bad...)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #218474 -
        Attachment is obsolete: true

        Attachment #220990 -
        Attachment is obsolete: true

    
    

    
  
Ah, okay.  If the multipart channel is not generating an OnStopRequest when it is canceled, then that is a critical bug.  I misunderstood your comment because I focused on the bit where you said that if only GetIsLastPart were reliable :-/

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #221017 -
        Attachment is obsolete: true

        Attachment #221136 -
        Flags: superreview?(bzbarsky)

        Attachment #221136 -
        Flags: review?(pavlov)

    
  
Summary: fix GC hazards created by bug 241518 → fix GC hazards for loading images created by bug 241518

    
    

    
  
So here's the basic idea here, for reviewers.  Start with bug 206520 comment 14, since that code is used here as well.

Then the basic idea is that I need to use the API described in that comment, symetrically, whenever an image is in a state that it could fire an onload or onerror event.  This basically means at two points:
 * when we're waiting for a network request to return
 * when we've posted an event to the event queue so that we fire onload or onerror (since that's asynchronous in itself).

The former is a bit difficult, since the image code, especially for multipart/x-mixed-replace doesn't make it particularly easy to know when something is done loading.  I've done a bit to this by propagating OnStartRequest and (more importantly, here) OnStopRequest to the imgIDecoderObservers that observer imgIRequest objects.  I've also added code that will make this work correctly for multipart/x-mixed-replace streams that close themselves at the end of their last part (not sure exactly what the timing issue is here).  This patch isn't quite correct for multipart/x-mixed-replace that's stopped by a load group cancel between parts, since we never get an OnStopRequest that says it's the last part; that's a deficiency of the multipart converter.  So this patch reintroduces the leak fixed in bug 241518 for only that case:  multipart/x-mixed-replace images that have javascript closures as event listeners.  The nsDOMClassInfo changes in bug 206520 are intentionally exact so that I don't recreate the leak for more cases.

    
    

    
  
Comment on attachment 221136 [details] [diff] [review]
patch

>Index: modules/libpr0n/public/imgIDecoderObserver.idl

>+   * called at the same time that nsIRequestObserver::onStartRequest would be
>+   * (used only for observers of imgIRequest objects, not imgIDecoder objects)
We should probably document that this can at present be triggered sync in some cases (unlike nsIRequestObserver::onStartRequest), right?

>+   * called at the same time that nsIRequestObserver::onStopRequest would be
>+   * (used only for observers of imgIRequest objects, not imgIDecoder objects)
Same here.

>Index: modules/libpr0n/src/imgRequestProxy.cpp

>imgRequestProxy::~imgRequestProxy()
>+         Passing false to aNotify means that mListener will still get
>+         OnStopRequest, if needed.

And we'll pass |this| to it, right?  In that case, we should document that the request passed to onStopRequest must not be accessed outside of OnStopRequest, since it might get destroyed immediately after the function returns, even if the callee takes a ref to it.  :(  In general, this does make me a little queasy, though.  We should file a followup bug on trunk, perhaps, to make the ownership model happier on trunk?

> NS_IMETHODIMP imgRequestProxy::Cancel(nsresult status)

So here I think we want a kungFuDeathGrip to ourselves.  Otherwise, consider a caller whose OnStopRequest drops the ref to the imgRequestProxy and which calls Cancel().  By the time we go to set mListener, we're deleted.  Alternately, we could make this the caller's responsibility, given the possibly sync semantics of this stuff, but I think we want to change to fully async semantics in general and it's better to just have the ugliness in one place, imo.

Please file a followup bug on the multipart converter? It should probably just not send OnStartRequest until either it's ready for the OnStartRequest for the next part or is done completely; that way it's guaranteed to get the "last part" boolean correct.

>Index: content/base/src/nsImageLoadingContent.cpp
>+#include "nsDOMClassInfo.h"

We're including this for the SetExternallyReferenced stuff, right?  Are you adding this with this patch, since I don't see it in lxr?  If so, could you attach diffs for classinfo?

>+  // This can actually fire for multipart/x-mixed-replace, since if the
>+  // load is canceled between parts (e.g., by cancelling the load
>+  // group), we won't get any notification.  See bug 321054 comment 31.
>+  // *If* that multipart/x-mixed-replace image has event handlers, we
>+  // won't even get to this warning; we'll leak instead.
>+  NS_WARN_IF_FALSE(mRootRefCount == 0,
>+                   "unbalanced handler preservation refcount");

Mention in this comment the bug# of the bug you file to fix the multimixed conv?  And mention this code block in that bug?  Once that's fixed I assume we could make this an assertion and remove the following block, right?

>@@ -395,51 +441,57 @@ nsImageLoadingContent::LoadImageWithChan
>+  if (NS_FAILED(rv))
>+    UnpreserveLoadHandlers();

Local style is to brace even one-line bodies.

> nsImageLoadingContent::LoadImage(const nsAString& aNewURI,
>+    // XXXldb Why duplicate the check in LoadImage below?

I thought we'd decided this was a merge error?  Just fix it.  ;)

> nsImageLoadingContent::LoadImage(nsIURI* aNewURI,

>+    // REMOVE BEFORE CHECKIN: Fix merging error between 1156 and 269125
>+    FireEvent(NS_LITERAL_STRING("error"));

I'd just remove the comment and fix this too.

>-class ImageEvent : public PLEvent
>+class nsImageLoadingContent::Event : public PLEvent

These changes will need merging to the threadmanager landing.  :(  Looks ok for 1.8 branch, though (although there you'll need to merge a tiny bit because it doesn't have nsEventDispatcher).

>Index: content/base/src/nsImageLoadingContent.h

>+  PRUint8 mRootRefCount;

Maybe document what this is (and esp. why a PRUint8 is big enough)?  I agree that it should be, fwiw; in fact I have a hard time seeing this refcount ever get above about 10, no matter what sync evil I concoct.

>Index: layout/base/nsPresContext.h
>-        NS_WARN_IF_FALSE(mPresContext->mLayoutPhaseCount[eLayoutPhase_FrameC] == 0,
>+        NS_ASSERTION(mPresContext->mLayoutPhaseCount[eLayoutPhase_FrameC] == 0,

This change doesn't belong to this patch, right?

sr=bzbarsky

        Attachment #221136 -
        Flags: superreview?(bzbarsky) → superreview+

    
    

    
  
I should clarify that the sr is conditional pending the classinfo patch.

    
    

    
  
OK, it looks like the classinfo changes are in attachment 221016 [details] [diff] [review] (on bug 206520).  Some comments on those:

>+PreservedWrapperClearEntry(PLDHashTable *table, PLDHashEntryHdr *hdr)
...
>+  if (entry->rootWhenExternallyReferenced) {

We should assert in this if body that sRootWhenExternallyReferencedTable.ops is not null, probably.

>nsDOMClassInfo::PreserveWrapper(void *aKey,
...
>+  if (aRootWhenExternallyReferenced) {
>+    if (!sRootWhenExternallyReferencedTable.ops &&
>+        !PL_DHashTableInit(&sRootWhenExternallyReferencedTable,
>+                           PL_DHashGetStubOps(), nsnull,
>+                           sizeof(RootWhenExternallyReferencedEntry), 16))
>+      return NS_ERROR_OUT_OF_MEMORY;

Shouldn't we remove the entry we just added to sPreservedWrapperTable and which has rootWhenExternallyReferenced set to true?  Otherwise we'll hit that ops == nsnull case above...

Similar if we fail to add the entry to sRootWhenExternallyReferencedTable.

>+    rwerEntry->participant = aParticipant;
>+    ++rwerEntry->refcnt;

Can we assert that either rwerEntry->refcnt == 0 or rwerEntry->participant == aParticipant?

I'm trying to understand the logic in nsDOMClassInfo::PreserveWrapper.  Say I add an event handler to an image, then set some random property.  We'll first call PreserveWrapper with aRootWhenExternallyReferenced true, then with it false.  This will trigger the assertand then fail to actually root when externally referenced, if I understand this code correctly.  Or am I missing something?  This is the one real problem I see in the setup.

    
    

    
  
(In reply to comment #42)
> I'm trying to understand the logic in nsDOMClassInfo::PreserveWrapper.  Say I
> add an event handler to an image then set some random property.  We'll first
> call PreserveWrapper with aRootWhenExternallyReferenced true,

...and aKey == the nsMarkedJSFunctionHolder<nsIDOMEventListener>

> then with it false.

... and aKey == the nsIXPConnectWrappedNative

> This will trigger the assert and then fail to actually root when
> externally referenced, if I understand this code correctly.  Or am I missing
> something?  This is the one real problem I see in the setup.

no, since aKey is different, and there are two separate entries in the preserved wrapper table

    
    

    
  
Ah, ok.  Most excellent.  So this fixes the addEventListener case but not the (never worked anyway) .onload= case, right?

sr=bzbarsky on the whole thing.

    
    

    
  
(In reply to comment #44)
> So this fixes the addEventListener case but not the
> (never worked anyway) .onload= case, right?

Do you have a testcase for that case?

    
    

    
  
Comment on attachment 221136 [details] [diff] [review]
patch

I'd like pavlov to review this eventually, but in order to get this on the 1.8 branch I may need to land this before that can happen, so requesting additional review from darin.

        Attachment #221136 -
        Flags: review?(darin)

    
    

    
  
Comment on attachment 221136 [details] [diff] [review]
patch

>Index: modules/libpr0n/public/imgIDecoderObserver.idl

> [scriptable, uuid(cce7152e-4395-4231-a781-c347c5446cc2)]
> interface imgIDecoderObserver : imgIContainerObserver
> {
>   /**
>+   * called at the same time that nsIRequestObserver::onStartRequest would be
>+   * (used only for observers of imgIRequest objects, not imgIDecoder objects)
>+   */
>+  void onStartRequest(in imgIRequest aRequest);

Interface changes require UUID bumps.  Remember also that a branch
patch must preserve APIs, so you're going to need imgIDecoderObserver2
or imgIDecoderObserver_MOZILLA_1_8_BRANCH.  It depends on whether or
not we want to change imgIDecoderObserver on the trunk, which we can
do because it is not frozen.

Reading the comment, I don't understand the connection to nsIRequestObserver.
This interface does not inherit from that interface, so how is it related?


>+  /**
>+   * called at the same time that nsIRequestObserver::onStopRequest would be
>+   * (used only for observers of imgIRequest objects, not imgIDecoder objects)
>+   */
>+  void onStopRequest(in imgIRequest aRequest, in boolean aIsLastPart);

Same here.  I think the comments need more details so it is clear what
this is for.


>Index: modules/libpr0n/src/imgRequestProxy.cpp

>   // It is important to call |SetLoadFlags()| before calling |Init()| because
>   // |Init()| adds the request to the loadgroup.
>+  // XXXldb That's not true anymore.  Stuff from imgLoader adds the
>+  // request to the loadgroup, but is that relevant?

When a request is added to a loadgroup, its load flags are merged
with the load flags of the loadgroup.


> void imgRequestProxy::OnStartRequest(nsIRequest *request, nsISupports *ctxt)
> {
...
>+  if (mListener) {
>+    // Hold a ref to the listener while we call it, just in case.
>+    nsCOMPtr<imgIDecoderObserver> kungFuDeathGrip(mListener);
>+    mListener->OnStartRequest(this);
>+  }
> }

Use of kungFuDeathGrips always look like a hack.  Maybe this is better:

    // mListener may be cleared during OnStartRequest
    nsCOMPtr<imgIDecoderObserver> listener = mListener;
    if (listener)
      listener->OnStartRequest(this);


>+  if (mListener) {
>+    // Hold a ref to the listener while we call it, just in case.
>+    nsCOMPtr<imgIDecoderObserver> kungFuDeathGrip(mListener);
>+    mListener->OnStopRequest(this, lastPart);
>   }

I recommend the same change here.


>Index: content/base/src/nsImageLoadingContent.cpp

>+/* static */ void* PR_CALLBACK
>+nsImageLoadingContent::Event::Handle(PLEvent* aEvent)

Sigh.  You're going to have some fun merging with the trunk.


>Index: content/svg/content/src/nsSVGImageElement.cpp

>+nsSVGImageElement::~nsSVGImageElement()
>+{
>+  DestroyImageLoadingContent();
>+}

It feels like this call should be in some base class's destructor,
or perhaps in the destructor of some member variable.


>Index: layout/base/nsPresContext.h

>+        NS_ASSERTION(mPresContext->mLayoutPhaseCount[eLayoutPhase_FrameC] == 0,
>                          "recurring into frame construction");

Was this meant to be part of this patch?  I hit that warning a bunch
on the trunk FWIW.


>Index: layout/svg/base/src/nsSVGImageFrame.cpp

>+NS_IMETHODIMP nsSVGImageListener::OnStartRequest(imgIRequest *aRequest)
>+{
>+  return NS_OK;
>+}
>+
> NS_IMETHODIMP nsSVGImageListener::OnStartDecode(imgIRequest *aRequest)
> {
>   return NS_OK;
> }
> 
> NS_IMETHODIMP nsSVGImageListener::OnStartContainer(imgIRequest *aRequest,
>                                                    imgIContainer *aImage)
> {
>   return NS_OK;
> }

Perhaps there should be some imgBaseDecoderObserver that stubs these
methods out so that their code may be shared.


r=darin w/ at least the interface change corrected

        Attachment #221136 -
        Flags: review?(darin) → review+

    
    

    
  
(In reply to comment #47)
> Reading the comment, I don't understand the connection to nsIRequestObserver.
> This interface does not inherit from that interface, so how is it related?

It's related at least because imgIRequest inherits from nsIRequest.

> >+  if (mListener) {
> >+    // Hold a ref to the listener while we call it, just in case.
> >+    nsCOMPtr<imgIDecoderObserver> kungFuDeathGrip(mListener);
> >+    mListener->OnStartRequest(this);
> >+  }
> > }
> 
> Use of kungFuDeathGrips always look like a hack.  Maybe this is better:
> 
>     // mListener may be cleared during OnStartRequest
>     nsCOMPtr<imgIDecoderObserver> listener = mListener;
>     if (listener)
>       listener->OnStartRequest(this);

It does make it clear that the author intended to have an additional AddRef/Release pair so that the object would be kept alive until the end of the call; calling it |listener| would not be as clear.

> >+nsSVGImageElement::~nsSVGImageElement()
> >+{
> >+  DestroyImageLoadingContent();
> >+}
> 
> It feels like this call should be in some base class's destructor,
> or perhaps in the destructor of some member variable.

This one in particular?  The problem is that it needs to happen before the derived class destructor starts calling base class destructors, because it needs to QI |this| to other base classes before they have been destroyed.  Otherwise we crash calling __pure_virtual (or whatever it's called)

> Was this meant to be part of this patch?

No.

> Perhaps there should be some imgBaseDecoderObserver that stubs these
> methods out so that their code may be shared.

A separate patch, perhaps?

    
    

    
  
> It's related at least because imgIRequest inherits from nsIRequest.

I think that should be made more clear from the comments.


> > Use of kungFuDeathGrips always look like a hack.  Maybe this is better:
...
> It does make it clear that the author intended to have an additional
> AddRef/Release pair so that the object would be kept alive until the end of the
> call; calling it |listener| would not be as clear.

That's why I suggested the comment.  The kungFuDeathGrip business reads like a band-aid to me :-/


> > >+nsSVGImageElement::~nsSVGImageElement()
> > >+{
> > >+  DestroyImageLoadingContent();
> > >+}
> > 
> > It feels like this call should be in some base class's destructor,
> > or perhaps in the destructor of some member variable.
> 
> This one in particular?

No, I meant this one and all the rest like it.


> The problem is that it needs to happen before the
> derived class destructor starts calling base class destructors, because it
> needs to QI |this| to other base classes before they have been destroyed. 
> Otherwise we crash calling __pure_virtual (or whatever it's called)

I see.


> > Perhaps there should be some imgBaseDecoderObserver that stubs these
> > methods out so that their code may be shared.
> 
> A separate patch, perhaps?

Sure.

    
    

    
  
(In reply to comment #40)
> Please file a followup bug on the multipart converter?

Filed bug 339610.

(In reply to comment #40)
> >imgRequestProxy::~imgRequestProxy()
> >+         Passing false to aNotify means that mListener will still get
> >+         OnStopRequest, if needed.
> 
> And we'll pass |this| to it, right?  In that case, we should document that the
> request passed to onStopRequest must not be accessed outside of OnStopRequest,
> since it might get destroyed immediately after the function returns, even if
> the callee takes a ref to it.  :(  In general, this does make me a little
> queasy, though.  We should file a followup bug on trunk, perhaps, to make the
> ownership model happier on trunk?

There is an:
NS_PRECONDITION(!mListener, "Someone forgot to properly cancel this request!");
at the beginning of the destructor which hopefully means this doesn't actually happen.  I think perhaps the best thing to do is (within this patch) just null out mListener at the beginning of the destructor in case it's not already null so that we don't potentially have this problem.

(In reply to comment #49)
> > > Perhaps there should be some imgBaseDecoderObserver that stubs these
> > > methods out so that their code may be shared.
> > 
> > A separate patch, perhaps?
> 
> Sure.

Filed bug 339612.

    
    

    
  
(In reply to comment #40)
> So here I think we want a kungFuDeathGrip to ourselves.  Otherwise, consider a
> caller whose OnStopRequest drops the ref to the imgRequestProxy and which calls
> Cancel().  By the time we go to set mListener, we're deleted.  Alternately, we
> could make this the caller's responsibility, given the possibly sync semantics
> of this stuff, but I think we want to change to fully async semantics in
> general and it's better to just have the ugliness in one place, imo.

I think darin prefers the idea that it's the caller's responsibility to do this if it has to.  I don't think I'm introducing any such callers.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
This addresses comments from darin and bzbarsky but is not merged to trunk.

        Attachment #221136 -
        Attachment is obsolete: true

        Attachment #223861 -
        Flags: review?(pavlov)

        Attachment #221136 -
        Flags: review?(pavlov)

    
    

    
  
(In reply to comment #45)
> (In reply to comment #44)
> > So this fixes the addEventListener case but not the
> > (never worked anyway) .onload= case, right?
> 
> Do you have a testcase for that case?

Oh, my first testcase?  It *used* to fix that; not sure why it doesn't anymore, but I saw
WARNING: unbalanced handler preservation refcount: 'mRootRefCount == 0', file /builds/trunk/mozilla/content/base/src/nsImageLoadingContent.cpp, line 139
fly by.  I'll look into it.  I probably broke something at some point.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  
Merged to trunk.

    
    

    
  
> Do you have a testcase for that case?

Doesn't "testcase showing the bug (always broken in Mozilla)" show it?  Or does this patch fix that one?

    
    

    
  
Sorry, I forgot that I'd used that in that testcase.

In any case, what regressed fixing the first testcase was the exactness changes in bug 206520 comment 12 -- since that code isn't detecting that case as an event handler.  I'll see how hard it is to fix -- I don't think it should be too bad.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  
So this fixes the regression by adding the changes to nsDOMClassInfo at the start of the patch.

        Attachment #223861 -
        Attachment is obsolete: true

        Attachment #223867 -
        Attachment is obsolete: true

        Attachment #223985 -
        Flags: review?(pavlov)

        Attachment #223861 -
        Flags: review?(pavlov)

        Attachment #223985 -
        Flags: superreview?(bzbarsky)

    
    

    
  
Comment on attachment 223985 [details] [diff] [review]
patch

I assume the classinfo changes are the only change from the previous diff?

If so, make the comment in nsEventReceiverSH::NewResolve say something more like "make sure that this node can't go away while waiting for a network load" and sr=bzbarsky

        Attachment #223985 -
        Flags: superreview?(bzbarsky) → superreview+

    
    

    
  
Checked in to trunk.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED

    
    

    
  
Fixed on MOZILLA_1_8_BRANCH by checkin of bug 336791.
Keywords: fixed1.8.1

    
  

        Attachment #223985 -
        Flags: review?(pavlov)
Flags: blocking1.9a1?

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: