RESOLVED FIXED 220482
Crash at SOAuthorizationSession::dismissViewController
https://bugs.webkit.org/show_bug.cgi?id=220482
Summary Crash at SOAuthorizationSession::dismissViewController
Jiewen Tan
Reported 2021-01-08 14:24:31 PST
Crash at SOAuthorizationSession::dismissViewController.
Attachments
Patch (3.79 KB, patch)
2021-01-08 14:37 PST, Jiewen Tan
no flags
Patch (2.48 KB, patch)
2021-01-11 12:39 PST, Jiewen Tan
no flags
Patch (2.52 KB, patch)
2021-01-11 16:17 PST, Jiewen Tan
no flags
Jiewen Tan
Comment 1 2021-01-08 14:24:56 PST
Jiewen Tan
Comment 2 2021-01-08 14:37:31 PST
youenn fablet
Comment 3 2021-01-11 04:33:05 PST
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
Jiewen Tan
Comment 4 2021-01-11 12:37:35 PST
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.
Jiewen Tan
Comment 5 2021-01-11 12:39:57 PST
youenn fablet
Comment 6 2021-01-11 12:51:11 PST
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.
Jiewen Tan
Comment 7 2021-01-11 16:17:31 PST
Jiewen Tan
Comment 8 2021-01-11 16:17:41 PST
(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.
Darin Adler
Comment 9 2021-01-13 12:06:26 PST
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.
Jiewen Tan
Comment 10 2021-01-13 15:11:49 PST
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.
EWS
Comment 11 2021-01-13 15:19:28 PST
Committed r271467: <https://trac.webkit.org/changeset/271467> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417419 [details].
Note You need to log in before you can comment on or make changes to this bug.