DropAllLocks RELEASE_ASSERT on iOS

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Ian Ragsdale]

Created attachment 243311 [details]
Instruments trace leading up to the crash.

I'm using WebKit in an iOS app (via UIWebView), and we're seeing a semi-frequent crash that I'm trying to track down. From the backtraces, I _think_ it appears to be a WebKit bug, and so I'd like to try to find a workaround, and/or submit a useful bug or patch.

A full thread dump is available here: http://crashes.to/s/cf0cdb52701

The assertion appears to be happening when the WebThread tries to call my delegate to decide whether to load a URL:

Thread : Crashed: WebThread
0  JavaScriptCore                 0x27e864aa WTFCrash + 53
1  JavaScriptCore                 0x27e86457 WTFPrintBacktrace + 130
2  JavaScriptCore                 0x27dc92e1 JSC::JSLock::DropAllLocks::DropAllLocks(JSC::VM*)
3  WebCore                        0x31cd3061 SendDelegateMessage(NSInvocation*) + 184
4  WebKitLegacy                   0x327be1f5 -[_WebSafeForwarder forwardInvocation:] + 116
5  CoreFoundation                 0x269d766f ___forwarding___ + 354
6  CoreFoundation                 0x26909058 _CF_forwarding_prep_0 + 24
7  WebKitLegacy                   0x327ffb01 WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(WebCore::NavigationAction const&, WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, std::__1::function<void (WebCore::PolicyAction)>) + 344

From looking at the source, it tries to drop all locks from the current javascript VM before calling the delegate, and when it does that it asserts if the VM is busy garbage collecting. I'm guessing there needs to be some sort of guard there to make sure the VM isn't doing GC before dropping the locks?

This crash becomes much easier to trigger when setting JSC_slowPathAllocsBetweenGCs to a low number.

I've attached an Instruments trace leading up to the crash, which I believe verifies that the app is not accessing the UIWebView on any non-main thread.

[Ian Ragsdale]

I've got a bit of new information to share. We released a new beta of the app in which this crash went from infrequent to very frequent (from 8 crashes in ~6k sessions to 160 crashes in ~5k sessions).

Very little in the app changed - some CSS changes, some minor Javascript changes (animating a width change and a few more style changes in a completion callback), and a change to how we detect the initial document load is complete. However, that was enough for a huge spike in crashes.

This was interesting because while the stack trace of the crashed thread varied, 156 of the 160 crashes showed the same backtrace for the main thread:

Thread : com.apple.main-thread
0  libsystem_kernel.dylib         0x2fe05ba8 __psynch_mutexwait + 24
1  libsystem_pthread.dylib        0x2fe8104b _pthread_mutex_lock + 398
2  WebCore                        0x2d353b91 _WebTryThreadLock(bool) + 44
3  WebCore                        0x2d3543ad WebThreadLock + 80
4  UIKit                          0x2561ca69 -[UIWebDocumentView setDataDetectorTypes:] + 56
5  Boxer                          0x000cc245 -[ConversationViewController renderConversation] (ConversationViewController.m:869)
6  libdispatch.dylib              0x2fd1f7bb _dispatch_call_block_and_release + 10
7  libdispatch.dylib              0x2fd1f7a7 _dispatch_client_callout + 22
8  libdispatch.dylib              0x2fd22fa3 _dispatch_main_queue_callback_4CF + 718
9  CoreFoundation                 0x21fcf3b1 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 8
10 CoreFoundation                 0x21fcdab1 __CFRunLoopRun + 1512
11 CoreFoundation                 0x21f1b3c1 CFRunLoopRunSpecific + 476
12 CoreFoundation                 0x21f1b1d3 CFRunLoopRunInMode + 106
13 GraphicsServices               0x293190a9 GSEventRunModal + 136
14 UIKit                          0x2552afa1 UIApplicationMain + 1440
15 Boxer                          0x000900a7 main (main.m:11)

(More details here: http://crashes.to/s/b1531e29971)

[Geoffrey Garen]

I looked through the Instruments trace and it convinced me of something that, in retrospect, is obvious just from the crashing backtrace: It's possible for SendDelegateMessage to DropAllLocks when it doesn't hold the lock and, if it does so while the main thread is garbage collecting, this RELEASE_ASSERT will file.

I think the right solution here is to refactor this RELEASE_ASSERT not to fire under this condition, since the condition is otherwise harmless.

[Geoffrey Garen]

The only way to work around this bug is to avoid using -stringByEvaluatingJavaScriptFromString: on the main thread while the web thread may be loading. If you're just trying to execute code, and you don't care about the return value, you can wrap your code in a zero-delay timer and you'll eliminate 99.9% of the risk of crash here.

[Ian Ragsdale]

Thanks, Geoff! I think that workaround will work for us, really appreciate you taking a look at this. If there is anything else I can do to help lock this one down let me know.

[Radar WebKit Bug Importer]

<rdar://problem/19281601>

[Geoffrey Garen]

Created attachment 254382 [details]
Patch

[Mark Lam]

Comment on attachment 254382 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSLock.cpp:266
> +    RELEASE_ASSERT(!currentThreadIsHoldingLock() || !m_vm->isCollectorBusy());

Shouldn’t this be 
    RELEASE_ASSERT(currentThreadIsHoldingLock() && !m_vm->isCollectorBusy());

The current thread shouldn’t be allowed to drop locks if it doesn’t own it, or if GC is in progress.

[Geoffrey Garen]

> The current thread shouldn’t be allowed to drop locks if it doesn’t own it

Yes it should be:

        * runtime/JSLock.cpp:
        (JSC::JSLock::dropAllLocks): Removed a comment because it duplicated
        the code beneath it. Removed a FIXME because we can't ASSERT that
        we're holding the lock. WebKit1 on iOS drops the lock before calling to
        delegates, not knowing whether it holds the lock or not.

[Geoffrey Garen]

Created attachment 254386 [details]
Patch

[Geoffrey Garen]

New patch to fix the build.

[Geoffrey Garen]

Note that dropping the lock when you do not hold it is a no-op. (You are not allowed to drop someone else's lock.)

[Mark Lam]

(In reply to comment #11)
> Note that dropping the lock when you do not hold it is a no-op. (You are not
> allowed to drop someone else's lock.)

That’s what I was missing!

[Mark Lam]

Comment on attachment 254386 [details]
Patch

r=me

[Geoffrey Garen]

Committed r185268: <http://trac.webkit.org/changeset/185268>