Bug 32614 - [Qt] Memory leak detected in ThreadingQt
Summary: [Qt] Memory leak detected in ThreadingQt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Chang Shu
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2009-12-16 09:20 PST by Chang Shu
Modified: 2009-12-28 12:29 PST (History)
5 users (show)

See Also:


Attachments
fix patch (1.08 KB, patch)
2009-12-20 08:03 PST, Chang Shu
no flags Details | Formatted Diff | Diff
fix patch to take care both synchronous and asynchronous mode (1.42 KB, patch)
2009-12-23 13:59 PST, Chang Shu
no flags Details | Formatted Diff | Diff
fix crash in patch 2 (2.11 KB, patch)
2009-12-28 10:54 PST, Chang Shu
no flags Details | Formatted Diff | Diff
fix style check (2.10 KB, patch)
2009-12-28 11:45 PST, Chang Shu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chang Shu 2009-12-16 09:20:30 PST
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);
...
Comment 1 Chang Shu 2009-12-20 08:03:49 PST
Created attachment 45280 [details]
fix patch
Comment 2 WebKit Review Bot 2009-12-20 08:06:53 PST
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
Comment 3 Adam Barth 2009-12-20 09:45:35 PST
Haha.  That's a lame false positive from the style bot.  I'll file a bug.
Comment 4 Shinichiro Hamaji 2009-12-20 10:08:49 PST
My apologies. This is my mistake. I posted a patch to fix this false alarm. Thanks Adam for filing this bug.
Comment 5 Chang Shu 2009-12-23 13:59:48 PST
Created attachment 45450 [details]
fix patch to take care both synchronous and asynchronous mode
Comment 6 WebKit Review Bot 2009-12-23 14:00:14 PST
style-queue ran check-webkit-style on attachment 45450 [details] without any errors.
Comment 7 Laszlo Gombos 2009-12-24 08:54:38 PST
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.
Comment 8 WebKit Commit Bot 2009-12-24 09:03:51 PST
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>
Comment 9 WebKit Commit Bot 2009-12-24 09:03:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Laszlo Gombos 2009-12-24 09:36:33 PST
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.
Comment 11 Chang Shu 2009-12-28 10:54:09 PST
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.
Comment 12 WebKit Review Bot 2009-12-28 10:57:07 PST
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
Comment 13 Chang Shu 2009-12-28 11:45:59 PST
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.
Comment 14 WebKit Review Bot 2009-12-28 11:48:02 PST
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
Comment 15 Laszlo Gombos 2009-12-28 12:16:11 PST
Comment on attachment 45564 [details]
fix style check

Looks good, r=me
Comment 16 WebKit Commit Bot 2009-12-28 12:28:59 PST
Comment on attachment 45564 [details]
fix style check

Clearing flags on attachment: 45564

Committed r52602: <http://trac.webkit.org/changeset/52602>
Comment 17 WebKit Commit Bot 2009-12-28 12:29:04 PST
All reviewed patches have been landed.  Closing bug.