Bug 90849

Summary: Threadsafety issues in WebScriptObject
Product: WebKit Reporter: Gavin Barraclough <barraclough>
Component: WebCore JavaScriptAssignee: 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 Flags
Fix
fpizlo: review+
New improved fix, less risk of deadlock due to lock inversion!
none
+ ASSERTs
none
Simplify spinlock initialization oliver: review+, buildbot: commit-queue-

Description Gavin Barraclough 2012-07-09 23:20:29 PDT
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).
Comment 1 Gavin Barraclough 2012-07-09 23:30:22 PDT
rdar:9906341
Comment 2 Gavin Barraclough 2012-07-09 23:32:03 PDT
Created attachment 151403 [details]
Fix
Comment 3 Gavin Barraclough 2012-07-10 00:09:07 PDT
Fixed in r122198
Comment 4 Gavin Barraclough 2012-07-12 01:44:17 PDT
Created attachment 151885 [details]
New improved fix, less risk of deadlock due to lock inversion!
Comment 5 Filip Pizlo 2012-07-12 09:28:00 PDT
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?
Comment 6 Gavin Barraclough 2012-07-12 10:40:35 PDT
Created attachment 151994 [details]
+ ASSERTs
Comment 7 Gavin Barraclough 2012-07-12 10:49:33 PDT
(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.
Comment 8 Gavin Barraclough 2012-07-12 11:40:53 PDT
Created attachment 152012 [details]
Simplify spinlock initialization
Comment 9 Build Bot 2012-07-12 11:49:24 PDT
Comment on attachment 152012 [details]
Simplify spinlock initialization

Attachment 152012 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13199799
Comment 10 Gavin Barraclough 2012-07-12 13:04:10 PDT
Fixed in r122494
Comment 11 Andy Estes 2012-07-17 08:57:54 PDT
(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.
Comment 12 Andy Estes 2012-07-17 09:02:13 PDT
(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.