Bug 210873 – [iOS] Crash on RunningBoard process assertion invalidation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Crash on RunningBoard process assertion invalidation:
Thread 0 name:  Dispatch queue: com.apple.main-thread
Thread 0 Crashed:
[...]
9   WebKit                        	0x000000018cff590c invocation function for block in WebKit::ProcessAssertion::ProcessAssertion(int, WTF::ASCIILiteral, WebKit::ProcessAssertionType) + 168 (/Library/Caches/com.apple.xbs/Sources/WebKit2/WebKit2-7610.1.10/UIProcess/ios/ProcessAssertionIOS.mm:375)
10  ...g_rt.asan_ios_dynamic.dylib	0x0000000104068e2c __wrap_dispatch_async_block_invoke + 188 
11  libdispatch.dylib             	0x0000000184717878 _dispatch_call_block_and_release + 24 
12  libdispatch.dylib             	0x0000000184718840 _dispatch_client_callout + 16 
13  libdispatch.dylib             	0x00000001846fade8 _dispatch_main_queue_callback_4CF$VARIANT$armv81 + 856 
14  CoreFoundation                	0x00000001849d0e48 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 12
15  CoreFoundation                	0x00000001849cb790 __CFRunLoopRun + 1884
16  CoreFoundation                	0x00000001849cac50 CFRunLoopRunSpecific + 520
17  XCTest                        	0x0000000106089668 -[XCTWaiter waitForExpectations:timeout:enforceOrder:] + 808

[Chris Dumez]

<rdar://problem/62194917>

[Chris Dumez]

Created attachment 397262 [details]
Patch

[Chris Dumez]

Created attachment 397264 [details]
Patch

[Darin Adler]

Comment on attachment 397264 [details]
Patch

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

> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:293
> +        if (weakSelf && weakSelf.invalidationCallback)
> +            weakSelf.invalidationCallback();

The pattern I see is that people copy such things into a strong pointer, so there’s no race with another process. I think that might be needed here.

> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:424
> +        m_delegate.get().invalidationCallback = nil;

I am mixed up a bit: What guarantees that the main queue thread didn’t already fetch the callback and then will call it some arbitrary length of time later when it was up?

[Chris Dumez]

Comment on attachment 397264 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:293
>> +            weakSelf.invalidationCallback();
> 
> The pattern I see is that people copy such things into a strong pointer, so there’s no race with another process. I think that might be needed here.

I am not sure I understand your comment. Do you want me to use a strongSelf? If so, it would work but I don't think it is necessary to keep WKRBSAssertionDelegate alive longer than the ProcessAssertion since we're only calling the ProcessAssertion's invalidationHandler.

>> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:424
>> +        m_delegate.get().invalidationCallback = nil;
> 
> I am mixed up a bit: What guarantees that the main queue thread didn’t already fetch the callback and then will call it some arbitrary length of time later when it was up?

The ProcessAssertion destructor always runs on the main thread. Both WKRBSAssertionDelegate and ProcessAssertion only set / access invalidationCallback on the main thread. Therefore, there is no race.

[Chris Dumez]

Created attachment 397276 [details]
Patch

[Chris Dumez]

Comment on attachment 397276 [details]
Patch

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

> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:286
>  - (void)assertion:(RBSAssertion *)assertion didInvalidateWithError:(NSError *)error

This may get called on a background thread, thus the dispatch_async below before accessing / calling invalidationCallback. invalidationCallback gets set on the main thread by the ProcessAssertion code and the ProcessAssertion expects the invalidation handler to get called on the main thread.

> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:425
> +        m_delegate.get().invalidationCallback = nil;

ProcessAssertion is a main thread object. It always gets constructed, destroyed on the main thread. 

m_delegate.get().invalidationCallback gets initialized in the ProcessAssertion constructor on the main thread.
m_delegate.get().invalidationCallback gets null out here in the destructor on the main thread.
m_delegate.get().invalidationCallback gets accessed in WKRBSAssertionDelegate.didInvalidateWithError on the main thread (enforced by our dispatch_async)

therefore, the invalidationCallback is always used on the main thread and there should be no race.

[Chris Dumez]

Comment on attachment 397276 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:425
>> +        m_delegate.get().invalidationCallback = nil;
> 
> ProcessAssertion is a main thread object. It always gets constructed, destroyed on the main thread. 
> 
> m_delegate.get().invalidationCallback gets initialized in the ProcessAssertion constructor on the main thread.
> m_delegate.get().invalidationCallback gets null out here in the destructor on the main thread.
> m_delegate.get().invalidationCallback gets accessed in WKRBSAssertionDelegate.didInvalidateWithError on the main thread (enforced by our dispatch_async)
> 
> therefore, the invalidationCallback is always used on the main thread and there should be no race.

Also note that ProcessAssertion retains the WKRBSAssertionDelegate via m_delegate.

[Chris Dumez]

Comment on attachment 397276 [details]
Patch

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

> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:292
> +        WKRBSAssertionDelegate *strongSelf = weakSelf;

Hopefully this is what Darin meant by using strongSelf. Geoff recommended doing this to avoid races.

[Darin Adler]

Comment on attachment 397276 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:292
>> +        WKRBSAssertionDelegate *strongSelf = weakSelf;
> 
> Hopefully this is what Darin meant by using strongSelf. Geoff recommended doing this to avoid races.

Yes, this is what I meant. Not sure it’s needed, but this is the pattern I have seen before.

[EWS]

Committed r260544: <https://trac.webkit.org/changeset/260544>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397276 [details].

[Adam Roben (:aroben)]

(In reply to Darin Adler from comment #10)
> Comment on attachment 397276 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397276&action=review
> 
> >> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:292
> >> +        WKRBSAssertionDelegate *strongSelf = weakSelf;
> > 
> > Hopefully this is what Darin meant by using strongSelf. Geoff recommended doing this to avoid races.
> 
> Yes, this is what I meant. Not sure it’s needed, but this is the pattern I
> have seen before.

It also has a minor performance benefit since each use of a weak variable involves some locking and hash table lookups in the Objective-C runtime.

[Chris Dumez]

Committed r260595: <https://trac.webkit.org/changeset/260595>