Bug 197333 – [WKTR] Move test timeout handling to the UIProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Dumez]

Move test timeout handling in WebKitTestRunner to the UIProcess to play nicely with PSON. Previous, we'd start the timeout timer in the InjectedBundle, which would fail to account of the time spent in every WebContent process in the case of swapping.
Also, because of process caching, the timeout timer would sometime fire in a cached process and it would lead to crashes like so:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000740b0269e WTFCrash + 14 (Assertions.cpp:305)
1   com.apple.WebKitTestRunner.InjectedBundle	0x00000001b629acd9 WTF::CrashOnOverflow::crash() + 9
2   com.apple.WebKitTestRunner.InjectedBundle	0x00000001b629aca9 WTF::CrashOnOverflow::overflowed() + 9
3   com.apple.WebKitTestRunner.InjectedBundle	0x00000001b62ad16b WTF::Vector<std::__1::unique_ptr<WTR::InjectedBundlePage, std::__1::default_delete<WTR::InjectedBundlePage> >, 0ul, WTF::CrashOnOverflow, 16ul>::at(unsigned long) const + 75
4   com.apple.WebKitTestRunner.InjectedBundle	0x00000001b62a79dd WTF::Vector<std::__1::unique_ptr<WTR::InjectedBundlePage, std::__1::default_delete<WTR::InjectedBundlePage> >, 0ul, WTF::CrashOnOverflow, 16ul>::operator[](unsigned long) const + 29 (Vector.h:720)
5   com.apple.WebKitTestRunner.InjectedBundle	0x00000001b62a79ad WTR::InjectedBundle::page() const + 29 (InjectedBundle.cpp:168)
6   com.apple.WebKitTestRunner.InjectedBundle	0x00000001b62a80a6 WTR::InjectedBundle::outputText(WTF::String const&) + 150 (InjectedBundle.cpp:617)
7   com.apple.WebKitTestRunner.InjectedBundle	0x00000001b62d9158 WTR::TestRunner::waitToDumpWatchdogTimerFired() + 184 (TestRunner.cpp:192)
8   com.apple.WebKitTestRunner.InjectedBundle	0x00000001b62f1a55 WTR::waitUntilDoneWatchdogTimerFired(__CFRunLoopTimer*, void*) + 37 (TestRunnerMac.mm:51)
9   com.apple.CoreFoundation      	0x00007fff44e8a9e5 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
10  com.apple.CoreFoundation      	0x00007fff44e8a59f __CFRunLoopDoTimer + 859
11  com.apple.CoreFoundation      	0x00007fff44e89fc5 __CFRunLoopDoTimers + 317
12  com.apple.CoreFoundation      	0x00007fff44e6b2bb __CFRunLoopRun + 2213
13  com.apple.CoreFoundation      	0x00007fff44e6a791 CFRunLoopRunSpecific + 499
14  com.apple.Foundation          	0x00007fff474b7fa3 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 212
15  com.apple.Foundation          	0x00007fff474b7ebc -[NSRunLoop(NSRunLoop) run] + 76
16  libxpc.dylib                  	0x00007fff7b677bca _xpc_objc_main.cold.4 + 38
17  libxpc.dylib                  	0x00007fff7b66abb9 _xpc_objc_main + 470
18  libxpc.dylib                  	0x00007fff7b66a72d xpc_main + 377
19  com.apple.WebKit              	0x00000007206add2a WebKit::XPCServiceMain(int, char const**) + 1322 (XPCServiceMain.mm:147)
20  com.apple.WebKit              	0x00000007204e3ecb WKXPCServiceMain + 27 (WKMain.mm:34)
21  com.apple.WebKit.WebContent   	0x000000010658defe main + 94 (AuxiliaryProcessMain.cpp:32)
22  libdyld.dylib                 	0x00007fff7b4201b1 start + 1

[Chris Dumez]

Created attachment 368366 [details]
Patch

[Chris Dumez]

Created attachment 368367 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 368367 [details]
Patch

Clearing flags on attachment: 368367

Committed r244723: <https://trac.webkit.org/changeset/244723>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50273711>

[Jonathan Bedard]

I think with PSON changes, something like this makes sense. It's probably a performance win for layout tests, but I don't have the numbers to back up that claim.

Although, I must question what made you think to do this, because I would have expected that the timeouts a level up (in run-webkit-tests) would have caught most of these cases.

[Chris Dumez]

(In reply to Jonathan Bedard from comment #6)
> I think with PSON changes, something like this makes sense. It's probably a
> performance win for layout tests, but I don't have the numbers to back up
> that claim.
> 
> Although, I must question what made you think to do this, because I would
> have expected that the timeouts a level up (in run-webkit-tests) would have
> caught most of these cases.

The crashes above when I run the layout tests locally is what pushed to me fix this.