Bug 32614 – [Qt] Memory leak detected in ThreadingQt

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chang Shu]

a new instance of ThreadPriviate is created in function createThreadInternal() (ThreadingQt.cpp), but no place takes care of the deletion. See code below.

ThreadIdentifier createThreadInternal(ThreadFunction entryPoint, void* data, const char*)
{
    ThreadPrivate* thread = new ThreadPrivate(entryPoint, data);
...

[Chang Shu]

Created attachment 45280 [details]
fix patch

[WebKit Review Bot]

Attachment 45280 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/qt/ThreadingQt.cpp:183:  static_cast is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1

[Adam Barth]

Haha.  That's a lame false positive from the style bot.  I'll file a bug.

[Shinichiro Hamaji]

My apologies. This is my mistake. I posted a patch to fix this false alarm. Thanks Adam for filing this bug.

[Chang Shu]

Created attachment 45450 [details]
fix patch to take care both synchronous and asynchronous mode

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45450 [details] without any errors.

[Laszlo Gombos]

Comment on attachment 45450 [details]
fix patch to take care both synchronous and asynchronous mode

This looks good to me as detachThread() could be called from any thread including "self" thread, but the ThreadPrivate destructor is called from the main application thread.

LGTM, r=me.

[WebKit Commit Bot]

Comment on attachment 45450 [details]
fix patch to take care both synchronous and asynchronous mode

Clearing flags on attachment: 45450

Committed r52550: <http://trac.webkit.org/changeset/52550>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Laszlo Gombos]

REOPEN as The change regressed the following LayoutTests for QtWebKit.

fast/workers/worker-call.html -> crashed
fast/workers/worker-close.html -> crashed

Reverted as http://trac.webkit.org/changeset/52551.

[Chang Shu]

Created attachment 45563 [details]
fix crash in patch 2

calling deleteLater in detachThread may not guarantee the thread is finished while being deleted. Using a slot function to capture the finished() signal and calling deleteLater from there is safe.

[WebKit Review Bot]

Attachment 45563 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/qt/ThreadingQt.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
JavaScriptCore/wtf/qt/ThreadingQt.cpp:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
JavaScriptCore/wtf/qt/ThreadingQt.cpp:291:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3

[Chang Shu]

Created attachment 45564 [details]
fix style check

fixed the first 2 style checks.
I think the 3rd one is a kind of false alarm. Qt requires to put "#include *.moc" at the end of the cpp file. I ran the check-webkit-style script on some existing files and got the same complains.

[WebKit Review Bot]

Attachment 45564 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/qt/ThreadingQt.cpp:293:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1

[Laszlo Gombos]

Comment on attachment 45564 [details]
fix style check

Looks good, r=me

[WebKit Commit Bot]

Comment on attachment 45564 [details]
fix style check

Clearing flags on attachment: 45564

Committed r52602: <http://trac.webkit.org/changeset/52602>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.