Changeset 220580 in webkit


Ignore:
Timestamp:
Aug 10, 2017 11:01:15 PM (7 years ago)
Author:
Carlos Garcia Campos
Message:

[GTK][WPE] Assertion failure in TimerBase inside WebCore::IconRecord::setImageData
​https://bugs.webkit.org/show_bug.cgi?id=173866
<rdar://problem/33122050>

Reviewed by Michael Catanzaro.

IconDatabase creates and destroys IconRecord objects in both database and main thread. If the IconRecord has a
valid icon, its Image could be created in one thread and destroyed in another, something that is not expected to
happen, because Image has a Timer member. Since we have all the data and we are decoding it right after creating
the Image, we don't really need to keep the Image object around, we could simply take a reference of the encoded
data and the decoded native image to be returned by synchronousIconForPageURL().

  • UIProcess/API/glib/IconDatabase.cpp:

(WebKit::IconDatabase::IconRecord::image): Return NativeImagePtr now.
(WebKit::IconDatabase::IconRecord::setImageData): Create a BitmapImage to decode it and keep a reference to the
encoded data and decoded native image.
(WebKit::IconDatabase::IconRecord::snapshot const): Use m_imageData to get the encoded data.
(WebKit::IconDatabase::synchronousIconForPageURL): Return the native image and whether the icon is known or not.
(WebKit::IconDatabase::IconRecord::loadImageFromResource): Deleted.

  • UIProcess/API/glib/IconDatabase.h:
  • UIProcess/API/glib/WebKitFaviconDatabase.cpp:

(getIconSurfaceSynchronously): Use new API.

Location:
trunk/Source/WebKit
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r220578 r220580  
     12017-08-10  Carlos Garcia Campos  <cgarcia@igalia.com>
     2
     3        [GTK][WPE] Assertion failure in TimerBase inside WebCore::IconRecord::setImageData
     4        https://bugs.webkit.org/show_bug.cgi?id=173866
     5        <rdar://problem/33122050>
     6
     7        Reviewed by Michael Catanzaro.
     8
     9        IconDatabase creates and destroys IconRecord objects in both database and main thread. If the IconRecord has a
     10        valid icon, its Image could be created in one thread and destroyed in another, something that is not expected to
     11        happen, because Image has a Timer member. Since we have all the data and we are decoding it right after creating
     12        the Image, we don't really need to keep the Image object around, we could simply take a reference of the encoded
     13        data and the decoded native image to be returned by synchronousIconForPageURL().
     14
     15        * UIProcess/API/glib/IconDatabase.cpp:
     16        (WebKit::IconDatabase::IconRecord::image): Return NativeImagePtr now.
     17        (WebKit::IconDatabase::IconRecord::setImageData): Create a BitmapImage to decode it and keep a reference to the
     18        encoded data and decoded native image.
     19        (WebKit::IconDatabase::IconRecord::snapshot const): Use m_imageData to get the encoded data.
     20        (WebKit::IconDatabase::synchronousIconForPageURL): Return the native image and whether the icon is known or not.
     21        (WebKit::IconDatabase::IconRecord::loadImageFromResource): Deleted.
     22        * UIProcess/API/glib/IconDatabase.h:
     23        * UIProcess/API/glib/WebKitFaviconDatabase.cpp:
     24        (getIconSurfaceSynchronously): Use new API.
     25
    1262017-08-10  Dan Bernstein  <mitz@apple.com>
    227

  • trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp

    r219863 r220580  
    107107}
    108108
    109 Image* IconDatabase::IconRecord::image(const IntSize&)
     109NativeImagePtr IconDatabase::IconRecord::image(const IntSize&)
    110110{
    111111    // FIXME rdar://4680377 - For size right now, we are returning our one and only image and the Bridge
    112112    // is resizing it in place. We need to actually store all the original representations here and return a native
    113113    // one, or resize the best one to the requested size and cache that result.
    114     return m_image.get();
     114    return m_image;
    115115}
    116116
    … …  
    118118{
    119119    m_dataSet = true;
    120 
    121     // It's okay to delete the raw image here. Any existing clients using this icon will be
    122     // managing an image that was created with a copy of this raw image data.
    123     if (!data->size()) {
    124         m_image = nullptr;
    125         return;
    126     }
    127 
    128     m_image = BitmapImage::create();
    129     if (m_image->setData(WTFMove(data), true) < EncodedDataStatus::SizeAvailable) {
     120    m_imageData = WTFMove(data);
     121    m_image = nullptr;
     122
     123    if (!m_imageData->size()) {
     124        m_imageData = nullptr;
     125        return;
     126    }
     127
     128    auto image = BitmapImage::create();
     129    if (image->setData(RefPtr<SharedBuffer> { m_imageData }, true) < EncodedDataStatus::SizeAvailable) {
    130130        LOG(IconDatabase, "Manual image data for iconURL '%s' FAILED - it was probably invalid image data", m_iconURL.ascii().data());
    131         m_image = nullptr;
    132     }
    133 }
    134 
    135 void IconDatabase::IconRecord::loadImageFromResource(const char* resource)
    136 {
    137     if (!resource)
    138         return;
    139 
    140     m_image = Image::loadPlatformResource(resource);
    141     m_dataSet = true;
     131        m_imageData = nullptr;
     132        return;
     133    }
     134
     135    m_image = image->nativeImageForCurrentFrame();
     136    if (!m_image) {
     137        LOG(IconDatabase, "Manual image data for iconURL '%s' FAILED - it was probably invalid image data", m_iconURL.ascii().data());
     138        m_imageData = nullptr;
     139    }
    142140}
    143141
    … …  
    154152{
    155153    if (forDeletion)
    156         return IconSnapshot(m_iconURL, 0, 0);
    157 
    158     return IconSnapshot(m_iconURL, m_stamp, m_image ? m_image->data() : 0);
     154        return IconSnapshot(m_iconURL, 0, nullptr);
     155
     156    return IconSnapshot(m_iconURL, m_stamp, m_imageData ? m_imageData.get() : nullptr);
    159157}
    160158
    … …  
    306304}
    307305
    308 Image* IconDatabase::synchronousIconForPageURL(const String& pageURLOriginal, const IntSize& size)
     306std::pair<NativeImagePtr, IconDatabase::IsKnownIcon> IconDatabase::synchronousIconForPageURL(const String& pageURLOriginal, const IntSize& size)
    309307{
    310308    ASSERT_NOT_SYNC_THREAD();
    … …  
    314312
    315313    if (!isOpen() || !documentCanHaveIcon(pageURLOriginal))
    316         return nullptr;
     314        return { nullptr, IsKnownIcon::No };
    317315
    318316    LockHolder locker(m_urlAndIconLock);
    … …  
    339337            m_pageURLsInterestedInIcons.add(pageURLCopy);
    340338
    341         return nullptr;
     339        return { nullptr, IsKnownIcon::No };
    342340    }
    343341
    … …  
    348346    // we can just bail now
    349347    if (!m_iconURLImportComplete && !iconRecord)
    350         return nullptr;
     348        return { nullptr, IsKnownIcon::No };
    351349
    352350    // Assuming we're done initializing and cleanup is allowed,
    … …  
    355353
    356354    if (!iconRecord)
    357         return nullptr;
     355        return { nullptr, IsKnownIcon::No };
    358356
    359357    // If it's a new IconRecord object that doesn't have its imageData set yet,
    … …  
    367365        m_iconsPendingReading.add(iconRecord);
    368366        wakeSyncThread();
    369         return nullptr;
     367        return { nullptr, IsKnownIcon::No };
    370368    }
    371369
    … …  
    373371    // and isn't actually interested in the image return value
    374372    if (size == IntSize(0, 0))
    375         return nullptr;
    376 
    377     // PARANOID DISCUSSION: This method makes some assumptions. It returns a WebCore::image which the icon database might dispose of at anytime in the future,
    378     // and Images aren't ref counted. So there is no way for the client to guarantee continued existence of the image.
    379     // This has *always* been the case, but in practice clients would always create some other platform specific representation of the image
    380     // and drop the raw Image*. On Mac an NSImage, and on windows drawing into an HBITMAP.
    381     // The async aspect adds a huge question - what if the image is deleted before the platform specific API has a chance to create its own
    382     // representation out of it?
    383     // If an image is read in from the icondatabase, we do *not* overwrite any image data that exists in the in-memory cache.
    384     // This is because we make the assumption that anything in memory is newer than whatever is in the database.
    385     // So the only time the data will be set from the second thread is when it is INITIALLY being read in from the database, but we would never
    386     // delete the image on the secondary thread if the image already exists.
    387     return iconRecord->image(size);
     373        return { nullptr, IsKnownIcon::Yes };
     374
     375    return { iconRecord->image(size), IsKnownIcon::Yes };
    388376}
    389377

  • trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.h

    r218922 r220580  
    3838
    3939namespace WebCore {
    40 class Image;
    4140class SharedBuffer;
    4241}
    … …  
    6059
    6160private:
    62     enum ImageDataStatus {
    63         ImageDataStatusPresent, ImageDataStatusMissing, ImageDataStatusUnknown
    64     };
    65 
    6661    class IconSnapshot {
    6762    public:
    … …  
    9691
    9792        void setImageData(RefPtr<WebCore::SharedBuffer>&&);
    98         WebCore::Image* image(const WebCore::IntSize&);
     93        WebCore::NativeImagePtr image(const WebCore::IntSize&);
    9994
    10095        String iconURL() { return m_iconURL; }
    101 
    102         void loadImageFromResource(const char*);
    10396
    10497        enum class ImageDataStatus { Present, Missing, Unknown };
    … …  
    114107        String m_iconURL;
    115108        time_t m_stamp { 0 };
    116         RefPtr<WebCore::Image> m_image;
     109        RefPtr<WebCore::SharedBuffer> m_imageData;
     110        WebCore::NativeImagePtr m_image;
    117111
    118112        HashSet<String> m_retainingPageURLs;
    … …  
    198192    void setIconURLForPageURL(const String& iconURL, const String& pageURL);
    199193
    200     WebCore::Image* synchronousIconForPageURL(const String&, const WebCore::IntSize&);
     194    enum class IsKnownIcon { No, Yes };
     195    std::pair<WebCore::NativeImagePtr, IsKnownIcon> synchronousIconForPageURL(const String&, const WebCore::IntSize&);
    201196    String synchronousIconURLForPageURL(const String&);
    202197    bool synchronousIconDataKnownForIconURL(const String&);

  • trunk/Source/WebKit/UIProcess/API/glib/WebKitFaviconDatabase.cpp

    r219863 r220580  
    140140    // The exact size we pass is irrelevant to the iconDatabase code.
    141141    // We must pass something greater than 0x0 to get an icon.
    142     WebCore::Image* iconImage = database->priv->iconDatabase->synchronousIconForPageURL(pageURL, WebCore::IntSize(1, 1));
    143     if (!iconImage) {
     142    auto iconData = database->priv->iconDatabase->synchronousIconForPageURL(pageURL, WebCore::IntSize(1, 1));
     143    if (iconData.second == IconDatabase::IsKnownIcon::No) {
    144144        g_set_error(error, WEBKIT_FAVICON_DATABASE_ERROR, WEBKIT_FAVICON_DATABASE_ERROR_FAVICON_UNKNOWN, _("Unknown favicon for page %s"), pageURL.utf8().data());
    145145        return nullptr;
    146146    }
    147147
    148     RefPtr<cairo_surface_t> surface = iconImage->nativeImageForCurrentFrame();
    149     if (!surface) {
     148    if (!iconData.first) {
    150149        g_set_error(error, WEBKIT_FAVICON_DATABASE_ERROR, WEBKIT_FAVICON_DATABASE_ERROR_FAVICON_NOT_FOUND, _("Page %s does not have a favicon"), pageURL.utf8().data());
    151150        return nullptr;
    152151    }
    153152
    154     return surface;
     153    return iconData.first;
    155154}
    156155
Note: See TracChangeset for help on using the changeset viewer.