Modified: trunk/Source/WebKit/ChangeLog (220579 => 220580)
--- trunk/Source/WebKit/ChangeLog 2017-08-11 05:31:32 UTC (rev 220579)
+++ trunk/Source/WebKit/ChangeLog 2017-08-11 06:01:15 UTC (rev 220580)
@@ -1,3 +1,28 @@
+2017-08-10 Carlos Garcia Campos <cgar...@igalia.com>
+
+ [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.
+
2017-08-10 Dan Bernstein <m...@apple.com>
Restored svn:ignore values that went missing when the project got renamed.
Modified: trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp (220579 => 220580)
--- trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp 2017-08-11 05:31:32 UTC (rev 220579)
+++ trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.cpp 2017-08-11 06:01:15 UTC (rev 220580)
@@ -106,39 +106,37 @@
LOG(IconDatabase, "Destroying IconRecord for icon url %s", m_iconURL.ascii().data());
}
-Image* IconDatabase::IconRecord::image(const IntSize&)
+NativeImagePtr IconDatabase::IconRecord::image(const IntSize&)
{
// FIXME rdar://4680377 - For size right now, we are returning our one and only image and the Bridge
// is resizing it in place. We need to actually store all the original representations here and return a native
// one, or resize the best one to the requested size and cache that result.
- return m_image.get();
+ return m_image;
}
void IconDatabase::IconRecord::setImageData(RefPtr<SharedBuffer>&& data)
{
m_dataSet = true;
+ m_imageData = WTFMove(data);
+ m_image = nullptr;
- // 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.
- if (!data->size()) {
- m_image = nullptr;
+ if (!m_imageData->size()) {
+ m_imageData = nullptr;
return;
}
- m_image = BitmapImage::create();
- if (m_image->setData(WTFMove(data), true) < EncodedDataStatus::SizeAvailable) {
+ auto image = BitmapImage::create();
+ if (image->setData(RefPtr<SharedBuffer> { m_imageData }, true) < EncodedDataStatus::SizeAvailable) {
LOG(IconDatabase, "Manual image data for iconURL '%s' FAILED - it was probably invalid image data", m_iconURL.ascii().data());
- m_image = nullptr;
+ m_imageData = nullptr;
+ return;
}
-}
-void IconDatabase::IconRecord::loadImageFromResource(const char* resource)
-{
- if (!resource)
- return;
-
- m_image = Image::loadPlatformResource(resource);
- m_dataSet = true;
+ m_image = image->nativeImageForCurrentFrame();
+ if (!m_image) {
+ LOG(IconDatabase, "Manual image data for iconURL '%s' FAILED - it was probably invalid image data", m_iconURL.ascii().data());
+ m_imageData = nullptr;
+ }
}
IconDatabase::IconRecord::ImageDataStatus IconDatabase::IconRecord::imageDataStatus()
@@ -153,9 +151,9 @@
IconDatabase::IconSnapshot IconDatabase::IconRecord::snapshot(bool forDeletion) const
{
if (forDeletion)
- return IconSnapshot(m_iconURL, 0, 0);
+ return IconSnapshot(m_iconURL, 0, nullptr);
- return IconSnapshot(m_iconURL, m_stamp, m_image ? m_image->data() : 0);
+ return IconSnapshot(m_iconURL, m_stamp, m_imageData ? m_imageData.get() : nullptr);
}
IconDatabase::PageURLRecord::PageURLRecord(const String& pageURL)
@@ -305,7 +303,7 @@
return !documentURL.isEmpty() && !protocolIs(documentURL, "about");
}
-Image* IconDatabase::synchronousIconForPageURL(const String& pageURLOriginal, const IntSize& size)
+std::pair<NativeImagePtr, IconDatabase::IsKnownIcon> IconDatabase::synchronousIconForPageURL(const String& pageURLOriginal, const IntSize& size)
{
ASSERT_NOT_SYNC_THREAD();
@@ -313,7 +311,7 @@
// We should go our of our way to only copy it if we have to store it
if (!isOpen() || !documentCanHaveIcon(pageURLOriginal))
- return nullptr;
+ return { nullptr, IsKnownIcon::No };
LockHolder locker(m_urlAndIconLock);
@@ -338,7 +336,7 @@
if (!m_iconURLImportComplete)
m_pageURLsInterestedInIcons.add(pageURLCopy);
- return nullptr;
+ return { nullptr, IsKnownIcon::No };
}
IconRecord* iconRecord = pageRecord->iconRecord();
@@ -347,7 +345,7 @@
// In this case, the pageURL is already in the set to alert the client when the iconURL mapping is complete so
// we can just bail now
if (!m_iconURLImportComplete && !iconRecord)
- return nullptr;
+ return { nullptr, IsKnownIcon::No };
// Assuming we're done initializing and cleanup is allowed,
// the only way we should *not* have an icon record is if this pageURL is retained but has no icon yet.
@@ -354,7 +352,7 @@
ASSERT(iconRecord || databaseCleanupCounter || m_retainedPageURLs.contains(pageURLOriginal));
if (!iconRecord)
- return nullptr;
+ return { nullptr, IsKnownIcon::No };
// If it's a new IconRecord object that doesn't have its imageData set yet,
// mark it to be read by the background thread
@@ -366,25 +364,15 @@
m_pageURLsInterestedInIcons.add(pageURLCopy);
m_iconsPendingReading.add(iconRecord);
wakeSyncThread();
- return nullptr;
+ return { nullptr, IsKnownIcon::No };
}
// If the size parameter was (0, 0) that means the caller of this method just wanted the read from disk to be kicked off
// and isn't actually interested in the image return value
if (size == IntSize(0, 0))
- return nullptr;
+ return { nullptr, IsKnownIcon::Yes };
- // PARANOID DISCUSSION: This method makes some assumptions. It returns a WebCore::image which the icon database might dispose of at anytime in the future,
- // and Images aren't ref counted. So there is no way for the client to guarantee continued existence of the image.
- // This has *always* been the case, but in practice clients would always create some other platform specific representation of the image
- // and drop the raw Image*. On Mac an NSImage, and on windows drawing into an HBITMAP.
- // 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
- // representation out of it?
- // If an image is read in from the icondatabase, we do *not* overwrite any image data that exists in the in-memory cache.
- // This is because we make the assumption that anything in memory is newer than whatever is in the database.
- // 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
- // delete the image on the secondary thread if the image already exists.
- return iconRecord->image(size);
+ return { iconRecord->image(size), IsKnownIcon::Yes };
}
String IconDatabase::synchronousIconURLForPageURL(const String& pageURLOriginal)
Modified: trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.h (220579 => 220580)
--- trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.h 2017-08-11 05:31:32 UTC (rev 220579)
+++ trunk/Source/WebKit/UIProcess/API/glib/IconDatabase.h 2017-08-11 06:01:15 UTC (rev 220580)
@@ -37,7 +37,6 @@
#include <wtf/text/WTFString.h>
namespace WebCore {
-class Image;
class SharedBuffer;
}
@@ -59,10 +58,6 @@
WTF_MAKE_FAST_ALLOCATED;
private:
- enum ImageDataStatus {
- ImageDataStatusPresent, ImageDataStatusMissing, ImageDataStatusUnknown
- };
-
class IconSnapshot {
public:
IconSnapshot() = default;
@@ -95,12 +90,10 @@
void setTimestamp(time_t stamp) { m_stamp = stamp; }
void setImageData(RefPtr<WebCore::SharedBuffer>&&);
- WebCore::Image* image(const WebCore::IntSize&);
+ WebCore::NativeImagePtr image(const WebCore::IntSize&);
String iconURL() { return m_iconURL; }
- void loadImageFromResource(const char*);
-
enum class ImageDataStatus { Present, Missing, Unknown };
ImageDataStatus imageDataStatus();
@@ -113,7 +106,8 @@
String m_iconURL;
time_t m_stamp { 0 };
- RefPtr<WebCore::Image> m_image;
+ RefPtr<WebCore::SharedBuffer> m_imageData;
+ WebCore::NativeImagePtr m_image;
HashSet<String> m_retainingPageURLs;
@@ -197,7 +191,8 @@
void setIconDataForIconURL(RefPtr<WebCore::SharedBuffer>&&, const String& iconURL);
void setIconURLForPageURL(const String& iconURL, const String& pageURL);
- WebCore::Image* synchronousIconForPageURL(const String&, const WebCore::IntSize&);
+ enum class IsKnownIcon { No, Yes };
+ std::pair<WebCore::NativeImagePtr, IsKnownIcon> synchronousIconForPageURL(const String&, const WebCore::IntSize&);
String synchronousIconURLForPageURL(const String&);
bool synchronousIconDataKnownForIconURL(const String&);
IconLoadDecision synchronousLoadDecisionForIconURL(const String&);
Modified: trunk/Source/WebKit/UIProcess/API/glib/WebKitFaviconDatabase.cpp (220579 => 220580)
--- trunk/Source/WebKit/UIProcess/API/glib/WebKitFaviconDatabase.cpp 2017-08-11 05:31:32 UTC (rev 220579)
+++ trunk/Source/WebKit/UIProcess/API/glib/WebKitFaviconDatabase.cpp 2017-08-11 06:01:15 UTC (rev 220580)
@@ -139,19 +139,18 @@
// The exact size we pass is irrelevant to the iconDatabase code.
// We must pass something greater than 0x0 to get an icon.
- WebCore::Image* iconImage = database->priv->iconDatabase->synchronousIconForPageURL(pageURL, WebCore::IntSize(1, 1));
- if (!iconImage) {
+ auto iconData = database->priv->iconDatabase->synchronousIconForPageURL(pageURL, WebCore::IntSize(1, 1));
+ if (iconData.second == IconDatabase::IsKnownIcon::No) {
g_set_error(error, WEBKIT_FAVICON_DATABASE_ERROR, WEBKIT_FAVICON_DATABASE_ERROR_FAVICON_UNKNOWN, _("Unknown favicon for page %s"), pageURL.utf8().data());
return nullptr;
}
- RefPtr<cairo_surface_t> surface = iconImage->nativeImageForCurrentFrame();
- if (!surface) {
+ if (!iconData.first) {
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());
return nullptr;
}
- return surface;
+ return iconData.first;
}
static void deletePendingIconRequests(WebKitFaviconDatabase* database, PendingIconRequestVector* requests, const String& pageURL)
