Bug 201303 - [GTK] IconDatabase is not thread-safe
Summary: [GTK] IconDatabase is not thread-safe
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 202001 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-08-29 09:04 PDT by Charlie Turner
Modified: 2019-09-30 05:38 PDT (History)
13 users (show)

See Also:


Attachments
Patch (167.35 KB, patch)
2019-09-26 00:51 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix WPE build (169.26 KB, patch)
2019-09-26 01:04 PDT, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charlie Turner 2019-08-29 09:04:22 PDT
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
Comment 1 Charlie Turner 2019-09-11 04:00:49 PDT
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
Comment 2 Carlos Garcia Campos 2019-09-20 01:09:27 PDT
*** Bug 202001 has been marked as a duplicate of this bug. ***
Comment 3 Carlos Garcia Campos 2019-09-26 00:51:50 PDT
Created attachment 379623 [details]
Patch
Comment 4 EWS Watchlist 2019-09-26 00:52:37 PDT
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
Comment 5 Carlos Garcia Campos 2019-09-26 01:04:38 PDT
Created attachment 379624 [details]
Try to fix WPE build

Do not build the favicon database tests in WPE since it's GTK specific
Comment 6 Adrian Perez 2019-09-26 02:04:47 PDT
Nice work removing unneeded code, Carlos!
Comment 7 Charlie Turner 2019-09-26 04:58:16 PDT
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
Comment 8 Zan Dobersek 2019-09-30 05:16:24 PDT
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?
Comment 9 Carlos Garcia Campos 2019-09-30 05:35:42 PDT
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.
Comment 10 Carlos Garcia Campos 2019-09-30 05:38:52 PDT
Committed r250518: <https://trac.webkit.org/changeset/250518>