Bug 83345 – Leak in WebSocketChannel with workers/worker-reload.html

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Yuta Kitamura]

Original report: http://code.google.com/p/chromium/issues/detail?id=122233

Reported by project member [e-mail], Today (4 hours ago)
r130973, http://build.chromium.org/p/chromium.memory.fyi/builders/Webkit%20Linux%20%28valgrind%20layout%29/builds/17942/

Leak_DefinitelyLost
1,316 (704 direct, 612 indirect) bytes in 2 blocks are definitely lost in loss record 4,975 of 5,181
  malloc (m_replacemalloc/vg_replace_malloc.c:1072)
  WTF::fastMalloc(unsigned long) (third_party/WebKit/Source/WTF/wtf/FastMalloc.cpp:268)
  WebCore::WebSocketChannel::operator new(unsigned long) (third_party/WebKit/Source/WebCore/Modules/websockets/WebSocketChannel.h:63)
  WebCore::WebSocketChannel::create(WebCore::Document*, WebCore::WebSocketChannelClient*) (third_party/WebKit/Source/WebCore/Modules/websockets/WebSocketChannel.h:65)
  WebCore::WorkerThreadableWebSocketChannel::Peer::Peer(WTF::PassRefPtr<WebCore::ThreadableWebSocketChannelClientWrapper>, WebCore::WorkerLoaderProxy&, WebCore::ScriptExecutionContext*, WTF::String const&) (third_party/WebKit/Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:157)
  WebCore::WorkerThreadableWebSocketChannel::Peer::create(WTF::PassRefPtr<WebCore::ThreadableWebSocketChannelClientWrapper>, WebCore::WorkerLoaderProxy&, WebCore::ScriptExecutionContext*, WTF::String const&) (third_party/WebKit/Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.h:86)
  WebCore::WorkerThreadableWebSocketChannel::Bridge::mainThreadInitialize(WebCore::ScriptExecutionContext*, WebCore::WorkerLoaderProxy*, WTF::PassRefPtr<WebCore::ThreadableWebSocketChannelClientWrapper>, WTF::String const&) (third_party/WebKit/Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:402)
  WebCore::CrossThreadTask3<WebCore::WorkerLoaderProxy*, WebCore::WorkerLoaderProxy*, WTF::PassRefPtr<WebCore::ThreadableWebSocketChannelClientWrapper>, WTF::PassRefPtr<WebCore::ThreadableWebSocketChannelClientWrapper>, WTF::String, WTF::String const&>::performTask(WebCore::ScriptExecutionContext*) (third_party/WebKit/Source/WebCore/dom/CrossThreadTask.h:146)
  WebCore::Document::didReceiveTask(void*) (third_party/WebKit/Source/WebCore/dom/Document.cpp:5026)
The report came from the `#URL:http://127.0.0.1:8000/websocket/tests/hybi/workers/worker-reload.html` test.

Leak_DefinitelyLost
1,974 (120 direct, 1,854 indirect) bytes in 3 blocks are definitely lost in loss record 5,009 of 5,181
  malloc (m_replacemalloc/vg_replace_malloc.c:1072)
  WTF::fastMalloc(unsigned long) (third_party/WebKit/Source/WTF/wtf/FastMalloc.cpp:268)
  WebCore::WorkerThreadableWebSocketChannel::Peer::operator new(unsigned long) (third_party/WebKit/Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.h:82)
  WebCore::WorkerThreadableWebSocketChannel::Peer::create(WTF::PassRefPtr<WebCore::ThreadableWebSocketChannelClientWrapper>, WebCore::WorkerLoaderProxy&, WebCore::ScriptExecutionContext*, WTF::String const&) (third_party/WebKit/Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.h:86)
  WebCore::WorkerThreadableWebSocketChannel::Bridge::mainThreadInitialize(WebCore::ScriptExecutionContext*, WebCore::WorkerLoaderProxy*, WTF::PassRefPtr<WebCore::ThreadableWebSocketChannelClientWrapper>, WTF::String const&) (third_party/WebKit/Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:402)
  WebCore::CrossThreadTask3<WebCore::WorkerLoaderProxy*, WebCore::WorkerLoaderProxy*, WTF::PassRefPtr<WebCore::ThreadableWebSocketChannelClientWrapper>, WTF::PassRefPtr<WebCore::ThreadableWebSocketChannelClientWrapper>, WTF::String, WTF::String const&>::performTask(WebCore::ScriptExecutionContext*) (third_party/WebKit/Source/WebCore/dom/CrossThreadTask.h:146)
  WebCore::Document::didReceiveTask(void*) (third_party/WebKit/Source/WebCore/dom/Document.cpp:5026)
The report came from the `#URL:http://127.0.0.1:8000/websocket/tests/hybi/workers/worker-reload.html` test.

Leak_DefinitelyLost
658 (136 direct, 522 indirect) bytes in 1 blocks are definitely lost in loss record 4,752 of 5,181
  malloc (m_replacemalloc/vg_replace_malloc.c:1072)
  WTF::fastMalloc(unsigned long) (third_party/WebKit/Source/WTF/wtf/FastMalloc.cpp:268)
  WTF::ThreadSafeRefCountedBase::operator new(unsigned long) (third_party/WebKit/Source/WTF/wtf/ThreadSafeRefCounted.h:72)
  WebCore::ThreadableWebSocketChannelClientWrapper::create(WebCore::ScriptExecutionContext*, WebCore::WebSocketChannelClient*) (third_party/WebKit/Source/WebCore/Modules/websockets/ThreadableWebSocketChannelClientWrapper.cpp:58)
  WebCore::WorkerThreadableWebSocketChannel::WorkerThreadableWebSocketChannel(WebCore::WorkerContext*, WebCore::WebSocketChannelClient*, WTF::String const&) (third_party/WebKit/Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:58)
  WebCore::WorkerThreadableWebSocketChannel::create(WebCore::WorkerContext*, WebCore::WebSocketChannelClient*, WTF::String const&) (third_party/WebKit/Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.h:60)
  WebCore::ThreadableWebSocketChannel::create(WebCore::ScriptExecutionContext*, WebCore::WebSocketChannelClient*) (third_party/WebKit/Source/WebCore/Modules/websockets/ThreadableWebSocketChannel.cpp:65)
  WebCore::WebSocket::connect(WTF::String const&, WTF::Vector<WTF::String, 0ul> const&, int&) (third_party/WebKit/Source/WebCore/Modules/websockets/WebSocket.cpp:232)
  WebCore::WebSocket::connect(WTF::String const&, int&) (third_party/WebKit/Source/WebCore/Modules/websockets/WebSocket.cpp:183)
  WebCore::V8WebSocket::constructorCallback(v8::Arguments const&) (third_party/WebKit/Source/WebCore/bindings/v8/custom/V8WebSocketCustom.cpp:85)
  v8::internal::MaybeObject* v8::internal::HandleApiCallHelper<true>(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*) (v8/src/builtins.cc:1115)
  v8::internal::Builtin_HandleApiCallConstruct(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*) (v8/src/builtins.cc:1137)
  0x39F12850618D ()
  0x39F128524856 ()
  0x39F12852BF2D ()
  0x39F128524966 ()
  0x39F1285113D6 ()
  v8::internal::Invoke(bool, v8::internal::Handle<v8::internal::JSFunction>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*, bool*) (v8/src/execution.cc:118)
  v8::internal::Execution::Call(v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*, bool*, bool) (v8/src/execution.cc:173)
  v8::Script::Run() (v8/src/api.cc:1599)
  WebCore::WorkerContextExecutionProxy::runScript(v8::Handle<v8::Script>) (third_party/WebKit/Source/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp:262)
  WebCore::WorkerContextExecutionProxy::evaluate(WTF::String const&, WTF::String const&, WTF::TextPosition const&, WebCore::WorkerContextExecutionState*) (third_party/WebKit/Source/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp:213)
  WebCore::WorkerScriptController::evaluate(WebCore::ScriptSourceCode const&, WebCore::ScriptValue*) (third_party/WebKit/Source/WebCore/bindings/v8/WorkerScriptController.cpp:85)
  WebCore::WorkerScriptController::evaluate(WebCore::ScriptSourceCode const&) (third_party/WebKit/Source/WebCore/bindings/v8/WorkerScriptController.cpp:76)
  WebCore::WorkerThread::workerThread() (third_party/WebKit/Source/WebCore/workers/WorkerThread.cpp:152)
  WebCore::WorkerThread::workerThreadStart(void*) (third_party/WebKit/Source/WebCore/workers/WorkerThread.cpp:129)
  WTF::threadEntryPoint(void*) (third_party/WebKit/Source/WTF/wtf/Threading.cpp:69)
  WTF::wtfThreadEntryPoint(void*) (third_party/WebKit/Source/WTF/wtf/ThreadingPthreads.cpp:162)
The report came from the `#URL:http://127.0.0.1:8000/websocket/tests/hybi/workers/worker-reload.html` test.

[Yuta Kitamura]

And my comment:

Comment 3 by [e-mail], Today (21 minutes ago)
This is related to http://trac.webkit.org/changeset/113138

A speculative fix is on the way; I think I just need to make mainThreadDestroy() a cleanup task so that it won't get lost during main thread's cleanup period.

[Yuta Kitamura]

Created attachment 135966 [details]
Patch

[David Levin]

Comment on attachment 135966 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135966&action=review

> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:579
> +    virtual ~MainThreadDestroyTask() { }

Why is this needed?

> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:586
> +    virtual bool isCleanupTask() const OVERRIDE { return true; }

Even better, make WorkerThreadableWebSocketChannel::Peer*, OwnPtr<WorkerThreadableWebSocketChannel::Peer> instead and don't delete m_peer explicitly just let the destructor do it.

You could even get rid of this class and just do 

OwnPtr<Peer> peer = m_peer;
m_peer = 0;
m_loaderProxy.postTaskToLoader(MainThreadDestroyTask::create(peer.release()));

in void WorkerThreadableWebSocketChannel::Bridge::disconnect() and make

You may need to make WorkerThreadableWebSocketChannel::mainThreadDestroy take a PassOwnPtr.

and put in a comment that this method is there just to allow peer to get deleted and even if it doesn't run, the destructor for the callbacktask will delete it.

[Yuta Kitamura]

Created attachment 135980 [details]
Patch v2

[David Levin]

Bonus point for getting rid of AllowCrossThreadAccess in one place. :)

[Yuta Kitamura]

Comment on attachment 135980 [details]
Patch v2

Thanks!

[WebKit Review Bot]

Comment on attachment 135980 [details]
Patch v2

Clearing flags on attachment: 135980

Committed r113414: <http://trac.webkit.org/changeset/113414>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.