Bug 208784

Summary: REGRESSION (r258064): API tests intermittently crashing under WebKit::registerDefaultsOverride
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: New BugsAssignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, darin, pvollan, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=208690
Attachments:
Description Flags
crash log
none
Patch
darin: review+
Patch none

Description Ryan Haddad 2020-03-08 09:42:56 PDT 
Created attachment 392957 [details]
crash log

As seen here, API tests have been intermittently crashing on macOS Debug queues after r258064:
https://results.webkit.org/suites?platform=mac&style=debug&suite=api-tests

It seems to frequently be different tests, I don't see a theme. Here are the ones that crashed on a couple of different runs:

    TestWebKitAPI.SOAuthorizationRedirect.NSNotificationCenter
    TestWebKitAPI.WebKit.QuotaDelegateReload
    TestWebKitAPI.ServiceWorkers.ThrottleCrash
    TestWebKitAPI.ServiceWorkers.DifferentSessionsUseDifferentRegistrations
    TestWebKitAPI.DownloadProgress.PublishProgressOnPartialDownload
    TestWebKitAPI.WebKit.UploadDirectory
    TestWebKitAPI.WKAttachmentTests.AttachmentUpdatesWhenUndoingAndRedoing


Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   ???                           	000000000000000000 0 + 0
1   com.apple.WebKit              	0x0000000111f41e5a void wtfCallIMP<void, NSDictionary<NSString*, objc_object*>*>(void (*)(), objc_object*, objc_selector*, NSDictionary<NSString*, objc_object*>*) + 42 (ObjCRuntimeExtras.h:40)
2   com.apple.WebKit              	0x0000000111f41983 registerDefaultsOverride(objc_object*, objc_selector*, NSDictionary<NSString*, objc_object*>*) + 51 (PreferenceObserver.mm:40)
3   com.apple.AppKit              	0x00007fff33327db6 ___NSWindowFetchCoreUIShadowMeasurements_block_invoke + 6619
4   libdispatch.dylib             	0x00007fff6d64550e _dispatch_client_callout + 8
5   libdispatch.dylib             	0x00007fff6d646686 _dispatch_once_callout + 20
6   com.apple.AppKit              	0x00007fff3332625a _NSWindowConstructShadowParameters + 1987
7   com.apple.AppKit              	0x00007fff33325a45 -[NSWindow shadowParameters] + 71
8   com.apple.AppKit              	0x00007fff3332596a -[NSWindow _setShadowParameters] + 50
9   com.apple.AppKit              	0x00007fff3331ef18 -[NSWindow _reallyDoOrderWindowAboveOrBelow:relativeTo:findKey:forCounter:force:isModal:] + 1576
10  com.apple.AppKit              	0x00007fff3331e599 -[NSWindow _reallyDoOrderWindow:relativeTo:findKey:forCounter:force:isModal:] + 135
11  com.apple.AppKit              	0x00007fff3331d505 -[NSWindow _doOrderWindow:relativeTo:findKey:forCounter:force:isModal:] + 283
12  com.apple.AppKit              	0x00007fff3331d389 -[NSWindow orderWindow:relativeTo:] + 155
13  com.apple.AppKit              	0x00007fff3330f8ed -[NSWindow makeKeyAndOrderFront:] + 60
14  TestWebKitAPI                 	0x000000010763ef54 -[TestWKWebView _setUpTestWindow:] + 388
15  TestWebKitAPI                 	0x000000010763ecd4 -[TestWKWebView initWithFrame:configuration:addToWindow:] + 292 (TestWKWebView.mm:443)
16  TestWebKitAPI                 	0x000000010763eb9d -[TestWKWebView initWithFrame:configuration:] + 125 (TestWKWebView.mm:416)
17  TestWebKitAPI                 	0x000000010763eb14 -[TestWKWebView initWithFrame:] + 148 (TestWKWebView.mm:411)
18  TestWebKitAPI                 	0x000000010714e38a WKWebView_FindAPIBackwardsNoWrap_Test::TestBody() + 154 (FindInPageAPI.mm:175)
19  TestWebKitAPI                 	0x000000010785e57e void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) + 126 (gtest.cc:2443)
20  TestWebKitAPI                 	0x000000010782c5cb void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) + 123 (gtest.cc:2479)
21  TestWebKitAPI                 	0x000000010782c4f6 testing::Test::Run() + 198 (gtest.cc:2524)
Comment 1 Radar WebKit Bug Importer 2020-03-08 09:43:27 PDT 
<rdar://problem/60201631>
Comment 2 Per Arne Vollan 2020-03-08 12:31:52 PDT 
Looking into this now.
Comment 3 Per Arne Vollan 2020-03-08 12:40:35 PDT 
Created attachment 392976 [details]
Patch
Comment 4 Darin Adler 2020-03-08 13:01:26 PDT 
Comment on attachment 392976 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:107
>      registerDefaultsOriginal = method_setImplementation(registerDefaultsMethod, (IMP)registerDefaultsOverride);

Is this early enough? Seems like there is a race here if registerDefaults is being called on another thread. Doing it on the main thread is good, but also it needs to be really early, before other threads exist.
Comment 5 Per Arne Vollan 2020-03-08 15:28:51 PDT 
Created attachment 392993 [details]
Patch
Comment 6 Per Arne Vollan 2020-03-08 15:33:35 PDT 
(In reply to Darin Adler from comment #4)
> Comment on attachment 392976 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392976&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:107
> >      registerDefaultsOriginal = method_setImplementation(registerDefaultsMethod, (IMP)registerDefaultsOverride);
> 
> Is this early enough? Seems like there is a race here if registerDefaults is
> being called on another thread. Doing it on the main thread is good, but
> also it needs to be really early, before other threads exist.

Yes, this should be early enough. As long as we do it before we start observing preference changes, we should be fine!

Thanks for reviewing!
Comment 7 Per Arne Vollan 2020-03-08 16:24:13 PDT 
Committed r258119: <https://trac.webkit.org/changeset/258119/webkit>.