Bug 7805 – LEAK: method name leaks in KJS::Bindings::CInstance::invokeMethod

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

A leak observed on BuildBot:

Leak: 0x106d6c90  size=16	string 'getURL'
	Call stack: [thread 24a7]: | 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:] | -[WebDataSource(WebPrivate) _commitLoadWithData:] | -[WebHTMLRepresentation receivedData:withDataSource:] | -[WebFrameBridge receivedData:textEncodingName:] | -[WebCoreFrameBridge addData:] | WebCore::Frame::addData(char const*, int) | WebCore::Frame::write(char const*, int) | WebCore::HTMLTokenizer::write(WebCore::SegmentedString const&, bool) | WebCore::HTMLTokenizer::parseTag(WebCore::SegmentedString&, WebCore::HTMLTokenizer::State) | WebCore::HTMLTokenizer::parseSpecial(WebCore::SegmentedString&, WebCore::HTMLTokenizer::State) | WebCore::HTMLTokenizer::scriptHandler(WebCore::HTMLTokenizer::State) | WebCore::HTMLTokenizer::scriptExecution(QString const&, WebCore::HTMLTokenizer::State, QString, int) | WebCore::Frame::executeScript(QString const&, int, WebCore::NodeImpl*, QString const&) | WebCore::KJSProxyImpl::evaluate(WebCore::String const&, int, WebCore::String const&, WebCore::NodeImpl*) | KJS::Interpreter::evaluate(KJS::UString const&, int, KJS::UChar const*, int, KJS::JSValue*) | KJS::InterpreterImp::evaluate(KJS::UChar const*, int, KJS::JSValue*, KJS::UString const&, int) | KJS::BlockNode::execute(KJS::ExecState*) | KJS::SourceElementsNode::execute(KJS::ExecState*) | KJS::TryNode::execute(KJS::ExecState*) | KJS::BlockNode::execute(KJS::ExecState*) | KJS::SourceElementsNode::execute(KJS::ExecState*) | KJS::IfNode::execute(KJS::ExecState*) | KJS::BlockNode::execute(KJS::ExecState*) | KJS::SourceElementsNode::execute(KJS::ExecState*) | KJS::IfNode::execute(KJS::ExecState*) | KJS::BlockNode::execute(KJS::ExecState*) | KJS::SourceElementsNode::execute(KJS::ExecState*) | KJS::ExprStatementNode::execute(KJS::ExecState*) | KJS::FunctionCallDotNode::evaluate(KJS::ExecState*) | KJS::JSObject::call(KJS::ExecState*, KJS::JSObject*, KJS::List const&) | KJS::RuntimeMethod::callAsFunction(KJS::ExecState*, KJS::JSObject*, KJS::List const&) | KJS::Bindings::CInstance::invokeMethod(KJS::ExecState*, KJS::Bindings::MethodList const&, KJS::List const&) | KJS::Bindings::CMethod::name() const | _NPN_UTF8FromIdentifier | strdup

[Alexey Proskuryakov]

Created attachment 7114 [details]
proposed fix

[Geoffrey Garen]

Comment on attachment 7114 [details]
proposed fix

Nice fix.

I really wish the method/field weren't responsible for deleting this data. What if a caller wanted to hang on to it past the method's/field's lifetime?

Still, in practice that doesn't seem to happen, and this is the idiom used in the rest of the bindings code, so I think it's the right fix.

[Darin Adler]

I also noticed this leak. But when I went to fix this leak I went crazy and also fixed a lot of other things. We should land Alexey's patch.

[Darin Adler]

Created attachment 7117 [details]
a patch that includes my fix for this leak

[Darin Adler]

Actually, although my patch has lots of other fixes, the fix for the leak is better. There no need to allocate and then later destroy a string. We can just use the pointer to the string that exists inside the private identifier.

Here's an excerpt from mine:

+const char* CMethod::name() const
+{
+    PrivateIdentifier *i = (PrivateIdentifier *)_methodIdentifier;
+    return i->isString ? i->value.string : 0;
+}

[Alexey Proskuryakov]

Comment on attachment 7114 [details]
proposed fix

Most importantly, my patch is simply wrong - it still leaks the name. Didn't clean up properly when moving from char* to CString...

[John Sullivan]

Comment on attachment 7117 [details]
a patch that includes my fix for this leak

I almost reviewed this, but got scared away by (1) the many style changes hiding the few (one? I gave up trying to find them) substantive changes; (2) the lack of ChangeLog explaining what substantive changes I should be looking for.

[Maciej Stachowiak]

Comment on attachment 7117 [details]
a patch that includes my fix for this leak

Per sullivan's comment, please add a ChangeLog entry explaining the substantial change - I couldn't find it either.

[Darin Adler]

The reason the patch doesn't have a change log is that I wasn't really ready to submit it for review. I attached it to demonstrate the leak fix within and then Geoff set the review flag. I decided to set the review flag myself to make it clear I had made the code change, but I haven't done my usual steps to prepare to land a code change yet.

[Geoffrey Garen]

Sorry. My bad.