[webkit-reviews] review granted: [Bug 220482] Crash at SOAuthorizationSession::dismissViewController : [Attachment 417419] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jiewen Tan <jiewen_tan at apple.com>'s
request for review:
Bug 220482: Crash at SOAuthorizationSession::dismissViewController
https://bugs.webkit.org/show_bug.cgi?id=220482

Attachment 417419: Patch

https://bugs.webkit.org/attachment.cgi?id=417419&action=review




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 417419
  --> https://bugs.webkit.org/attachment.cgi?id=417419
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.