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
Created attachment 7114 [details] proposed fix
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.
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.
Created attachment 7117 [details] a patch that includes my fix for this leak
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; +}
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...
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.
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.
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.
Sorry. My bad.