WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(2.48 KB, patch)
2021-01-11 12:39 PST
,
Jiewen Tan
no flags
Details
Formatted Diff
Diff
Patch
(2.52 KB, patch)
2021-01-11 16:17 PST
,
Jiewen Tan
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Jiewen Tan
Comment 1
2021-01-08 14:24:56 PST
<
rdar://problem/72375494
>
Jiewen Tan
Comment 2
2021-01-08 14:37:31 PST
Created
attachment 417304
[details]
Patch
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
Created
attachment 417401
[details]
Patch
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
Created
attachment 417419
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug