188729 - Ownership cycles during load are dangerous

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Right now we have two ownership cycles during load

1. sink->document->parser->sink
2. sink->document->scriptloader->sink

1 is an easy fix since the document doesn't really need the parser, it only uses
it to get the loadcommand so that it knows if it should fire a loadevent or not
(why this is I don't know, but there we are).

This cycle I fixed by simply adding a mLoadedAsData flag.

I had to do a tad of cleaning up in nsXMLDocument::StartDocumentLoad since the
current code relied on that rv was set to success at the point where we created
the parser, otherwise an errorvalue from further up ended up being returned.
(This is a very good example of why you shouldn't end your functions with
|return rv;|)


2 is a bit trickier since the scriptloader always holds owning references, and
the sink does need listen to the scriptloaders notifications. I fixed this the
same way peterv fixed a similar problem in the syncloader; I created a
proxy-object that holds a weak reference and forwards all calls. So the same
chain cycle still exists, but the scriptloader->sink reference is weak.

I had to make sure that the NS_NewX(M|B)LContentSink functions kept an owning
reference to the sink while calling the Init function since creating the proxy
addrefed/released the sink. (And you shouldn't use an object with refcount=0 anyway)

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch to fix

There was one more thing i had to do. Just breaking the circle killed XSLT
since the XSLT-code only kept a weak reference to the sink, so the only thing
that kept the sink alive was the ownership circle.

I changed this so that the XSLT code keeps a owning reference to the sink. To
avoid a circular ownership between tranform-mediator<->sink i let the sink drop
it's reference to the mediator once the mediator is fully set up.

patch is diffed with -u10 to make bz happy :-)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111320 [details] [diff] [review]
patch to fix

> +  nsScriptLoaderObserverProxy* proxy = new nsScriptLoaderObserverProxy(this);
> +  NS_ENSURE_TRUE(proxy, NS_ERROR_OUT_OF_MEMORY);

So.. hold an owning ref to this till after you call AddObserver()?  'twould be
poor for the callee to addref and release the pointer (eg by doing a QI into an
nsCOMPtr and having that go out of scope) on the reasonable assumption that the
caller is holding a ref....(see comment 0 anyway ;) ).

Also, should that not be NS_STATIC_CAST(nsIScriptLoaderObserver*, this)?  Since
that's not the first class nsXMLContentSink inherits from?

+  nsCOMPtr<nsIParser> parser = do_CreateInstance(kCParserCID);
+  NS_ENSURE_TRUE(parser, NS_ERROR_OUT_OF_MEMORY);

Let's not make up error values, ok?  Get the rv from do_CreateInstance (fwiw,
I've had do_CreateInstance fail with NS_ERROR_NO_INTERFACE when someone used
the wrong contractid; having the right retval helps a lot in debugging).

   if(aContainer)
   {
     docShell = do_QueryInterface(aContainer, &rv);

Could you declare webShell outside this if, set it in the if, and move the code
that creates the sinks out of the if?  That will give you identical behavior
with less code duplication....

+  // Set the parser as the stream listener for the document loader...
+  CallQueryInterface(parser, aDocListener);

Why do you no longer check the rv here?

sr=bzbarsky with those changes addressed.

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 111320 [details] [diff] [review]
patch to fix

With bz's comments addressed.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

checked in.

Fixed all comments except
> Also, should that not be NS_STATIC_CAST(nsIScriptLoaderObserver*, this)?  Since
> that's not the first class nsXMLContentSink inherits from?

This is not needed since there will be an implicit cast (which is always safe)

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

checked in for real now. Leaving open since i realized that i might need to
break the sink->document->scriptloader->sink cycle for the HTML-sink as well

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ah, good point.  As long as you're not casting to void* and back the implicit
cast should be ok.

And you are not holding that owning ref as you call addObserver....

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

the checked in code does:

nsCOMPtr<nsIScriptLoaderObserver> proxy =
    new nsScriptLoaderObserverProxy(this);

so it should be holding an owning reference.

Comment 8

[Boris Zbarsky [:bzbarsky]]

ugh.  I thought you'd attached a patch with my comments addressed, but that was
just peterv commenting... I need a break.  ;)

Comment 9

[Boris Zbarsky [:bzbarsky]]

+  
+  nsCOMPtr<nsIXMLContentSink> kungFuDeathGrip = it;
   nsresult rv = it->Init(aDoc, aURL, aWebShell, aChannel);
   if (NS_OK != rv) {
     delete it;
     return rv;
   }

This causes a crash if Init() fails (double-delete).  Can't believe I missed that...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: fix crash

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 114577 [details] [diff] [review]
fix crash

This comes up in SVG-enabled builds a lot, since Init() fails for SVG
documents...

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 114577 [details] [diff] [review]
fix crash

ugh. r=me

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 114577 [details] [diff] [review]
fix crash

Sorry for missing that.

Comment 14

[Boris Zbarsky [:bzbarsky]]

mozilla% cvs commit content/xml/document/src/nsXMLContentSink.cpp
Checking in content/xml/document/src/nsXMLContentSink.cpp;
/cvsroot/mozilla/content/xml/document/src/nsXMLContentSink.cpp,v  <-- 
nsXMLContentSink.cpp
new revision: 1.251; previous revision: 1.250
done

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

upps, this was fixed a long time ago

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

oh, wait, i left this open to fix the html-sink as well. Reopening

Comment 17

[Harshal Pradhan]

The html part of this bug was fixed by bug 218837.