Bug 174760 – [GTK] Icon database error and crash

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Seems the icon database is broken after the recent changes. Every time I start Epiphany I get this lovely warning:

ERROR: Failed to start load for icon at url /favicon.ico
../../Source/WebCore/loader/icon/IconLoader.cpp(96) : void WebCore::IconLoader::startLoading()

Now, right click on any page and click Save As. The UI process will crash. (This might be two different bugs.)

#0  0x00007f6ef51168f9 in WTFCrash ()
    at ../../Source/WTF/wtf/Assertions.cpp:278
No locals.
#1  0x00007f6efcfdb8fc in (anonymous namespace)::TimerBase::stop (
    this=0x7f6ee2a6f108) at ../../Source/WebCore/platform/Timer.cpp:214
        __PRETTY_FUNCTION__ = "void WebCore::TimerBase::stop()"
#2  0x00007f6efcfdb7b2 in (anonymous namespace)::TimerBase::~TimerBase (
    this=0x7f6ee2a6f108, __in_chrg=<optimized out>)
    at ../../Source/WebCore/platform/Timer.cpp:197
No locals.
#3  0x00007f6efb99042c in (anonymous namespace)::Timer::~Timer (
    this=0x7f6ee2a6f108, __in_chrg=<optimized out>)
    at ../../Source/WebCore/platform/Timer.h:116
No locals.
#4  0x00007f6efd0b489c in (anonymous namespace)::Image::~Image (
    this=0x7f6ee2a6f0e8, __in_chrg=<optimized out>)
    at ../../Source/WebCore/platform/graphics/Image.cpp:55
No locals.
#5  0x00007f6efd038382 in (anonymous namespace)::BitmapImage::~BitmapImage (
    this=0x7f6ee2a6f0e8, __in_chrg=<optimized out>)
    at ../../Source/WebCore/platform/graphics/BitmapImage.cpp:61
No locals.
#6  0x00007f6efd03839e in (anonymous namespace)::BitmapImage::~BitmapImage (
    this=0x7f6ee2a6f0e8, __in_chrg=<optimized out>)
    at ../../Source/WebCore/platform/graphics/BitmapImage.cpp:67
No locals.
#7  0x00007f6efba9b1de in WTF::RefCounted<WebCore::Image>::deref (
    this=0x7f6ee2a6f0f0) at ../../Source/WTF/wtf/RefCounted.h:145
No locals.
#8  0x00007f6efba98cbc in WTF::derefIfNotNull<WebCore::Image> (
    ptr=0x7f6ee2a6f0e8) at ../../Source/WTF/wtf/RefPtr.h:45
No locals.
#9  0x00007f6efba967a5 in WTF::RefPtr<WebCore::Image>::~RefPtr (
    this=0x7f6ee2af3888, __in_chrg=<optimized out>)
    at ../../Source/WTF/wtf/RefPtr.h:69
No locals.
#10 0x00007f6efbf334eb in (anonymous namespace)::IconDatabase::IconRecord::~IconRecord (this=0x7f6ee2af3870, __in_chrg=<optimized out>)
    at ../../Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:105
No locals.
#11 0x00007f6efbf41fe9 in WTF::RefCounted<WebKit::IconDatabase::IconRecord>::deref (this=0x7f6ee2af3870) at ../../Source/WTF/wtf/RefCounted.h:145
No locals.
#12 0x00007f6efbf420db in WTF::derefIfNotNull<WebKit::IconDatabase::IconRecord>
    (ptr=0x7f6ee2af3870) at ../../Source/WTF/wtf/RefPtr.h:45
No locals.
#13 0x00007f6efbf404e5 in WTF::RefPtr<WebKit::IconDatabase::IconRecord>::~RefPtr (this=0x7f6ee2a74890, __in_chrg=<optimized out>)
    at ../../Source/WTF/wtf/RefPtr.h:69
No locals.
#14 0x00007f6efbf337f9 in (anonymous namespace)::IconDatabase::PageURLRecord::~PageURLRecord (this=0x7f6ee2a74888, __in_chrg=<optimized out>)
    at ../../Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:164
No locals.
#15 0x00007f6efbf352ba in (anonymous namespace)::IconDatabase::performReleaseIconForPageURL (this=0x7f6ee2acc000, pageURLOriginal=..., releaseCount=1)
    at ../../Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:536
        __PRETTY_FUNCTION__ = "void WebKit::IconDatabase::performReleaseIconForPageURL(const WTF::String&, int)"
        pageRecord = 0x7f6ee2a74888
        iconRecord = 0x7f6ee2af3870
#16 0x00007f6efbf39451 in (anonymous namespace)::IconDatabase::performPendingRetainAndReleaseOperations (this=0x7f6ee2acc000)
    at ../../Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:1369
        entry = @0x7f6ee2aa4690: {key = {m_impl = {
              static isRefPtr = <optimized out>, m_ptr = 0x7f6ee2afc780}}, 
          value = 1}
        __for_range = @0x7f6e92a9e920: {m_impl = {m_impl = {
              static m_maxLoad = 2, static m_minLoad = 6, 
              m_table = 0x7f6ee2aa4680, m_tableSize = 8, m_tableSizeMask = 7, 
              m_keyCount = 1, m_deletedCount = 0, 
              m_iterators = 0x7f6e92a9e8e8, 
              m_mutex = std::unique_ptr<WTF::Lock> containing 0x7f6ee2aba088}}}
        __for_begin = {<std::iterator<std::forward_iterator_tag, WTF::KeyValuePair<WTF::String, unsigned int>, long, WTF::KeyValuePair<WTF::String, unsigned int>*, WTF::KeyValuePair<WTF::String, unsigned int>&>> = {<No data fields>}, 
          m_impl = {<std::iterator<std::forward_iterator_tag, WTF::KeyValuePair<WTF::String, unsigned int>, long, WTF::KeyValuePair<WTF::String, unsigned int>*, WTF::KeyValuePair<WTF::String, unsigned int>&>> = {<No data fields>}, 
            m_iterator = {<std::iterator<std::forward_iterator_tag, WTF::KeyValuePair<WTF::String, unsigned int>, long, WTF::KeyValuePair<WTF::String, unsigned int> const*, WTF::KeyValuePair<WTF::String, unsigned int> const&>> = {<No data fields>}, m_position = 0x7f6ee2aa4690, m_endPosition = 0x7f6ee2aa4700, 
              m_table = 0x7f6e92a9e920, m_next = 0x0, 
              m_previous = 0x7f6e92a9e8e8}}}
        __for_end = {<std::iterator<std::forward_iterator_tag, WTF::KeyValuePair<WTF::String, unsigned int>, long, WTF::KeyValuePair<WTF::String, unsigned int>*, WTF::KeyValuePair<WTF::String, unsigned int>&>> = {<No data fields>}, 
          m_impl = {<std::iterator<std::forward_iterator_tag, WTF::KeyValuePair<WTF::String, unsigned int>, long, WTF::KeyValuePair<WTF::String, unsigned int>*, WTF::KeyValuePair<WTF::String, unsigned int>&>> = {<No data fields>}, 
            m_iterator = {<std::iterator<std::forward_iterator_tag, WTF::KeyValuePair<WTF::String, unsigned int>, long, WTF::KeyValuePair<WTF::String, unsigned int> const*, WTF::KeyValuePair<WTF::String, unsigned int> const&>> = {<No data fields>}, m_position = 0x7f6ee2aa4700, m_endPosition = 0x7f6ee2aa4700, 
              m_table = 0x7f6e92a9e920, m_next = 0x7f6e92a9e8b8, 
              m_previous = 0x0}}}
        __PRETTY_FUNCTION__ = "void WebKit::IconDatabase::performPendingRetainAndReleaseOperations()"
        toRetain = {m_impl = {m_impl = {static m_maxLoad = 2, 
              static m_minLoad = 6, m_table = 0x0, m_tableSize = 0, 
              m_tableSizeMask = 0, m_keyCount = 0, m_deletedCount = 0, 
              m_iterators = 0x0, 
              m_mutex = std::unique_ptr<WTF::Lock> containing 0x7f6ee2aba080}}}
        toRelease = {m_impl = {m_impl = {static m_maxLoad = 2, 
              static m_minLoad = 6, m_table = 0x7f6ee2aa4680, m_tableSize = 8, 
              m_tableSizeMask = 7, m_keyCount = 1, m_deletedCount = 0, 
              m_iterators = 0x7f6e92a9e8e8, 
              m_mutex = std::unique_ptr<WTF::Lock> containing 0x7f6ee2aba088}}}
#17 0x00007f6efbf38f3a in (anonymous namespace)::IconDatabase::syncThreadMainLoop (this=0x7f6ee2acc000)
    at ../../Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:1276
        locker = {<WTF::AbstractLocker> = {<No data fields>}, 
          m_lockable = 0x7f6ee2acc064}
        timeStamp = 3175.4553249999999
        didAnyWork = false
        newstamp = 3170.4540929999998
        __PRETTY_FUNCTION__ = "void WebKit::IconDatabase::syncThreadMainLoop()"
#18 0x00007f6efbf37681 in (anonymous namespace)::IconDatabase::iconDatabaseSyncThread (this=0x7f6ee2acc000)
    at ../../Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:953
        __PRETTY_FUNCTION__ = "void WebKit::IconDatabase::iconDatabaseSyncThread()"
        startTime = 3006.9423579999998
        journalFilename = {m_impl = {static isRefPtr = <optimized out>, 
            m_ptr = 0x7f6ee2aca000}}
        timeStamp = 3006.9435509999998
        newStamp = 3006.9436879999998
#19 0x00007f6efbf33a59 in (anonymous namespace)::IconDatabase::<lambda()>::operator()(void) const (__closure=0x7f6ee2af8098)
    at ../../Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:224
        this = 0x7f6ee2acc000
#20 0x00007f6efbf3f6c8 in WTF::Function<void()>::CallableWrapper<WebKit::IconDatabase::open(const WTF::String&, const WTF::String&)::<lambda()> >::call(void) (
    this=0x7f6ee2af8090) at ../../Source/WTF/wtf/Function.h:102
No locals.
#21 0x00007f6efb94cb2b in WTF::Function<void()>::operator()(void) const (
    this=0x7f6e92a9eb20) at ../../Source/WTF/wtf/Function.h:56
No locals.
#22 0x00007f6ef51383de in WTF::threadEntryPoint (contextData=0x7f6ee2af44d0)
    at ../../Source/WTF/wtf/Threading.cpp:101
        context = 0x7f6ee2af44d0
        entryPoint = {
          m_callableWrapper = std::unique_ptr<WTF::Function<void()>::CallableWrapperBase> containing 0x7f6ee2af8090}
#23 0x00007f6ef5178314 in WTF::wtfThreadEntryPoint (param=0x7f6ee2afe0c0)
    at ../../Source/WTF/wtf/ThreadingPthreads.cpp:209
        invocation = std::unique_ptr<WTF::ThreadFunctionInvocation> containing 0x7f6ee2afe0c0
#24 0x00007f6ef105d36d in start_thread (arg=0x7f6e92a9f700)
    at pthread_create.c:456
        __res = <optimized out>
        pd = 0x7f6e92a9f700
        now = <optimized out>
        unwind_buf = {cancel_jmp_buf = {{jmp_buf = {140112883742464, 
                4941255678535664290, 0, 140733047339136, 140112883743168, 0, 
                -5021521763199090014, -5021724863860047198}, 
              mask_was_saved = 0}}, priv = {pad = {0x0, 0x0, 0x0, 0x0}, 
            data = {prev = 0x0, cleanup = 0x0, canceltype = 0}}}
        not_first_call = <optimized out>
        pagesize_m1 = <optimized out>
        sp = <optimized out>
        freesize = <optimized out>
        __PRETTY_FUNCTION__ = "start_thread"
#25 0x00007f6f03b59b8f in clone ()
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S:97
No locals.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> Now, right click on any page and click Save As. The UI process will crash.
> (This might be two different bugs.)

Actually you don't need to do anything. Just load any page. It will hit that crash.

[Brady Eidson]

The assertion in the ~TimerBase destructor is:
    ASSERT(canAccessThreadLocalDataForThread(m_thread));

So a Timer is getting created on one thread but destroyed on another thread.

It's not immediately clear to me how my recent changes changed this.

[Brady Eidson]

That said, this is the sync thread, and it's super surprising to see IconRecords destroyed (and therefore WebCore::Images destroyed) on a background thread.

That's definitely not okay.

[Carlos Garcia Campos]

I think this problem has always existed, but before removing the code, history items were retaining icons preventing them from being deleted in some cases. IconRecord and PageURLRecord objects can be created and destroyed either in sync or main thread. The problem seems to be the frame timer of the BitmapImage, that is not even used nor needed at all. We currently retain icons when they are returned by webkit_favicon_database_get_favicon(), but we release them if we fail to get them. I guess this crash happens for a page that doesn't have a favicon. I don't think it's worth trying to fix the IconDatabase mess, because we will eventually replace it, hopefully during the next cycle. So, for now, I think we can simple change IconRecord::setImageData() to not create the image if the given data is nullptr.

[Carlos Garcia Campos]

Created attachment 316273 [details]
Patch

Could you try this patch, Michael?

[Build Bot]

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

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #4)
> I don't think it's worth trying to fix the IconDatabase mess, because we will
> eventually replace it, hopefully during the next cycle.

I'd be interested in seeing a plan for this.

[Michael Catanzaro]

Comment on attachment 316273 [details]
Patch

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

I'm not sure I understand this fix. Is your goal to prevent the crash by ensuring that IconDatabase::performReleaseIconForPageURL is never called ever? That is quite a hack, but I think that's fine as you say you're planning to delete all this code, and I'm confident that will happen sooner rather than later. The IconDatabase has always been a mess anyway.

I will test the change to see if it works... building now.

> Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:123
>      // It's okay to delete the raw image here. Any existing clients using this icon will be
>      // managing an image that was created with a copy of this raw image data.

Um... I'm not sure if this comment is really supposed to be in this function at all... but I would have added your new conditional above it rather than below it.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #7)
> (In reply to Carlos Garcia Campos from comment #4)
> > I don't think it's worth trying to fix the IconDatabase mess, because we will
> > eventually replace it, hopefully during the next cycle.
> 
> I'd be interested in seeing a plan for this.

The plan is to use a new database format that supports multiple icons and sizes per page URL, and modernize and simplify the code.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #8)
> Comment on attachment 316273 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316273&action=review
> 
> I'm not sure I understand this fix. Is your goal to prevent the crash by
> ensuring that IconDatabase::performReleaseIconForPageURL is never called
> ever?

No, but when this is called no Image will be deleted.

> That is quite a hack, but I think that's fine as you say you're
> planning to delete all this code, 

It's a side effect, but the change is good in any case, it avoids creating an image for nothing when the page doesn't have a favicon.

> and I'm confident that will happen sooner
> rather than later.

Not sure how soon it will be, but definitely not in this release cycle.

> The IconDatabase has always been a mess anyway.

Indeed.

> I will test the change to see if it works... building now.

Thanks! I'll make a release tomorrow.

> > Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:123
> >      // It's okay to delete the raw image here. Any existing clients using this icon will be
> >      // managing an image that was created with a copy of this raw image data.
> 
> Um... I'm not sure if this comment is really supposed to be in this function
> at all... but I would have added your new conditional above it rather than
> below it.

No, the data is removed when the image is destroyed, so setting it to nullptr would destroy any previous data.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #8)
> I will test the change to see if it works... building now.

Your patch fixes the crash I posted above.

It does not fix this error:

ERROR: Failed to start load for icon at url /favicon.ico
../../Source/WebCore/loader/icon/IconLoader.cpp(82) : void WebCore::IconLoader::startLoading()

I will file a new bug for that.

There is also an icon database crash on exit. I will file a new bug for that, too.

[Carlos Garcia Campos]

Committed r219861: <http://trac.webkit.org/changeset/219861>