Bug 205888

Summary: [iOS] Crash when running WebKit legacy layout tests on device
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebKit Misc.Assignee: Per Arne Vollan <pvollan>
Status: NEW ---    
Severity: Normal CC: ap, bfulgham, sabouhallawa, simon.fraser, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch thorton: review-

Description Per Arne Vollan 2020-01-07 14:38:40 PST 
Thread 0 Crashed:
0   JavaScriptCore                	0x0000000117312128 WTFCrash + 28
1   WebKitLegacy                  	0x000000012bfad6ec WTF::RefCountedBase::applyRefDerefThreadingCheck() const + 168
2   WebKitLegacy                  	0x000000012bfad4c0 WTF::RefCountedBase::derefBase() const + 32
3   WebKitLegacy                  	0x000000012c158684 WTF::RefCounted<LayerFlushController, std::__1::default_delete<LayerFlushController> >::deref() const + 32
4   WebKitLegacy                  	0x000000012c15c270 void WTF::derefIfNotNull<LayerFlushController>(LayerFlushController*) + 52
5   WebKitLegacy                  	0x000000012c16256c WTF::RefPtr<LayerFlushController, WTF::DumbPtrTraits<LayerFlushController> >::~RefPtr() + 48
6   WebKitLegacy                  	0x000000012c1623a8 WTF::RefPtr<LayerFlushController, WTF::DumbPtrTraits<LayerFlushController> >::~RefPtr() + 32
7   WebKitLegacy                  	0x000000012c1f8078 WebViewLayerFlushScheduler::layerFlushCallback() + 136
8   WebKitLegacy                  	0x000000012c1f8ef8 WebViewLayerFlushScheduler::WebViewLayerFlushScheduler(LayerFlushController*)::$_0::operator()() const + 28
9   WebKitLegacy                  	0x000000012c1f8e98 WTF::Detail::CallableWrapper<WebViewLayerFlushScheduler::WebViewLayerFlushScheduler(LayerFlushController*)::$_0, void>::call() + 28
10  WebCore                       	0x000000010394d2c0 WTF::Function<void ()>::operator()() const + 128
11  WebCore                       	0x0000000104de9eac WebCore::RunLoopObserver::runLoopObserverFired() + 124
12  WebCore                       	0x0000000104de9e24 WebCore::RunLoopObserver::runLoopObserverFired(__CFRunLoopObserver*, unsigned long, void*) + 36
13  CoreFoundation                	0x00000001aad43210 __CFRUNLOOP_IS_CALLING_OUT_TO_AN_OBSERVER_CALLBACK_FUNCTION__ + 32
14  CoreFoundation                	0x00000001aad3e104 __CFRunLoopDoObservers + 420
15  CoreFoundation                	0x00000001aad3e580 __CFRunLoopRun + 968
16  CoreFoundation                	0x00000001aad3de8c CFRunLoopRunSpecific + 424
17  DumpRenderTree                	0x000000010083ff74 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) + 2600
18  DumpRenderTree                	0x000000010083f4b0 runTestingServerLoop() + 168
19  DumpRenderTree                	0x000000010083ee54 dumpRenderTree(int, char const**) + 772
20  DumpRenderTree                	0x000000010084078c -[DumpRenderTree _runDumpRenderTree] + 48
21  Foundation                    	0x00000001ab1ac398 __NSThreadPerformPerform + 184
22  CoreFoundation                	0x00000001aad43dbc __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 24
23  CoreFoundation                	0x00000001aad43d14 __CFRunLoopDoSource0 + 80
24  CoreFoundation                	0x00000001aad434f8 __CFRunLoopDoSources0 + 276
25  CoreFoundation                	0x00000001aad3e4cc __CFRunLoopRun + 788
26  CoreFoundation                	0x00000001aad3de8c CFRunLoopRunSpecific + 424
27  GraphicsServices              	0x00000001b53a738c GSEventRunModal + 160
28  UIKitCore                     	0x00000001aee4fbb4 UIApplicationMain + 1932
29  DumpRenderTree                	0x0000000100840cc8 DumpRenderTreeMain(int, char const**) + 148
30  DumpRenderTree                	0x00000001008aecdc main + 36
31  libdyld.dylib                 	0x00000001aabc5810 start + 4
Comment 1 Per Arne Vollan 2020-01-07 14:42:41 PST 
Created attachment 387036 [details]
Patch
Comment 2 Brent Fulgham 2020-01-07 15:04:58 PST 
Comment on attachment 387036 [details]
Patch

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

r=me

> Source/WebKitLegacy/mac/WebView/WebViewData.h:127
> +class LayerFlushController : public ThreadSafeRefCounted<LayerFlushController> {

Whoops!
Comment 3 Brent Fulgham 2020-01-07 15:05:27 PST 
Comment on attachment 387036 [details]
Patch

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

> Source/WebKitLegacy/mac/ChangeLog:9
> +        and another thread.

Is that expected? Should all flushing happen on main thread?
Comment 4 Brent Fulgham 2020-01-07 15:05:45 PST 
Comment on attachment 387036 [details]
Patch

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

>> Source/WebKitLegacy/mac/ChangeLog:9
>> +        and another thread.
> 
> Is that expected? Should all flushing happen on main thread?

You might check with Tim or Simon to make sure this is expected.
Comment 5 Per Arne Vollan 2020-01-07 15:25:12 PST 
(In reply to Brent Fulgham from comment #4)
> Comment on attachment 387036 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387036&action=review
> 
> >> Source/WebKitLegacy/mac/ChangeLog:9
> >> +        and another thread.
> > 
> > Is that expected? Should all flushing happen on main thread?
> 
> You might check with Tim or Simon to make sure this is expected.

Tim, Simon, Said, is this the correct way to fix this issue?
Comment 6 Simon Fraser (smfr) 2020-01-07 15:32:03 PST 
What's the "other" thread? Is it the WebThread?
Comment 7 Simon Fraser (smfr) 2020-01-07 15:46:21 PST 
There are plenty of ref-counted things that are touched from the main thread and the web thread, but WebThreadLock should protect them. Why is LayerFlushScheduler different?
Comment 8 Per Arne Vollan 2020-01-07 15:55:54 PST 
(In reply to Simon Fraser (smfr) from comment #7)
> There are plenty of ref-counted things that are touched from the main thread
> and the web thread, but WebThreadLock should protect them. Why is
> LayerFlushScheduler different?

Ah, so this might not be a real problem if the WebThreadLock is protecting LayerFlushScheduler? At the moment, I am not sure what other thread is accessing the LayerFlushScheduler. It would still be good to fix the assert, though.
Comment 9 Tim Horton 2020-01-07 16:29:15 PST 
isMainThread includes the Web Thread and the main thread, so this means some third thread is doing reffing or dereffing LayerFlushScheduler, which suggests you're papering over the problem.
Comment 10 Tim Horton 2020-01-07 16:30:51 PST 
Actually, suggests it was *created* on a third thread, since the crash is in deref
Comment 11 Per Arne Vollan 2020-01-07 16:31:38 PST 
(In reply to Tim Horton from comment #9)
> isMainThread includes the Web Thread and the main thread, so this means some
> third thread is doing reffing or dereffing LayerFlushScheduler, which
> suggests you're papering over the problem.

That makes sense, I will try to find the third thread.

Thanks for reviewing, all!
Comment 12 Brent Fulgham 2020-01-07 16:33:02 PST 
Comment on attachment 387036 [details]
Patch

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

> Source/WebKitLegacy/mac/WebView/WebViewData.h:129
>      static Ref<LayerFlushController> create(WebView* webView)

Could you add an RELEASE_ASSERT(isMainThread()) here to find the bad guy?
Comment 13 Per Arne Vollan 2020-01-07 16:41:25 PST 
(In reply to Brent Fulgham from comment #12)
> Comment on attachment 387036 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387036&action=review
> 
> > Source/WebKitLegacy/mac/WebView/WebViewData.h:129
> >      static Ref<LayerFlushController> create(WebView* webView)
> 
> Could you add an RELEASE_ASSERT(isMainThread()) here to find the bad guy?

Good point! I will try that.