LEAK: in ThreadableWebSocketChannel::create()

[Shinichiro Hamaji]

Call stack: [thread 0xb0227000]: | thread_start | _pthread_start | __ZN3WTFL16threadEntryPointEPv | WebCore::WorkerThread::workerThreadStart(void*) | WebCore::WorkerThread::workerThread() | WebCore::WorkerScriptController::evaluate(WebCore::ScriptSourceCode const&) | WebCore::WorkerScriptController::evaluate(We
bCore::ScriptSourceCode const&, WebCore::ScriptValue*) | JSC::evaluate(JSC::ExecState*, JSC::ScopeChain&, JSC::SourceCode const&, JSC::JSValue) | JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::ScopeChainNode*, JSC:
:JSObject*, JSC::JSValue*) | JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*, JSC::JSValue*) | jscGeneratedNativeCode | cti_op_construct_NotJSConstruct | __ZN7WebCoreL18constructWebSocketEPN3JSC9ExecStateEPNS0_
8JSObjectERKNS0_7ArgListE | WebCore::WebSocket::connect(WebCore::KURL const&, int&) | WebCore::WebSocket::connect(WebCore::KURL const&, WebCore::String const&, int&) | WebCore::ThreadableWebSocketChannel::create(WebCore::ScriptExecutionContext*, WebCore::WebSocketChannelClient*, WebCore::KURL const&, WebCore::String const&) | WebCore::String::append(WebCore::String const&) | WebCore::StringImpl::c
reateUninitialized(unsigned int, unsigned short*&) | WTF::fastMalloc(unsigned long) | malloc | malloc_zone_malloc 

Looks like WorkerThreadableWebSocketChannel::Bridge isn't freed.

[Shinichiro Hamaji]

Created attachment 53343 [details]
Patch v1

[Fumitoshi Ukai]

(In reply to comment #1)
> Created an attachment (id=53343) [details]
> Patch v1

LGTM.
Thanks!

[David Levin]

Comment on attachment 53343 [details]
Patch v1


> diff --git a/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp b/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp
> index 3dda104..d99efaa 100644
> --- a/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp
> +++ b/WebCore/websockets/WorkerThreadableWebSocketChannel.cpp
> @@ -52,7 +52,7 @@ namespace WebCore {
>  WorkerThreadableWebSocketChannel::WorkerThreadableWebSocketChannel(WorkerContext* context, WebSocketChannelClient* client, const String& taskMode, const KURL& url, const String& protocol)
>      : m_workerContext(context)
>      , m_workerClientWrapper(ThreadableWebSocketChannelClientWrapper::create(client))
> -    , m_bridge(new Bridge(m_workerClientWrapper, m_workerContext, taskMode, url, protocol))
> +    , m_bridge(adoptRef(new Bridge(m_workerClientWrapper, m_workerContext, taskMode, url, protocol)))

The standard way to do this is to make the constructor private and make a static create method which does the adoptRef and returns a PassRefPtr.

[Shinichiro Hamaji]

Created attachment 53348 [details]
Patch v2

[Fumitoshi Ukai]

(In reply to comment #4)
> Created an attachment (id=53348) [details]
> Patch v2

Looks good!
I believe no other class in websockets does the same mistake.

[David Levin]

Comment on attachment 53348 [details]
Patch v2

Your Changelog doesn't reflect your change now. (It seems to be missing the recent changes.) so please do a minor update on that and submit this.

Thanks!

[Shinichiro Hamaji]

Committed r57646: <http://trac.webkit.org/changeset/57646>

[Shinichiro Hamaji]

Oops. I fixed the ChangeLog entry before I landed. Thanks for your review!