Bug 37584

Summary: LEAK: in ThreadableWebSocketChannel::create()
Product: WebKit Reporter: Shinichiro Hamaji <hamaji>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ukai
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch v1
none
Patch v2 levin: review+

Description Shinichiro Hamaji 2010-04-14 10:14:55 PDT
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.
Comment 1 Shinichiro Hamaji 2010-04-14 10:16:36 PDT
Created attachment 53343 [details]
Patch v1
Comment 2 Fumitoshi Ukai 2010-04-14 10:24:06 PDT
(In reply to comment #1)
> Created an attachment (id=53343) [details]
> Patch v1

LGTM.
Thanks!
Comment 3 David Levin 2010-04-14 11:15:00 PDT
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.
Comment 4 Shinichiro Hamaji 2010-04-14 11:33:17 PDT
Created attachment 53348 [details]
Patch v2
Comment 5 Fumitoshi Ukai 2010-04-14 16:42:35 PDT
(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.
Comment 6 David Levin 2010-04-14 23:53:46 PDT
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!
Comment 7 Shinichiro Hamaji 2010-04-15 07:52:28 PDT
Committed r57646: <http://trac.webkit.org/changeset/57646>
Comment 8 Shinichiro Hamaji 2010-04-15 08:26:19 PDT
Oops. I fixed the ChangeLog entry before I landed. Thanks for your review!