Bug 90849 – Threadsafety issues in WebScriptObject

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Gavin Barraclough]

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).

[Gavin Barraclough]

rdar:9906341

[Gavin Barraclough]

Created attachment 151403 [details]
Fix

[Gavin Barraclough]

Fixed in r122198

[Gavin Barraclough]

Created attachment 151885 [details]
New improved fix, less risk of deadlock due to lock inversion!

[Filip Pizlo]

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?

[Gavin Barraclough]

Created attachment 151994 [details]
+ ASSERTs

[Gavin Barraclough]

(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.

[Gavin Barraclough]

Created attachment 152012 [details]
Simplify spinlock initialization

[Build Bot]

Comment on attachment 152012 [details]
Simplify spinlock initialization

Attachment 152012 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13199799

[Gavin Barraclough]

Fixed in r122494

[Andy Estes]

(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.

[Andy Estes]

(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.