Bug 201303 – [GTK] IconDatabase is not thread-safe

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Charlie Turner]

IconDatabase::writeToDatabase copies it's m_iconsPendingSync values which seem to be created on the main thread into a Vector on its icon-sync thread. IconSnapshot contain a RefPtr<SharedBuffer>, and ref()'ing the SharedBuffer on the main thread and also a background thread is unsafe, and triggers assertions in recent WebKits,

#0  0x00007f4286c380ad in WTFCrash () at /home/ubuntu/webkit/webkit-git/Source/WTF/wtf/Assertions.cpp:305
#1  0x00007f4294382447 in WTF::RefCountedBase::applyRefDerefThreadingCheck (this=0x55d0083284c0) at DerivedSources/ForwardingHeaders/wtf/RefCounted.h:115
#2  0x00007f42943822f2 in WTF::RefCountedBase::ref (this=0x55d0083284c0) at DerivedSources/ForwardingHeaders/wtf/RefCounted.h:43
#3  0x00007f42949ec421 in WTF::refIfNotNull<WebCore::SharedBuffer> (ptr=0x55d0083284c0) at DerivedSources/ForwardingHeaders/wtf/RefPtr.h:38
#4  0x00007f42949e8106 in WTF::RefPtr<WebCore::SharedBuffer, WTF::DumbPtrTraits<WebCore::SharedBuffer> >::RefPtr (this=0x7f421801f9d0, o=...)
    at DerivedSources/ForwardingHeaders/wtf/RefPtr.h:58
#5  0x00007f4294e0b388 in WebKit::IconDatabase::IconSnapshot::IconSnapshot (this=0x7f421801f9c0) at /home/ubuntu/webkit/webkit-git/Source/WebKit/UIProcess/API/glib/IconDatabase.h:61
#6  0x00007f4294e0e05c in WTF::Vector<WebKit::IconDatabase::IconSnapshot, 0ul, WTF::CrashOnOverflow, 16ul>::appendSlowCase<WebKit::IconDatabase::IconSnapshot&> (this=0x7f422e5f79d0, 
    value=...) at DerivedSources/ForwardingHeaders/wtf/Vector.h:1346
#7  0x00007f4294e0b446 in WTF::Vector<WebKit::IconDatabase::IconSnapshot, 0ul, WTF::CrashOnOverflow, 16ul>::append<WebKit::IconDatabase::IconSnapshot&> (this=0x7f422e5f79d0, 
    value=...) at DerivedSources/ForwardingHeaders/wtf/Vector.h:1304
#8  0x00007f4294e074bd in WTF::Vector<WebKit::IconDatabase::IconSnapshot, 0ul, WTF::CrashOnOverflow, 16ul>::appendRange<WTF::HashTableValuesIterator<WTF::HashTable<WTF::String, WTF::KeyValuePair<WTF::String, WebKit::IconDatabase::IconSnapshot>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::String, WebKit::IconDatabase::IconSnapshot> >, WTF::StringHash, WTF::HashMap<WTF::String, WebKit::IconDatabase::IconSnapshot, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WebKit::IconDatabase::IconSnapshot> >::KeyValuePairTraits, WTF::HashTraits<WTF::String> >, WTF::String, WebKit::IconDatabase::IconSnapshot> > (this=0x7f422e5f79d0, start=..., end=...) at DerivedSources/ForwardingHeaders/wtf/Vector.h:1036
#9  0x00007f4294dfeeb5 in WebKit::IconDatabase::writeToDatabase (this=0x55d008157770) at /home/ubuntu/webkit/webkit-git/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:1472
#10 0x00007f4294dfddfa in WebKit::IconDatabase::syncThreadMainLoop (this=0x55d008157770) at /home/ubuntu/webkit/webkit-git/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:1256
#11 0x00007f4294dfc45e in WebKit::IconDatabase::iconDatabaseSyncThread (this=0x55d008157770) at /home/ubuntu/webkit/webkit-git/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:931
#12 0x00007f4294df857f in WebKit::IconDatabase::<lambda()>::operator()(void) const (__closure=0x55d0082b0b18)
    at /home/ubuntu/webkit/webkit-git/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:223
#13 0x00007f4294e04572 in WTF::Detail::CallableWrapper<WebKit::IconDatabase::open(const WTF::String&, const WTF::String&)::<lambda()>, void>::call(void) (this=0x55d0082b0b10)
    at DerivedSources/ForwardingHeaders/wtf/Function.h:52
#14 0x00007f429445a81d in WTF::Function<void ()>::operator()() const (this=0x7f422e5f7c30) at DerivedSources/ForwardingHeaders/wtf/Function.h:79
#15 0x00007f4286c6f3ff in WTF::Thread::entryPoint (newThreadContext=0x55d0082a4480) at /home/ubuntu/webkit/webkit-git/Source/WTF/wtf/Threading.cpp:148
#16 0x00007f4286cee2e5 in WTF::wtfThreadEntryPoint (context=0x55d0082a4480) at /home/ubuntu/webkit/webkit-git/Source/WTF/wtf/posix/ThreadingPOSIX.cpp:200
#17 0x00007f42836036db in start_thread (arg=0x7f422e5f8700) at pthread_create.c:463
#18 0x00007f428332c88f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

[Charlie Turner]

Another issue I am hitting, triggered by new assertions in https://bugs.webkit.org/show_bug.cgi?id=200734

IconDatabase::IconRecord::setImageData is creating a BitmapImage from its sync-thread. This class now has to be created / destroyed on the main thread since internall it uses an ImageSource which has this constraint.

#0  0x00007f568af0b7db in WTFCrash () at /home/cturner/webkit/webkit-git/Source/WTF/wtf/Assertions.cpp:305
#1  0x00007f5698740e6f in CRASH_WITH_INFO(...) () at DerivedSources/ForwardingHeaders/wtf/Assertions.h:658
#2  0x00007f569b926479 in WebCore::ImageSource::ImageSource (this=0x7f561c0204e0, image=0x7f561c01e960, alphaOption=WebCore::AlphaOption::Premultiplied, 
    gammaAndColorProfileOption=WebCore::GammaAndColorProfileOption::Applied) at /home/cturner/webkit/webkit-git/Source/WebCore/platform/graphics/ImageSource.cpp:51
#3  0x00007f569b893936 in WebCore::ImageSource::create (image=0x7f561c01e960, alphaOption=WebCore::AlphaOption::Premultiplied, 
    gammaAndColorProfileOption=WebCore::GammaAndColorProfileOption::Applied) at /home/cturner/webkit/webkit-git/Source/WebCore/platform/graphics/ImageSource.h:50
#4  0x00007f569b885d3e in WebCore::BitmapImage::BitmapImage (this=0x7f561c01e960, observer=0x0) at /home/cturner/webkit/webkit-git/Source/WebCore/platform/graphics/BitmapImage.cpp:46
#5  0x00007f56991cd830 in WebCore::BitmapImage::create (observer=0x0) at DerivedSources/ForwardingHeaders/WebCore/BitmapImage.h:62
#6  0x00007f56991c0b3f in WebKit::IconDatabase::IconRecord::setImageData (this=0x7f561c01d7c0, data=...)
    at /home/cturner/webkit/webkit-git/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:127
#7  0x00007f56991c720c in WebKit::IconDatabase::readFromDatabase (this=0x55e88bc03940) at /home/cturner/webkit/webkit-git/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:1385
#8  0x00007f56991c6965 in WebKit::IconDatabase::syncThreadMainLoop (this=0x55e88bc03940) at /home/cturner/webkit/webkit-git/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:1260
#9  0x00007f56991c4fa6 in WebKit::IconDatabase::iconDatabaseSyncThread (this=0x55e88bc03940) at /home/cturner/webkit/webkit-git/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:931
#10 0x00007f56991c10c7 in WebKit::IconDatabase::<lambda()>::operator()(void) const (__closure=0x55e88bc5ee78)
    at /home/cturner/webkit/webkit-git/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:223
#11 0x00007f56991cd0ba in WTF::Detail::CallableWrapper<WebKit::IconDatabase::open(const WTF::String&, const WTF::String&)::<lambda()>, void>::call(void) (this=0x55e88bc5ee70)
    at DerivedSources/ForwardingHeaders/wtf/Function.h:52
#12 0x00007f5698828399 in WTF::Function<void ()>::operator()() const (this=0x7f56318e3c30) at DerivedSources/ForwardingHeaders/wtf/Function.h:79
#13 0x00007f568af42a7d in WTF::Thread::entryPoint (newThreadContext=0x55e88bc5f120) at /home/cturner/webkit/webkit-git/Source/WTF/wtf/Threading.cpp:148
#14 0x00007f568afc1963 in WTF::wtfThreadEntryPoint (context=0x55e88bc5f120) at /home/cturner/webkit/webkit-git/Source/WTF/wtf/posix/ThreadingPOSIX.cpp:200
#15 0x00007f568789a6db in start_thread () from /lib/x86_64-linux-gnu/libpthread.so.0
#16 0x00007f56875c388f in clone () from /lib/x86_64-linux-gnu/libc.so.6

[Carlos Garcia Campos]

*** Bug 202001 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

Created attachment 379623 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Carlos Garcia Campos]

Created attachment 379624 [details]
Try to fix WPE build

Do not build the favicon database tests in WPE since it's GTK specific

[Adrian Perez]

Nice work removing unneeded code, Carlos!

[Charlie Turner]

Comment on attachment 379624 [details]
Try to fix WPE build

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

> Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:-30
> -#include "Logging.h"

You need to keep this include for the logging calls

[Zan Dobersek]

Comment on attachment 379624 [details]
Try to fix WPE build

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

> Source/WebKit/UIProcess/API/glib/WebKitFaviconDatabase.cpp:181
> +#if 0

This can then be removed?

[Carlos Garcia Campos]

Comment on attachment 379624 [details]
Try to fix WPE build

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

>> Source/WebKit/UIProcess/API/glib/WebKitFaviconDatabase.cpp:181
>> +#if 0
> 
> This can then be removed?

Right! I forgot to remove this.

[Carlos Garcia Campos]

Committed r250518: <https://trac.webkit.org/changeset/250518>