WebScriptObject maintains a NSMap of wrapper objects. A race condition exists between a wrapper being retrieved from the map, and being released - if the final release on an object is called between a call to getJSWrapper and the subsequent retain, we may end up with a stale object reference. We can make this safe by hoisting the removal from the map from delloc up into release (if the retainCount is 1), and locking release against retrieval from the map. Since release may be called from another thread, and NSMap is not threadsafe, we'd better lock around all access to the map (this fix already necessitates get & remove to be locked, so this just adds 'add', too).
rdar:9906341
Created attachment 151403 [details] Fix
Fixed in r122198
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.