Bug 4796 – leaks of NodeImpl called from HTMLParser::textCreateErrorCheck, seen running webkit tests

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Sullivan]

This bug is also in Radar as <rdar://4232812>

This is split off from 4665. This is one of the many leaks found with these steps:

1. Build a development build of tip-of-tree WebKit
2. use run-webkit-tests --leaks

Leak: 0x1d5ac780  size=48	
	0x013f85b0 0x00000000 0x00000000 0x1d5d1990 	.?...........]..
	0x00000000 0x00000000 0x00000000 0x00000000 	................
	0x00000000 0x00000000 0x1f30fc90 0x00000000 	.........0......
	Call stack: [thread 239f]: | 0x0 | start | _start | main | dumpRenderTree | -[NSRunLoop 
runMode:beforeDate:] | CFRunLoopRunSpecific | __CFRunLoopRun | __CFRunLoopDoSources0 | 
_sendCallbacks | -[NSURLConnection(NSURLConnectionInternal) _sendCallbacks] | -[NSURLConnection
(NSURLConnectionInternal) _sendDidReceiveDataCallback] | -[WebLoader 
connection:didReceiveData:lengthReceived:] | -[WebMainResourceLoader 
didReceiveData:lengthReceived:] | -[WebLoader didReceiveData:lengthReceived:] | -
[WebMainResourceLoader addData:] | -[WebDataSource(WebPrivate) _receivedData:] | -
[WebHTMLRepresentation receivedData:withDataSource:] | -[WebBridge 
receivedData:textEncodingName:] | -[WebCoreBridge addData:] | KWQKHTMLPart::addData(char const*, 
int) | KHTMLPart::write(char const*, int) | khtml::HTMLTokenizer::write(khtml::TokenizerString const&, 
bool) | khtml::HTMLTokenizer::processToken() | HTMLParser::parseToken(khtml::Token*) | 
HTMLParser::getNode(khtml::Token*) | HTMLParser::textCreateErrorCheck(khtml::Token*, 
DOM::NodeImpl*&) | DOM::NodeImpl::operator new(unsigned long) | khtml::main_thread_malloc
(unsigned long) | malloc 

This leak occurred 4 times in the attachment in 4665

I still see this one today (most of the other leaks from 4665 have now been fixed).

[Kurt Kohler]

(In reply to comment #0)
> This bug is also in Radar as <rdar://4232812>
> 
> This is split off from 4665. This is one of the many leaks found with these steps:
> 
> 1. Build a development build of tip-of-tree WebKit
> 2. use run-webkit-tests --leaks
> 
> Leak: 0x1d5ac780  size=48	
> 	0x013f85b0 0x00000000 0x00000000 0x1d5d1990 	.?...........]..
> 	0x00000000 0x00000000 0x00000000 0x00000000 	................
> 	0x00000000 0x00000000 0x1f30fc90 0x00000000 	.........0......
> 	Call stack: [thread 239f]: | 0x0 | start | _start | main | dumpRenderTree |
-[NSRunLoop 
> runMode:beforeDate:] | CFRunLoopRunSpecific | __CFRunLoopRun |
__CFRunLoopDoSources0 | 
> _sendCallbacks | -[NSURLConnection(NSURLConnectionInternal) _sendCallbacks] |
-[NSURLConnection
> (NSURLConnectionInternal) _sendDidReceiveDataCallback] | -[WebLoader 
> connection:didReceiveData:lengthReceived:] | -[WebMainResourceLoader 
> didReceiveData:lengthReceived:] | -[WebLoader didReceiveData:lengthReceived:] | -
> [WebMainResourceLoader addData:] | -[WebDataSource(WebPrivate) _receivedData:] | -
> [WebHTMLRepresentation receivedData:withDataSource:] | -[WebBridge 
> receivedData:textEncodingName:] | -[WebCoreBridge addData:] |
KWQKHTMLPart::addData(char const*, 
> int) | KHTMLPart::write(char const*, int) |
khtml::HTMLTokenizer::write(khtml::TokenizerString const&, 
> bool) | khtml::HTMLTokenizer::processToken() |
HTMLParser::parseToken(khtml::Token*) | 
> HTMLParser::getNode(khtml::Token*) |
HTMLParser::textCreateErrorCheck(khtml::Token*, 
> DOM::NodeImpl*&) | DOM::NodeImpl::operator new(unsigned long) |
khtml::main_thread_malloc
> (unsigned long) | malloc 
> 
> This leak occurred 4 times in the attachment in 4665
> 
> I still see this one today (most of the other leaks from 4665 have now been
fixed).


  I've been looking at that same one and I may see the problem. In
HTMLParser::getNode() "result" is potentially set twice. When it's set the
second time, I don't see any guarantee that you won't be overwriting a non-null
pointer. I think there should be a "delete result" just before the second time
"result" is set. I can submit a patch, but it seems pretty trivial (assuming I'm
right!)

By the way, wouldn't it be more straightforward to have getNode return a
SmartPtr rather than converting the raw pointer after the (single) call to getNode?

[Kurt Kohler]

Unfortunately the change I suggested doesn't fix the leak. Sorry.

I'd still suggest an "assert(!result)" just before the assignment to result however.

[Darin Adler]

This function is where all text nodes are created while parsing.

I'm pretty sure that this is the same leak I fixed in my fix for bug 4797.

[Darin Adler]

Checked in a fix that should fix the leak described in this bug, as well as two others. The three bugs are 
bug 4795, bug 4796, and bug 4797.