36148 - document.open leaks a nsDocShell (1 ref, 2 if no doc.close())

Status: VERIFIED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

DESCRIPTION:  The page http://people.netscape.com/morse/jcal/index.html (lots of
frames) leaks 12 docshells when loaded in viewer.

STEPS TO REPRODUCE:
 * setenv XPCOM_MEM_LEAK_LOG 1 (in a debug build)
 * ./mozilla-viewer http://people.netscape.com/morse/jcal/index.html

ACTUAL RESULTS:
 * almost 200K of leaks, including 12 docshells

EXPECTED RESULTS:
 * much fewer leaks

DOES NOT WORK CORRECTLY ON:
 * Linux, mozilla, debug build from 2000-04-12 17:00 PDT.

ADDITIONAL INFORMATION:
Hopefully I'll have a chance to do some refcount logging on this and figure out
what's going on, but I want to get it into Bugzilla so it's not forgotten.

Comment 1

[Eric Pollmann]

Dup of 35573?  Only the reflog will tell...

Comment 2

[Eric Pollmann]

Ref logging round one: nsDocShell

I appears that whenever we call document.open, we are leaking a content sink,
and consequently, a webshell (HTMLContentSink holds a ref on it's webshell).  No
idea why we leak the content sink yet:

                  |         |     | 1      
nsLoadGroup::RemoveChannel(nsIChannel *, nsISupports *, unsigned int, unsigned
short const *)+0x00000326
                  |         |     |   1      
nsDocLoaderImpl::OnStopRequest(nsIChannel *, nsISupports *, unsigned int,
unsigned short const *)+0x00000078
                  |         |     |     1      
nsDocLoaderImpl::DocLoaderIsEmpty(unsigned int)+0x0000002B
                  |         |     |       1	  	 
nsDocLoaderImpl::DocLoaderIsEmpty(unsigned int)+0x000000FB
                  |         |     |         1	     
nsDocLoaderImpl::FireOnEndDocumentLoad(nsDocLoaderImpl *, nsIChannel *, unsigned
int)+0x00000451
                  |         |     |           1      
nsWebShell::OnEndDocumentLoad(nsIDocumentLoader *, nsIChannel *, unsigned
int)+0x000002AB
                  |         |     |             1      
GlobalWindowImpl::HandleDOMEvent(nsIPresContext *, nsEvent *, nsIDOMEvent **,
unsigned int, nsEventStatus *)+0x00000117
                  |         |     |               1      
nsEventListenerManager::HandleEvent(nsIPresContext *, nsEvent *, nsIDOMEvent **,
unsigned int, nsEventStatus *)+0x00001692
                  |         |     |                 1      
nsEventListenerManager::HandleEventSubType(nsListenerStruct *, nsIDOMEvent *,
unsigned int, unsigned int)+0x0000079D
                  |         |     |                   1      
nsJSEventListener::HandleEvent(nsIDOMEvent *)+0x00000324
                  |         |     |                     1      
nsJSContext::CallEventHandler(void *, void *, unsigned int, void *, int *,
int)+0x00000213
	              |         |     |                       1      
JS_CallFunctionValue+0x0000002F
	              |         |     |                         1      
js_InternalInvoke+0x000000EC
	              |         |     |                           1      
js_Invoke+0x0000089D
	              |         |     |                             1      
js_Interpret+0x0001032F
                  |         |     |                               1      
js_Invoke+0x0000083E
   	         	  |         |     |                                 1      
NS_NewScriptHTMLDivElement+0x00002798
   	              |         |     |                                   1      
nsHTMLDocument::Open(JSContext *, long *, unsigned int)+0x0000006F
	    	      |         |     |                                     1      
nsHTMLDocument::OpenCommon(nsIURI *)+0x000002A7
	    	    |         |     |                                     | 1      
nsPresContext::GetContainer(nsISupports **)+0x00000067
                  |         |     |                                     |  
1       unsigned int ns_if_addref<nsISupports *>(nsISupports *)+0x00000023
   	         	  |         |     |                                     |    
1       nsWebShell::AddRef(void)+0x0000001A
   	              |         |     |                                     |      
1       nsDocShell::AddRef(void)+0x00000069
	    	      |         |     |                                     1      
nsHTMLDocument::OpenCommon(nsIURI *)+0x000002F2
	    	    |         |     |                                     | 1      
nsCOMPtr<nsIWebShell>::operator=(nsQueryInterface const &)+0x00000027
	     	  |         |     |                                     |   1      
nsQueryInterface::operator()(nsID const &, void **) const+0xC7EC43A0
	       	  |         |     |                                     |     1      
nsQueryInterface::operator()(nsID const &, void **) const+0x00000036
             	  |         |     |                                     |      
1       nsWebShell::QueryInterface(nsID const &, void **)+0x00000215
	    	    |         |     |                                     |        
1       nsWebShell::AddRef(void)+0x0000001A
	    	  |	    |     |                                     |           1      
nsDocShell::AddRef(void)+0x00000069
                  |	    |     |                                     -1     
nsHTMLDocument::OpenCommon(nsIURI *)+0x00000300
                  |         |     |                                     |
-1      nsWebShell::Release(void)+0xC74D9560   
                  |         |     |                                     |  
-1      nsWebShell::Release(void)+0x0000001A
                  |         |     |                                     |    
-1      nsDocShell::Release(void)+0x00000065
                  |         |     |                                     1      
nsHTMLDocument::OpenCommon(nsIURI *)+0x00000349
   	     	  |         |     |                                     | 1      
NS_NewHTMLContentSink(nsIHTMLContentSink **, nsIDocument *, nsIURI *,
nsIWebShell *)+0x00000099
                  |         |     |                                     |  
1       HTMLContentSink::Init(nsIDocument *, nsIURI *, nsIWebShell *)+0x00000255
                  |         |     |                                     |    
1       nsWebShell::AddRef(void)+0x0000001A
    	            |         |     |                                     |      
1       nsDocShell::AddRef(void)+0x00000069
   	          |         |     |                                     -1     
nsHTMLDocument::OpenCommon(nsIURI *)+0x000003DA
                  |         |     |                                      
-1      nsWebShell::Release(void)+0xC74D3F40
	        	    |         |     |                                         -1     
nsWebShell::Release(void)+0x0000001A
	  	    |         |     |                                           -1     
nsDocShell::Release(void)+0x00000065

Comment 3

[Eric Pollmann]

Hm, pasting a tree in the comments doesn't work very well.  :S

Comment 4

[Eric Pollmann]

Ref logging round two: HTMLContentSink

It appears we are leaking two references to a content sink into the parser every
time document.write is called.  Now to see why we leak parsers.  I have to learn
how to use boehm...

Comment 5

[Eric Pollmann]

Hm, there seems to be some kind of incestuous, perhaps circular relationship
between parsers and content sinks that I don't really understand...  I'm
attaching two ref log trees that I tried to pare down to the bare minimum
relavent detail.  The problem seems to be present only when script is used to
open, write, and close documents.

Anyone have any ideas here?

Comment 6

[Eric Pollmann]

Attachment: leaking sink ref log tree

Comment 7

[Eric Pollmann]

Attachment: leaking parser ref log tree

Comment 8

[Eric Pollmann]

Attachment: simplified one-liner test case.

Comment 9

[Eric Pollmann]

*** Bug 35573 has been marked as a duplicate of this bug. ***

Comment 10

[Eric Pollmann]

Giving this to Harish to look at, thanks Harish!

Comment 11

[harishd]

Moving to M17..

Comment 12

[harishd]

Nominating for nsbeta2.

Comment 13

[Johnny Stenback (:jst)]

I think at least part of the problem here is that we we leak a CNavDTD in
nsHTMLDocument::OpenCommon(), we create it and pass it to the parser but we
never release it. From looking at the source it looks like we might be leaking
a parser (which holds on to the sink, which in turn holds on to the document...)
too if document.close() is never called, but I haven't done any testing that
supports my findings...

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Loading attachment 8681 [details], there is one nsParser leaked, and DidBuildModel is
never called on that nsParser.  I think calling DidBuildModel releases the
circular references between parser and content sink.

Comment 15

[Johnny Stenback (:jst)]

David, I think you're right, but unfortunately there's no obvious place where
we would know that the parsers DidBuildModel() should be called. Here's what
I think should happen when everything goes as planned:

document.open();

  create the parser (nsHTMLDocument::mParser) and the sink.

document.write();

  tell the parser to parse the written string, but aLastCall = PR_FALSE, since
  there might be more document.write() calls.

document.write();

  same as above...

document.close();

  tell the parser to parse an empty string, but pass aLastCall = PR_TRUE, now
  the parser know that this is the end of the document, the parser calls
  DidBuildModel().

So if document.close() is never called DidBuildModel() is never called, and the
result is that we leak I don't know how much...

I don't know off hand what method(s) on the document is called prior to it being
replaced by some other document, or simply being deleted, but if there's such a
method (Reset(), maybe?) we could check there if we're in the middle of
document.write():ing (mIsWriting == PR_TRUE) and we have a parser we should
tell the parser to parse an empty string and pass aLastCall = PR_TRUE, and then
release the parser.

I haven't tested any of this, so I might be a bit off...

Comment 16

[Eric Pollmann]

I think Harish has a fix for this leak already (including when document.close is 
not called)

Comment 17

[harishd]

Attachment: Leaks statistics - Before my change

Comment 18

[harishd]

Attachment: Leak statistics - After my change

Comment 19

[harishd]

BTW, the above leaks were measured while loading Pollmann's reduced case ( refer 
to comment 2000-05-15 03:56  ).

Let's hope the leak statistics would grab PDT's attention.

Comment 20

[harishd]

Attachment: Patch to kill!

Comment 21

[harishd]

Attachment: A minor change ( that should go in accordance with the above patch )

Comment 22

[harishd]

We shouldn't be leaking anymore. 

Note: Without a call to document.close() we probably are still leaking.. I'll 
look into this soon. 

Marking this bug FIXED.

Comment 23

[Chris Petersen]

Marking verified per last comments.