Add a Debug assert to catch improper use of WebCore::Timer in the UI Process on Apple platforms. <rdar://problem/32816079>
Created attachment 340571 [details] Sanity check v1
(In reply to David Kilzer (:ddkilzer) from comment #1) > Created attachment 340571 [details] > Sanity check v1 This will be split into three separate patches before landing. Just wanted a sanity check from EWS first.
Comment on attachment 340571 [details] Sanity check v1 Attachment 340571 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7713801 New failing tests: http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html
Created attachment 340637 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 341175 [details] Patch v1
Comment on attachment 341175 [details] Patch v1 Attachment 341175 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7786373 New failing tests: http/tests/security/contentSecurityPolicy/video-redirect-blocked.html
Created attachment 341181 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 341175 [details] Patch v1 Attachment 341175 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7786881 New failing tests: http/tests/preload/onload_event.html
Created attachment 341183 [details] Archive of layout-test-results from ews204 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews204 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to Build Bot from comment #8) > Comment on attachment 341175 [details] > Patch v1 > > Attachment 341175 [details] did not pass win-ews (win): > Output: http://webkit-queues.webkit.org/results/7786881 > > New failing tests: > http/tests/preload/onload_event.html There is literally no code that compiles for Windows in this patch. I guess layout tests are really flakey on EWS?
(In reply to David Kilzer (:ddkilzer) from comment #10) > (In reply to Build Bot from comment #8) > > Comment on attachment 341175 [details] > > Patch v1 > > > > Attachment 341175 [details] did not pass win-ews (win): > > Output: http://webkit-queues.webkit.org/results/7786881 > > > > New failing tests: > > http/tests/preload/onload_event.html > > There is literally no code that compiles for Windows in this patch. I guess > layout tests are really flakey on EWS? Definitely.
Comment on attachment 341175 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=341175&action=review > Source/WebCore/platform/Timer.cpp:261 > +bool TimerBase::isAllowedToUseWebCoreTimerInUIProcess() Don’t think you need the “InUIProcess” in this name, since this function actually returns the right value for all processes? > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:175 > + WebCore::setIsUIProcess(); What about WKView clients :P
(In reply to Tim Horton from comment #12) > Comment on attachment 341175 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=341175&action=review > > > Source/WebCore/platform/Timer.cpp:261 > > +bool TimerBase::isAllowedToUseWebCoreTimerInUIProcess() > > Don’t think you need the “InUIProcess” in this name, since this function > actually returns the right value for all processes? > > > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:175 > > + WebCore::setIsUIProcess(); > > What about WKView clients :P Yeah, this seems backwards. What’s known (and small, and entirely contained in the WebKit project) is the set of processes that are allowed to use WebCore::Timer, not the other way around. Those processes’ initialization routines could just call a Timer API that says they are allowed to use Timer.
Created attachment 341541 [details] Patch v2
Comment on attachment 341175 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=341175&action=review >>> Source/WebCore/platform/Timer.cpp:261 >>> +bool TimerBase::isAllowedToUseWebCoreTimerInUIProcess() >> >> Don’t think you need the “InUIProcess” in this name, since this function actually returns the right value for all processes? > > Yeah, this seems backwards. What’s known (and small, and entirely contained in the WebKit project) is the set of processes that are allowed to use WebCore::Timer, not the other way around. Those processes’ initialization routines could just call a Timer API that says they are allowed to use Timer. Thanks! Instead of using on Timer API method that says we're allowed to use WebCore::Timer, I saw an opportunity to remove the bundle ID checks for WebCore::isInWebProcess() in RuntimeApplicationChecksCocoa.mm, so I did that instead, but did it for all three processes.
Comment on attachment 341541 [details] 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.
Comment on attachment 341541 [details] 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.
Comment on attachment 341541 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=341541&action=review >>> 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(). Will change to isAllowed().
Comment on attachment 341541 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=341541&action=review >>> 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. To introduce this on other (non-iOS) platforms, we need to know two things: 1. Is the current process a WK2 XPC process? If yes, then the WebCore::Timer is allowed. 2. Assume this is a UI process because it's not a WK2 XPC process. Is the WebCore::Timer that is being created for WK2 (WKWebView) and not for WK1 (WebView)? If yes, then assert, else the WebCore::Timer is allowed. We only want to catch WebCore::Timers created for WK2 (WKWebView) in a UI process since that's the specific case that causes issues on iOS (see new comment below). For #1, we can use the current check on all platforms. For #2, we assume that the process is a UI process if it's not a WK2 XPC process. The WebCore::Timer check is easy for iOS because a WebCore::Timer for WK1 (UIWebView) will always be created with the WebThreadLock held or on WebThread itself. Any other WebCore::Timer created on iOS would be for WK2 (WKWebView), and thus should assert. For non-IOS platforms, the current thinking is that we need to walk the current stack trace to look for a stack frame from WebKit.framework (or equivalent) to know if the WebCore::Timer is being created for WK2 since there is no WebThread or WebThreadLock. However, is it possible to create a WebCore::Timer for WK2 without WebKit.framework in the stack (due to tail call inlining, or rescheduling a timer in WebCore.framework)? That's the tricky part for non-iOS 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. > > 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. Blargh. I didn't explain this correctly. Let's try again. The goal is that developers should be able to use WK1 (UIWebView) and WK2 (WKWebView) in their iOS apps at the same time. What makes this difficult is that when WebThread starts for WK1 (UIWebView), it forces all WebCore::Timers to be scheduled on the WebThread. That means that WK2 (WKWebView) code that expects WebCore::Timers to run on the main thread now must deal with the timers running on the WebThread, which introduces thread-safety issues and a long tail of crashes. The rule is that WK2 (WKWebView) may never create a WebCore::Timer in the UI Process so that if WebThread is ever started (via UIWebView), no thread safety issues will be introduced. See the new comment above for how we would implement this rule on non-iOS platforms. (Ironically, it's actually "easier" to implement on iOS because the WebThread exists.) >>> 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). The "non-goal" I stated above is incorrect. See the new comment above.
Created attachment 344076 [details] Patch v3
Comment on attachment 344076 [details] Patch v3 Attachment 344076 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8409612 New failing tests: http/tests/security/video-poster-cross-origin-crash2.html
Created attachment 344083 [details] Archive of layout-test-results from ews201 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Committed r233461: <https://trac.webkit.org/changeset/233461>
Filed Bug 187291 to fix Windows build.
Reverted r233461 for reason: Assertions triggered during iOS 11 debug layout and API tests Committed r233466: <https://trac.webkit.org/changeset/233466>
*** Bug 187291 has been marked as a duplicate of this bug. ***
(In reply to Jonathan Bedard from comment #25) > Reverted r233461 for reason: > > Assertions triggered during iOS 11 debug layout and API tests > > Committed r233466: <https://trac.webkit.org/changeset/233466> Why didn't EWS catch this? Does EWS not run layout tests for iOS Simulator on Debug builds? We only run on Release builds?
(In reply to Jonathan Bedard from comment #25) > Reverted r233461 for reason: > > Assertions triggered during iOS 11 debug layout and API tests > > Committed r233466: <https://trac.webkit.org/changeset/233466> Doh! Looks like we need to put the "This is the WebContent process" code earlier in the platform initialization routine. First Debug test run with this assertion: <https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20%28Tests%29/builds/5184> Test results: <https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r233461%20(5184)/results.html> Stack trace: <https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20(Tests)/r233461%20(5184)/accessibility/aria-current-crash-log.txt> ''' No crash log found for com.apple.WebKit.Networking.Development:18346. stdout: stderr: ASSERTION FAILED: WebCore::Timer should not be used in UI Process. false ./platform/Timer.cpp(199) : WebCore::TimerBase::TimerBase() 1 0x11a3c10d9 WTFCrash ASSERTION FAILED: WebCore::Timer should not be used in UI Process. false ./platform/Timer.cpp(199) : WebCore::TimerBase::TimerBase() 1 0x11d3a70d9 WTFCrash 2 0x1206e844b WebCore::TimerBase::TimerBase() 2 0x12342d44b WebCore::TimerBase::TimerBase() 3 0x12223bf50 WebCore::Timer::Timer(WTF::Function<void ()>&&) 4 0x12223b92d WebCore::Timer::Timer(WTF::Function<void ()>&&) 5 0x1237679cc WebCore::NetworkStateNotifier::NetworkStateNotifier() 6 0x123767a75 WebCore::NetworkStateNotifier::NetworkStateNotifier() 7 0x12376a691 WTF::NeverDestroyed<WebCore::NetworkStateNotifier>::NeverDestroyed<>() 8 0x123767955 WTF::NeverDestroyed<WebCore::NetworkStateNotifier>::NeverDestroyed<>() 9 0x123767928 WebCore::NetworkStateNotifier::singleton() 3 0x10e7ace37 WebCore::Timer::Timer<WebKit::WebProcess, WebKit::WebProcess>(WebKit::WebProcess&, void (WebKit::WebProcess::*)()) 4 0x10e796e1d WebCore::Timer::Timer<WebKit::WebProcess, WebKit::WebProcess>(WebKit::WebProcess&, void (WebKit::WebProcess::*)()) 5 0x10e7968e6 WebKit::WebProcess::WebProcess() 6 0x10e796455 WebKit::WebProcess::WebProcess() 7 0x10e79641d WebKit::WebProcess::singleton() 8 0x10e343a5f void WebKit::XPCServiceInitializer<WebKit::WebProcess, WebKit::XPCServiceInitializerDelegate>(WTF::OSObjectPtr<NSObject<OS_xpc_object>*>, NSObject<OS_xpc_object>*, NSObject<OS_xpc_object>*) 9 0x10e3436f6 WebContentServiceInitializer 10 0x10d4961dc invocation function for block in WebKit::XPCServiceEventHandler(NSObject<OS_xpc_object>*) 11 0x1147d96fb _xpc_connection_call_event_handler 12 0x1147da10a _xpc_connection_mach_event 13 0x1143b380d _dispatch_client_callout4 14 0x1143c69ea _dispatch_mach_msg_invoke 15 0x1143baffd _dispatch_queue_serial_drain 16 0x1143c7815 _dispatch_mach_invoke 17 0x1143bd5d4 _dispatch_main_queue_callback_4CF 18 0x112c1dc99 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ 19 0x112be1ea6 __CFRunLoopRun 20 0x112be130b CFRunLoopRunSpecific 21 0x10d541b4a -[NSRunLoop(NSRunLoop) runMode:beforeDate:] 22 0x10d541a25 -[NSRunLoop(NSRunLoop) run] 23 0x1147e08c9 _xpc_objc_main 24 0x1147e2d73 xpc_main 25 0x10d495c0e main 26 0x114428955 start 27 0x1 '''
(In reply to David Kilzer (:ddkilzer) from comment #27) > (In reply to Jonathan Bedard from comment #25) > > Reverted r233461 for reason: > > > > Assertions triggered during iOS 11 debug layout and API tests > > > > Committed r233466: <https://trac.webkit.org/changeset/233466> > > Why didn't EWS catch this? Does EWS not run layout tests for iOS Simulator > on Debug builds? We only run on Release builds? Correct. We run a debug mac configuration.
Created attachment 344217 [details] Patch v4 (check EWS)
Created attachment 344224 [details] Patch v5 (check EWS)
We should catch callOnMainThread and friends too, which have the same exact problem in the same scenarios.