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
<rdar://problem/62194917>
Created attachment 397262 [details] Patch
Created attachment 397264 [details] Patch
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?
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.
Created attachment 397276 [details] Patch
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.
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.
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.
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.
Committed r260544: <https://trac.webkit.org/changeset/260544> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397276 [details].
(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.
Committed r260595: <https://trac.webkit.org/changeset/260595>