[webkit-reviews] review denied: [Bug 185330] [iOS] Add assert to catch improper use of WebCore::Timer in UI Process : [Attachment 341541] Patch v2

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has denied  review:
Bug 185330: [iOS] Add assert to catch improper use of WebCore::Timer in UI
Process
https://bugs.webkit.org/show_bug.cgi?id=185330

Attachment 341541: Patch v2

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




--- Comment #17 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 341541
  --> https://bugs.webkit.org/attachment.cgi?id=341541
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=341541&action=review

>> Source/WebCore/platform/RuntimeApplicationChecks.h:48
>>  bool isInWebProcess();
> 
> These seem like WebKit concepts, not Cocoa concepts. Not great to have this
inside #if PLATFORM(COCOA).

Will fix for all the platforms except Windows (see code above this).

>> Source/WebCore/platform/Timer.cpp:196
>> +#if PLATFORM(IOS)
> 
> I think we should consider compiling this code on all platforms.

This is currently only an issue on iOS because of WebThread (please see next
comment).

Enabling on macOS (or all platforms) would only be useful insofar as not to
reintroduce the issue on iOS in shared code.  While I agree that's useful, it
will require a different type of check that's more difficult to get right (and
would probably require a slightly different solution for each platform), so
landing the iOS check first is not letting perfect be the enemy of good.

>> Source/WebCore/platform/Timer.cpp:262
>> +#if PLATFORM(IOS)
> 
> Why only iOS? Is this mistake unlikely to happen on other platforms? I seem
to recall that manipulating timers on non-main threads was a bad, hard-to-debug
problem on macOS, for example.

This only matters for iOS due to WebThread.  Enforcing this rule makes it
possible to use UIWebView and WKWebView together in the same UI process without
thread safety issues and the resulting long-tail crashes.

When WebThread is enabled, it forces all WebCore::Timers (from both WK1
UIWebView and WK2 WKWebView) to be scheduled on WebThread by using WebThread's
runloop.

Because WK2 doesn't know about WebThread and WebThreadLock (and we don't want
to teach it about it), all assumptions about the WebCore::Timers running on the
main thread are gone, and thread safety issues are introduced.

So the rule is that if a process will never instantiate a UIWebView (thus
starting WebThread) or if the WebThread is not running, then WebCore::Timers
are allowed.

The most obvious case where WebCore::Timers are allowed is the WebContent,
Networking and Storage processes for WK2.

The second case is any iOS app that never instantiates a UIWebView (thus
causing WebThread to be started).  Note that there are non-obvious ways that a
UIWebView is created, such as by using the NSAttributedString API to convert
HTML to an NSAttributedString, or by third-party developers including
third-party advertising frameworks in their apps.

>> Source/WebCore/platform/Timer.cpp:269
>> +#endif
> 
> Why no main thread check when WebThreadIsEnabled is false? I assume it’s not
OK to use a timer non-main thread in cases other than the web thread.

It is a non-goal to check which thread the WebCore::Timer is used on for this
patch.

See previous comment, but in a nutshell, the rule is that no WebCore::Timer (on
any thread) shall be used in a (UI) process that has WebThread active on iOS so
that UIWebView and WKWebView can co-exist peacefully (due to WebThread forcing
all WebCore::Timers to run on WebThread).

>> Source/WebCore/platform/Timer.h:81
>> +	static bool isAllowedToUseWebCoreTimer();
> 
> Function name is a bit strange. This class is WebCore::TimerBase so it
doesn’t make so much sense that it has a function with the words "WebCore" and
"Timer" in its name.

Will change to isAllowedToUse().

>> Source/WebCore/platform/cocoa/RuntimeApplicationChecksCocoa.mm:43
>> +static bool s_isInWebProcess;
> 
> Aren’t these all mutually exclusive? Semantically seems this would be an
enumeration rather than three separate booleans.

Yes, will change to an enum.