Summary: | Fix the leak of ThreadIdentifiers in threadMap across threads. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dmitry Titov <dimich> | ||||||||||
Component: | Web Template Framework | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | darin, eric, levin, mrobinson, webkit.review.bot, yael | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 31639 | ||||||||||||
Attachments: |
|
Description
Dmitry Titov
2009-12-17 18:10:46 PST
Created attachment 45119 [details]
Proposed patch.
One more comment on the patch: it might look that removing threadMap mapping in thread-specific destructor will cause calls like detachThread(threadID) or waitForThreadCompletion(threadID) made from other threads fail if they happen to be made just after the tread identified by threadID terminates. From what I see in debugger, Safari that uses WTF to start threads can realize this call pattern. This is ok, actually. Since resolving threadID via map will return 0 pthread handle, the pthread methods will simply return error and do nothing. Comment on attachment 45119 [details]
Proposed patch.
The code change and your explanation look good to me, but is it possible to provide a test case? It's ok if the test case only fails in debug builds with the relevant asserts enabled. If it's not possible to create a test case, please explain why in the ChangeLog.
Created attachment 45951 [details]
Patch with test.
Same change, with a test.
See DumRenderTree.mm and WebKitTools/ChangeLog for the description of the test. Basically, it starts couple of trivial threads that use WTF::currentThread() method. Without the fix, there are at least 2 ASSERTS on the way of DRT to completion.
It simulates scenarios when embedders like Safari run threads not created by WTF that call into JSC/WebKit code which can use currentThread(), or DRT running with '--threaded', to name a couple.
Attachment 45951 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/ThreadIdentifierDataPthreads.cpp:35: You should add a blank line after implementation file's own header. [build/include_order] [4]
JavaScriptCore/wtf/ThreadIdentifierDataPthreads.cpp:52: this_ptr is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
JavaScriptCore/wtf/ThreadIdentifierDataPthreads.cpp:67: this_ptr is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Total errors found: 3
Created attachment 45988 [details]
Patch with test and style fix.
Fixed style issue noticed by style bot.
Attachment 45988 [details] did not pass style-queue:
Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/ThreadIdentifierDataPthreads.cpp:35: You should add a blank line after implementation file's own header. [build/include_order] [4]
Total errors found: 1
Created attachment 46004 [details]
Updated patch.
I should have used check-webkit-style before... Updated patch.
style-queue ran check-webkit-style on attachment 46004 [details] without any errors.
(In reply to comment #8) > Created an attachment (id=46004) [details] > Updated patch. > > I should have used check-webkit-style before... Updated patch. I've filed bug 33275 on your behalf. :) Comment on attachment 46004 [details]
Updated patch.
r=me
I was hoping for a test that shows what goes wrong in actual browsing if this bug is not fixed, but the unit test you added seems fine too.
(In reply to comment #11) > (From update of attachment 46004 [details]) > r=me > > I was hoping for a test that shows what goes wrong in actual browsing if this > bug is not fixed, but the unit test you added seems fine too. Today, it doesn't fail in browsing (at least I don't have a repro case) but it prevents adding more ASSERTS with CurrentThread() verification (for example, a patch for bug 31639 is blocked by this). Thanks a lot for review! This appears to have caused Tiger nightly builds to crash after loading a page. Please see bug 34039. Seems we should roll out this change then. The only reason I haven't already is that the Tiger bug is not confirmed. Bug 34039 has been confirmed. I think this should be rolled out. Alexey reports the regression as being fixed in r53815. The GTK+ port seems to be hitting a lot of ASSERTs that look like this: ASSERTION FAILED: m_thread == currentThread() Is this related to this bug at all? Perhaps we are still missing some code? I'm happy to implement it. |