40149 - [FIX]nsParser::SetParserFilter() needs a "XPCOM cleanup".

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Johnny Stenback (:jst)]

nsParser::SetParserFilter() is currently defined to return a pointer to the old
parser filter, from reading the implementation it looks like it's meant to
return a weak reference to the old parser filter, ie no reference held by either
the parser nor the caller the gets the old filter. The way it's implemented the
parser releses the old filter with NS_RELEASE(old) then it does a return old;.
The result of this is that it always returns null, so nothing bad will ever
happen as a result of calling nsParser::SetParserFilter() but if a unrefcounted
pointer to the old parser filter would actually be returned the pointer could be
pointing to a deleted object since the parser doesn't know if calling release on
the old filter actually deletes the old filter or just decrements its
refcount...

wrong (TM).

I suggest nsIParser::SetParserFilter() should be changed to return the old
filter in an out parameter, and not as the return value. Then there's no
question about if the returned filter should be released or not. Clean, simple
XPCOM.

Comment 1

[rusty.lynch]

There is a related webshell leak to the current nsParser implementation.
The problem is that nsParser AddRef's the parser filter, mParserFilter,
on nsParser::SetParserFilter(), but doesn't release in the constructor.

Currently this results in a parser filter leak, which translate to a webshell
leak from nsHTMLDocument.cpp when auto-detection of character sets is turned 
on.

The fix to just to add a NS_IF_RELEASE(mParserFilter); to nsParser::~nsParser().

NOTE: nsHTMLDocument.cpp has some other leaks that will result in noise when
trying to test this if the patch at bug #39796 hasn't been checked into the tree
yet.

Comment 2

[harishd]

Moving to M17.

Comment 3

[rusty.lynch]

Can we go ahead and checkin the quick webshell leak fix talked about a 
couple of comments up?  It's not much of a leak, but it takes out a considerable
amount of noise it the bloat logs (which helps reduce my fustration level while
searching for root leaks.)

Comment 4

[harishd]

Ya sure.

Comment 5

[rusty.lynch]

So can I consider that a r=harishd? I'll bubble it up the approval stack
and check it in if it is.

Comment 6

[Simon Fraser [no longer active]]

I found this too. See bug 41695.

Comment 7

[harishd]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this
is an error, that you or another known resource will be working on this bug,or 
if it blocks your work in some way -- please
attach your concern to the bug for reconsideration. 

Comment 8

[Hoa Nguyen]

Wo!  I think something got lost here.  Rusty already filed a fix (note Rusty's 
first comment) and seems like [e-mail] was affirmative on the 
review (second comment by harishd: "Ya sure").  We understand Netscape's 
internal resource constraints, but we've already got the fix in hand.  We can 
go ahead and check this in following the normal review and approval process.  
Or was Rusty's fix not satisfactory and are we waiting on a better fix?

Comment 9

[Simon Fraser [no longer active]]

I checked in the fix to release the mParserFilter, in nsParser.

Comment 10

[Hoa Nguyen]

Not going to complain too much, but in the future, could you guys turn around 
the review a bit quicker.  Just a simple yay or nay would be great.  I know 
that this is all part of working on open source, but it's frustrating for 
developers to work on things that don't materialize.  Also there were 
dependecies on this bug (40321).  Would have been nice to get quicker 
notification that a fix was checked in so that we could proceed with any 
dependent bugs.  Don't mean to whine too much, but . . . 

Comment 11

[harishd]

Oops...Sorry folks.

Comment 12

[harishd]

I was under the impression that Rusty was going to checkin the fix.  I even 
pointed out this bug to Simon ( thanx for checking the fix in ) when he asked me 
review an identical snippet.  Anyway, I'll take the blame...cause it's my 
bug...and I should have taken care of it. Am I bad? :-(

Comment 13

[rusty.lynch]

Bad harishd! Bad harishd! :-O

Yea, with all of this nsbeta/catfood++/... stuff going on I'm probobly
being to careful trying to follow the correct protocol to checkin code.

Comment 14

[harishd]

No offense please :-) I's pointing finger standing before a mirror.

Comment 15

[harishd]

Isn't this bug fixed rusty?

Comment 16

[rusty.lynch]

There were really two bugs refered to in the report.  The initial bug talked 
about how doing some XPCOM clean in the implementation.  This still needs to be 
done, it's just not all that critical.

The second thing that came out of this was a parser leak that resulted from the 
same code.  That has been fixed.  (Looking back it might have made more sense 
to create a new bug for the parser leak, I was just thinking if all the code 
was overhauled using smart pointers the the like that the parser leak would be 
fixed in the process.)

Comment 17

[harishd]

Shooting for beta3.

Comment 18

[harishd]

Rusty, since I wouldn't (~) be able to get to this bug for beta3, would it be 
possible for you to own this bug? Thanx.

Reassigning to rusty ( per discussion with nisheeth and eKrock ). 

Btw, feel free to give this bug back to me if you're swamped with other things.

Comment 19

[Nisheeth Ranjan]

Marking nsbeta3-.  Rusty, if you have the bandwidth to  
do the XPCOM cleanup in the beta 3 timeframe you can get your patch code 
reviewed by brendan or waterson and check it in.  Thanks.

Comment 20

[bsharma]

updated qa contact.

Comment 21

[Henrik Lynggaard Hansen]

This bug was last changed 2001-03-02, and it has no milestone set. Is this
problem still present ? if so please verify otherwise please close this bug.

Comment 22

[Matthias Versen [:Matti]]

-> default owner

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Since none of the callers ever look at any of the return values....

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 119788 [details] [diff] [review]
Since none of the callers ever look at any of the return values....

Thoughts?

Comment 25

[Boris Zbarsky [:bzbarsky]]

.

Comment 26

[Simon Fraser [no longer active]]

+void nsParser::SetContentSink(nsIContentSink* aSink) {
+  NS_PRECONDITION(aSink,"sink cannot be null!");

It would be nice to standardize the method brace style in this file to the
mozilla default (opening brace on a new line, like SetParserFilter() has already).

+    // XXXbz does anyone actually pass in an non-null observer?  Do we
+    // even need this anymore?
     nsParser(nsITokenObserver* anObserver=0);

Editor used to use token observers, and might again, but I doubt anything needs
to pass one to the ctor.

+    nsITokenObserver*         mTokenObserver;

Would be nice to allow for > 1 token observer. That's for another patch, I
think. The rest looks good to me.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Sure, I can move the brace.

There seem to be no ways to set the observer short of passing it to the
constructor....  Further, it is not clear what the ownership model of the
observer is (we never release it, eg, and never addref it...)  I'm pretty much
fine with anything here (up to and removing all the observer code until it's
actually needed).  But yes, the observer cleanup should probably be a separate
bug, since it requires some actual knowledge of the parser... ;)

Comment 28

[harishd]

Comment on attachment 119788 [details] [diff] [review]
Since none of the callers ever look at any of the return values....

>Index: htmlparser/public/nsIParser.h
>===================================================================
>RCS file: /cvsroot/mozilla/htmlparser/public/nsIParser.h,v

>-    virtual nsIContentSink* SetContentSink(nsIContentSink* aSink)=0;
>+    virtual void SetContentSink(nsIContentSink* aSink)=0;

Can you replace virtual void with NS_IMETHOD_(void)? Same in other places too.

>+void nsParser::SetContentSink(nsIContentSink* aSink) {
>+  NS_PRECONDITION(aSink,"sink cannot be null!");
>+  mSink=aSink;

Add spaces around '=' and after ','. :)  

>Index: htmlparser/src/nsParser.h
>===================================================================
>      */
>+    // XXXbz does anyone actually pass in an non-null observer?  Do we
>+    // even need this anymore?
>     nsParser(nsITokenObserver* anObserver=0);

It's been a while and it's all hazy for me. AFAICT, there is nobody depending
on it. IMO, nsITokenObserver can be yanked.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: Address comments

1) Added NS_IMETHOD_() stuff

2) Made Parse() have the same decls in nsParser.h and nsIParser.h (same
defaults)

3) Removed unused GetScanner() method

4) Removed mTokenObserver

5) Made BuildModel() public, since it's on the nsIParser interface (!)

6) Fixed some obviously copied incorrect comments

Trying hard to avoid making this into the "make all of rickg's code pretty"
bug.... ;)

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 119846 [details] [diff] [review]
Address comments

What thinkest thou?

Comment 31

[Johnny Stenback (:jst)]

Comment on attachment 119788 [details] [diff] [review]
Since none of the callers ever look at any of the return values....

sr=jst

Comment 32

[Johnny Stenback (:jst)]

Comment on attachment 119846 [details] [diff] [review]
Address comments

sr=jst

Comment 33

[harishd]

Comment on attachment 119846 [details] [diff] [review]
Address comments

Good boy. r=harishd :)

Comment 34

[Boris Zbarsky [:bzbarsky]]

Fixed.