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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for 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 #16 from Darin Adler <darin at apple.com> ---
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

I’m going to say review+ because I think the goal here is likely a critical
one. But the concepts aren’t completely clear to me. I don’t understand the
overlap with all the canAccessThreadLocalDataForThread checks and per-thread
logic that TimerBase already has.

> Source/WebCore/platform/RuntimeApplicationChecks.h:48
> +WEBCORE_EXPORT void setIsInNetworkProcess();
> +bool isInNetworkProcess();
> +WEBCORE_EXPORT void setIsInStorageProcess();
> +bool isInStorageProcess();
> +WEBCORE_EXPORT void setIsInWebProcess();
>  bool isInWebProcess();

These seem like WebKit concepts, not Cocoa concepts. Not great to have this
inside #if PLATFORM(COCOA).

> Source/WebCore/platform/Timer.cpp:196
> +#if PLATFORM(IOS)

I think we should consider compiling this code on all platforms.

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

> Source/WebCore/platform/Timer.cpp:269
> +#if USE(WEB_THREAD)
> +    if (WebThreadIsEnabled() && (WebThreadIsCurrent() ||
WebThreadIsLocked()))
> +	   return true;
> +#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.

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

> Source/WebCore/platform/cocoa/RuntimeApplicationChecksCocoa.mm:43
> +static bool s_isInNetworkProcess;
> +static bool s_isInStorageProcess;
> +static bool s_isInWebProcess;

Aren’t these all mutually exclusive? Semantically seems this would be an
enumeration rather than three separate booleans.