Summary: | Threadsafety issues in WebScriptObject | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gavin Barraclough <barraclough> | ||||||||||
Component: | WebCore JavaScript | Assignee: | Gavin Barraclough <barraclough> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aestes, ddkilzer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 91505 | ||||||||||||
Attachments: |
|
Description
Gavin Barraclough
2012-07-09 23:20:29 PDT
rdar:9906341 Created attachment 151403 [details]
Fix
Created attachment 151885 [details]
New improved fix, less risk of deadlock due to lock inversion!
Comment on attachment 151885 [details] New improved fix, less risk of deadlock due to lock inversion! View in context: https://bugs.webkit.org/attachment.cgi?id=151885&action=review > Source/WebCore/bindings/objc/WebScriptObject.mm:81 > + JSWrapperCacheSpinLock->Init(); This feels like it should be inside of a pthread_once thingy? Created attachment 151994 [details]
+ ASSERTs
(In reply to comment #5) > (From update of attachment 151885 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151885&action=review > > > Source/WebCore/bindings/objc/WebScriptObject.mm:81 > > + JSWrapperCacheSpinLock->Init(); > > This feels like it should be inside of a pthread_once thingy? This code is threadsafe because it will only be called from the main thread – that wasn't obvious from my previous patch, so I added ASSERTs to indicate this. The threadsafety issue with the JSWrapperCache map comes from the fact that 'release' may be called from a different thread, so we need to make release threadsafe wrt get/add. I've been a little conservative here. Creating the first wrapper will bring the JSWrapperCache into existence, and no wrappers can be released until the first wrapper has been created, so the wrapper cache must exist before remove can be called – so I think the 'if (!JSWrapperCache)' in the remove methods could probably just be an ASSERT too, but I've left this as is to be safe. With the extra ASSERTS I think we don't need the pthread_once, hope that looks good to you. G. Created attachment 152012 [details]
Simplify spinlock initialization
Comment on attachment 152012 [details] Simplify spinlock initialization Attachment 152012 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13199799 Fixed in r122494 (In reply to comment #10) > Fixed in r122494 Looks like this caused platform/mac/plugins/root-object-premature-delete-crash.html to crash on the bots. (In reply to comment #11) > (In reply to comment #10) > > Fixed in r122494 > > Looks like this caused platform/mac/plugins/root-object-premature-delete-crash.html to crash on the bots. https://bugs.webkit.org/show_bug.cgi?id=91505 tracks this. |