Bug 220482 – Crash at SOAuthorizationSession::dismissViewController

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jiewen Tan]

Crash at SOAuthorizationSession::dismissViewController.

[Jiewen Tan]

<rdar://problem/72375494>

[Jiewen Tan]

Created attachment 417304 [details]
Patch

[youenn fablet]

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 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]

Created attachment 417401 [details]
Patch

[youenn fablet]

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]

Created attachment 417419 [details]
Patch

[Jiewen Tan]

(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 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 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]

Committed r271467: <https://trac.webkit.org/changeset/271467>

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