[Webkit-unassigned] [Bug 222336] WKScriptMessage can race against WKWebView destruction

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=222336

--- Comment #4 from Joshua Wise <joshua.wise at fullstory.com> ---
Hi Chris --

In general, I agree with you: the UIKit API (and the WKWebView API) shouldn't be called off the main thread.  I think that `release`ing -- and, by extension, `dealloc`ing -- a WKWebView needs to be a special exception to this rule, though.  It is very difficult to avoid accidentally causing a `[WKWebView dealloc]` off of the main thread: although in this minimal reproducer case, a `WKWebView` is directly held off the main thread, consider the case in which another object deeply holds a strong reference to a `WKWebView`, and another thread happens to hold a strong reference to this object.

It seems like an exceptionally non-obvious pitfall that this can go wrong, along three axes of surprise for a programmer:

1) There is no textual representation in the source that a disallowed use of `WKWebView` will occur: even if a block does not directly make mention of a `WKWebView`, the failure can occur.  (Phrased another way, it would be expected that it is unsafe to use a `WKWebView` -- but it's not expected that it's unsafe to even "think about" a `WKWebView`.)

2) Although this is disallowed behavior for other `UIKit` APIs (and objects that derive from `UIView`), in practice, deallocation from off the main thread does not seem to fail for other `UIKit` APIs.  (This is, admittedly, weak justification.  But programmers are, if nothing else, pattern recognition machines...)

3) There is a violation of an otherwise-reasonable assumption in a programmer's mental model that ARC-managed objects should be, in general, responsible for avoiding wild pointers (i.e., that an ARC-managed object, if it needs to have a longer retain lifecycle than a local scope allows, should be managed by the holder of that pointer).

Although `WKWebView` is technically *allowed* to fail in this fashion by its specification, it is probably not a good idea for it to do so nondeterministically: it should either be tolerant of this misuse, or reliably crash or emit a diagnostic on this misuse [1].

Technically, it is probably not a bug, if the defect metric is "does WKWebView correctly implement the specification".  But if the question is "does WKWebView present an interface that avoids footguns", I would probably classify this as a defect.

Thanks for your consideration --
joshua

[1] One way that `WKWebView` could emit a diagnostic is particularly unpleasant: it could implement `[WKWebView release]` on its own (eventually calling into `[NSObject release]`), and call into the particularly-reviled `[self retainCount]` to determine whether the system is about to deallocate the object.  If it is, and the `release` operation is happening off the main thread -- i.e., `[self retainCount] == 1 && ![NSThread isMainThread]` -- then WKWebView could subsequently assert, or minimally, emit a warning.

This relies on additional undefined and unpleasant behavior -- `[NSObject retainCount]` is specified to be unspecified.  But not only does it *happen* to work for this application, examination of the Objective-C runtime reveals that it *will* work for this application, too.  Tests of this by swizzling `[WKWebView release]` reliably catch this behavior in user applications.  (Astute observers may note that this cannot be 100% reliable, and that there is another -- smaller -- race condition here.  That's true, but the diagnostic now occurs when we 'win' the race, not when we 'lose', and as a best-effort method to help users avoid API misuse, that's probably good enough.)

All that said, doing that is not pleasant.  It may just be better to make WKWebView tolerant of this misuse :-)

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210317/7f340867/attachment.htm>