Summary: | [iOS] Crash on RunningBoard process assertion invalidation | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, aroben, darin, ddkilzer, ggaren, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Chris Dumez
2020-04-22 14:04:20 PDT
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> |