Bug 179190 – [iOS-WK1] Fix thread safety issue in WebSQLiteDatabaseTrackerClient

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

Fix thread safety issue in WebSQLiteDatabaseTrackerClient on iOS WK1.

WebSQLiteDatabaseTrackerClient and its HystererisActivity member are constructed on the UIThread. The HystererisActivity activity also fires on the UIThread, which means that WebSQLiteDatabaseTrackerClient::hysteresisUpdated() gets called on the UIThread.
However, the code in WebSQLiteDatabaseTrackerClient::willBeginFirstTransaction() / WebSQLiteDatabaseTrackerClient::didFinishLastTransaction() uses callOnMainThread() before calling methods on the HysteresisActivity. callOnMainThread() dispatches to the WebThread on WK1 iOS, which would lead to crashes when calling methods of the HystererisActivity object:
  *** -[CFRunLoopTimer respondsToSelector:]: message sent to deallocated instance 0x1c0b6a500

[Chris Dumez]

Created attachment 325746 [details]
Patch

[Chris Dumez]

<rdar://problem/34413695>

[Build Bot]

Comment on attachment 325746 [details]
Patch

Attachment 325746 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5079376

New failing tests:
imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting-cache.https.html

[Build Bot]

Created attachment 325762 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[David Kilzer (:ddkilzer)]

Comment on attachment 325746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325746&action=review

r=me if iOS Simulator layout test failure was not caused by this change.

> Source/WebCore/platform/ios/WebSQLiteDatabaseTrackerClient.mm:55
> +    ASSERT([NSThread isMainThread]);

Optionally (to avoid Objective-C overhead):

    ASSERT(ptrhread_main_np());

Not a blocker to land the patch.  Change at your discretion.

> Source/WebCore/platform/ios/WebSQLiteDatabaseTrackerClient.mm:65
> +    dispatch_async(dispatch_get_main_queue(), [this] {
> +        ASSERT([NSThread isMainThread]);

This assertion seems redundant; if dispatch_async(dispatch_get_main_queue(), ...) doesn't work, we have bigger problems.  :)

> Source/WebCore/platform/ios/WebSQLiteDatabaseTrackerClient.mm:73
> +    dispatch_async(dispatch_get_main_queue(), [this] {
> +        ASSERT([NSThread isMainThread]);

Ditto.

> Source/WebCore/platform/ios/WebSQLiteDatabaseTrackerClient.mm:80
> +    ASSERT([NSThread isMainThread]);

Optionally (to avoid Objective-C overhead):

    ASSERT(ptrhread_main_np());

[Chris Dumez]

Created attachment 325988 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 325988 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=325988&action=review

> Source/WebCore/ChangeLog:15
> +        would lead to crashes when calling methods of the HystererisActivity object:
> +        *** -[CFRunLoopTimer respondsToSelector:]: message sent to deallocated instance 0x1c0b6a500

y tho?

> Source/WebCore/platform/ios/WebSQLiteDatabaseTrackerClient.mm:65
> +    dispatch_async(dispatch_get_main_queue(), [this] {
>          m_hysteresis.start();

Don't you need to WebThreadLock here?

> Source/WebCore/platform/ios/WebSQLiteDatabaseTrackerClient.mm:71
> +    dispatch_async(dispatch_get_main_queue(), [this] {

Don't you need to WebThreadLock here?

[Simon Fraser (smfr)]

Seems like Dave and I are giving conflicting advice.

My concern is that it's hard to guarantee that the chain of callbacks from, say, HysteresisActivity::stop() never touch WebCore code.

[Chris Dumez]

(In reply to Simon Fraser (smfr) from comment #8)
> Seems like Dave and I are giving conflicting advice.
> 
> My concern is that it's hard to guarantee that the chain of callbacks from,
> say, HysteresisActivity::stop() never touch WebCore code.

HysteresisActivity is merely a wrapper around a RunLoop::Timer. It does not touch WebCore code.

[Chris Dumez]

(In reply to Simon Fraser (smfr) from comment #7)
> Comment on attachment 325988 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325988&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        would lead to crashes when calling methods of the HystererisActivity object:
> > +        *** -[CFRunLoopTimer respondsToSelector:]: message sent to deallocated instance 0x1c0b6a500
> 
> y tho?

Here the HysteresisActivity fires (on the UI thread), it calls RunLoop::Timer::stop():
	 void RunLoop::TimerBase::stop()
	{
	    if (!m_timer)
	        return;
	    
	    CFRunLoopTimerInvalidate(m_timer.get());
	    m_timer = nullptr;
	}


At the same time, WebSQLiteDatabaseTrackerClient::willBeginFirstTransaction() could dispatch the the WebThread and call m_hysteresis.start() which calls RunLoop::Timer:isActive():
bool RunLoop::TimerBase::isActive() const
   {
         return m_timer && CFRunLoopTimerIsValid(m_timer.get());
    }

So we have 1 thread invalidating and nulling out m_timer and the other thread calling CFRunLoopTimerIsValid() on the same m_timer.

[WebKit Commit Bot]

Comment on attachment 325988 [details]
Patch

Clearing flags on attachment: 325988

Committed r224452: <https://trac.webkit.org/changeset/224452>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.