Bug 201712 – [iOS] ASSERTION FAILED: Unsafe to ref/deref of ShareableBitmap from different threads

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ryan Haddad]

The following assertion failure was seen on the iOS Simulator debug bot with layout tests css3/filters/blur-clipped-by-ancestor.html and fast/writing-mode/english-bt-text-with-spelling-marker.html


ASSERTION FAILED: Unsafe to ref/deref from different threads
m_isOwnedByMainThread == isMainThread()
/Volumes/Data/slave/ios-simulator-12-debug/build/WebKitBuild/Debug-iphonesimulator/usr/local/include/wtf/RefCounted.h(115) : void WTF::RefCountedBase::applyRefDerefThreadingCheck() const
1   0x5369db4a9 WTFCrash
2   0x10c22fcda WTF::RefCountedBase::applyRefDerefThreadingCheck() const
3   0x10c22faa9 WTF::RefCountedBase::derefBase() const
4   0x10c2f880f WTF::RefCounted<WebKit::ShareableBitmap, std::__1::default_delete<WebKit::ShareableBitmap> >::deref() const
5   0x10ca89802 WebKit::ShareableBitmap::releaseBitmapContextData(void*, void*)
6   0x5326f70da CGBitmapContextInfoRelease
7   0x5326f95cb context_reclaim
8   0x1160f123e _CFRelease
9   0x10c480a19 WTF::RetainPtr<CGContext*>::~RetainPtr()
10  0x10c477865 WTF::RetainPtr<CGContext*>::~RetainPtr()
11  0x10c48089f WTF::VectorDestructor<true, WTF::RetainPtr<CGContext*> >::destruct(WTF::RetainPtr<CGContext*>*, WTF::RetainPtr<CGContext*>*)
12  0x10c4807dd WTF::VectorTypeOperations<WTF::RetainPtr<CGContext*> >::destruct(WTF::RetainPtr<CGContext*>*, WTF::RetainPtr<CGContext*>*)
13  0x10c48075f WTF::Vector<WTF::RetainPtr<CGContext*>, 0ul, WTF::CrashOnOverflow, 16ul>::~Vector()
14  0x10c477ac5 WTF::Vector<WTF::RetainPtr<CGContext*>, 0ul, WTF::CrashOnOverflow, 16ul>::~Vector()
15  0x10c47d643 WebKit::RemoteLayerTreeDrawingArea::BackingStoreFlusher::~BackingStoreFlusher()
16  0x10c47d615 WebKit::RemoteLayerTreeDrawingArea::BackingStoreFlusher::~BackingStoreFlusher()
17  0x10c47d5ea WTF::ThreadSafeRefCounted<WebKit::RemoteLayerTreeDrawingArea::BackingStoreFlusher, (WTF::DestructionThread)0>::deref() const::'lambda'()::operator()() const
18  0x10c47d5ad WTF::ThreadSafeRefCounted<WebKit::RemoteLayerTreeDrawingArea::BackingStoreFlusher, (WTF::DestructionThread)0>::deref() const
19  0x10c47d531 void WTF::derefIfNotNull<WebKit::RemoteLayerTreeDrawingArea::BackingStoreFlusher>(WebKit::RemoteLayerTreeDrawingArea::BackingStoreFlusher*)
20  0x10c47d4f9 WTF::RefPtr<WebKit::RemoteLayerTreeDrawingArea::BackingStoreFlusher, WTF::DumbPtrTraits<WebKit::RemoteLayerTreeDrawingArea::BackingStoreFlusher> >::~RefPtr()
21  0x10c475215 WTF::RefPtr<WebKit::RemoteLayerTreeDrawingArea::BackingStoreFlusher, WTF::DumbPtrTraits<WebKit::RemoteLayerTreeDrawingArea::BackingStoreFlusher> >::~RefPtr()
22  0x10c47ac95 WebKit::RemoteLayerTreeDrawingArea::flushLayers()::$_2::~$_2()
23  0x10c477cc5 WebKit::RemoteLayerTreeDrawingArea::flushLayers()::$_2::~$_2()
24  0x10c477ca9 __destroy_helper_block_e8_32c62_ZTSKZN6WebKit26RemoteLayerTreeDrawingArea11flushLayersEvE3$_2
25  0x53101e9cd _Block_release
26  0x530f2ad02 _dispatch_client_callout
27  0x530f31720 _dispatch_lane_serial_drain
28  0x530f32261 _dispatch_lane_invoke
29  0x530f3afcb _dispatch_workloop_worker_thread
30  0x53130c611 _pthread_wqthread
31  0x53130c3fd start_wqthread

https://build.webkit.org/results/Apple%20iOS%2012%20Simulator%20Debug%20WK2%20(Tests)/r249790%20(5549)/results.html

[Radar WebKit Bug Importer]

<rdar://problem/55289916>

[Ryan Haddad]

Same assertion seen attributed to different tests here, so I'm not sure which one is actually responsible:
https://build.webkit.org/results/Apple%20iOS%2012%20Simulator%20Debug%20WK2%20(Tests)/r249788%20(5548)/results.html

[Ryan Haddad]

Lots of tests hit this here on iOS 13:
https://build.webkit.org/results/Apple%20iOS%2013%20Simulator%20Debug%20WK2%20(Tests)/r250049%20(20)/results.html

We'll need to find repro steps for this.

[Chris Dumez]

Created attachment 379081 [details]
Patch

[Geoffrey Garen]

Comment on attachment 379081 [details]
Patch

r=me

[Geoffrey Garen]

Comment on attachment 379081 [details]
Patch

Actually, I don't think this fix is complete.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #6)
> Comment on attachment 379081 [details]
> Patch
> 
> Actually, I don't think this fix is complete.

Ok, I will take a deeper look tomorrow. I assume the issue is with the SharedMemory data member (given that SharedMemory is not ThreadSafeRefCounted). Or did you identify another issue?

[Geoffrey Garen]

If ShareableBitmap::releaseDataProviderData derefs typelessData to the point of deletion, then typelessData will deref m_sharedMemory, which is a RefPtr.

Mechanically, that deref is not thread-safe.

Possibly, if we think hard enough, we can convince ourselves that the deref is not observable in practice, and m_sharedMemory could have been a unique_ptr instead. But it feels fishy. We'd really like pointers to be safe at a glance, without deeper thought.

I think what I would prefer is either:

(a) Make ShareableBitmap::releaseDataProviderData forward to the main thread. I think that might be what the author expected in the first place.

OR

(b) Change m_sharedMemory to unique_ptr or ThreadSafeRefCounted.

[Tim Horton]

(a) makes sense to me

[Chris Dumez]

(In reply to Tim Horton from comment #9)
> (a) makes sense to me

I agree that a) makes sense but this is assuming ShareableBitmap objects are ALWAYS constructed on the main thread.

[Simon Fraser (smfr)]

Why doesn't the bug title have "ShareableBitmap" in it?

[Chris Dumez]

Created attachment 379136 [details]
Patch

[Chris Dumez]

No takers?

[WebKit Commit Bot]

Comment on attachment 379136 [details]
Patch

Clearing flags on attachment: 379136

Committed r250151: <https://trac.webkit.org/changeset/250151>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.