Crash at SOAuthorizationSession::dismissViewController.
<rdar://problem/72375494>
Created attachment 417304 [details] Patch
Comment on attachment 417304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417304&action=review > Source/WebKit/ChangeLog:16 > + chooses the latter. Or SOAuthorizationSession::dismissViewController() is called from a background thread. > Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:305 > + m_presentingWindowDidDeminiaturizeObserver = [[NSNotificationCenter defaultCenter] addObserverForName:NSWindowDidDeminiaturizeNotification object:presentingWindow queue:nil usingBlock:[weakThis = makeRefPtr(this), this] (NSNotification *) { Should be weakThis = makeWeakPtr(this) probably. > Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:321 > + m_applicationDidUnhideObserver = [[NSNotificationCenter defaultCenter] addObserverForName:NSApplicationDidUnhideNotification object:NSApp queue:nil usingBlock:[weakThis = makeRefPtr(this), this] (NSNotification *) { Ditto
Comment on attachment 417304 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417304&action=review Thanks Youenn for r+ this patch. Actually, I just realized that the object needs to be kept alive during the process. Therefore, I made another patch that utilized ThreadSafeRefCounted. >> Source/WebKit/ChangeLog:16 >> + chooses the latter. > > Or SOAuthorizationSession::dismissViewController() is called from a background thread. No, it's not. It's called from the main thread. >> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:305 >> + m_presentingWindowDidDeminiaturizeObserver = [[NSNotificationCenter defaultCenter] addObserverForName:NSWindowDidDeminiaturizeNotification object:presentingWindow queue:nil usingBlock:[weakThis = makeRefPtr(this), this] (NSNotification *) { > > Should be weakThis = makeWeakPtr(this) probably. Oops. >> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:321 >> + m_applicationDidUnhideObserver = [[NSNotificationCenter defaultCenter] addObserverForName:NSApplicationDidUnhideNotification object:NSApp queue:nil usingBlock:[weakThis = makeRefPtr(this), this] (NSNotification *) { > > Ditto Oops.
Created attachment 417401 [details] Patch
Comment on attachment 417401 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417401&action=review > Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.h:54 > +class SOAuthorizationSession : public ThreadSafeRefCounted<SOAuthorizationSession>, public CanMakeWeakPtr<SOAuthorizationSession> { You probably should mark it as DestructionThread::MainRunLoop.
Created attachment 417419 [details] Patch
(In reply to youenn fablet from comment #6) > Comment on attachment 417401 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417401&action=review > > > Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.h:54 > > +class SOAuthorizationSession : public ThreadSafeRefCounted<SOAuthorizationSession>, public CanMakeWeakPtr<SOAuthorizationSession> { > > You probably should mark it as DestructionThread::MainRunLoop. Fixed.
Comment on attachment 417419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417419&action=review > Source/WebKit/ChangeLog:15 > + One of the possible explanations is that the RefPtr is somehow over-released within NSNotificationCenter since > + it's not thread-safe. To fix that, the RefPtr can be made thread-safe. This is a *highly* likely explanation. I wish the change log wasn't so obliquely worded; it’s good to be humble that we aren’t sure, but maybe this is too tentative. Generally speaking, it’s *not* safe to have RefPtr fields inside Objective-C objects since Objective-C retain/release and deallocation are thread-safe and it’s common for them to happen on a non-main thread. Even an object that is normally only *used* on the main thread can often be *deallocated* on a non-main thread. WebCoreObjCScheduleDeallocateOnMainThread was created as one solution for this issue that does not require changes to the reference counting of the C++ objects. We should look for other examples of this mistake in WebKit, starting with RefPtr fields in Objective-C objects as well as code manipulating C++ reference-counted objects in dealloc methods.
Comment on attachment 417419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417419&action=review >> Source/WebKit/ChangeLog:15 >> + it's not thread-safe. To fix that, the RefPtr can be made thread-safe. > > This is a *highly* likely explanation. I wish the change log wasn't so obliquely worded; it’s good to be humble that we aren’t sure, but maybe this is too tentative. > > Generally speaking, it’s *not* safe to have RefPtr fields inside Objective-C objects since Objective-C retain/release and deallocation are thread-safe and it’s common for them to happen on a non-main thread. Even an object that is normally only *used* on the main thread can often be *deallocated* on a non-main thread. WebCoreObjCScheduleDeallocateOnMainThread was created as one solution for this issue that does not require changes to the reference counting of the C++ objects. > > We should look for other examples of this mistake in WebKit, starting with RefPtr fields in Objective-C objects as well as code manipulating C++ reference-counted objects in dealloc methods. Thanks Darin for r+ this patch. Right, I think it is a good approach to have some sort of helper classes and coding style rules to reduce the chance of making this kind of mistakes.
Committed r271467: <https://trac.webkit.org/changeset/271467> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417419 [details].