Bug 98507 - [GTK] Test /webkit2/WebKitWebView/populate-menu asserts in debug bots
Summary: [GTK] Test /webkit2/WebKitWebView/populate-menu asserts in debug bots
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
Keywords: Gtk
Depends on:
Reported: 2012-10-05 04:03 PDT by Carlos Garcia Campos
Modified: 2012-10-08 02:28 PDT (History)
4 users (show)

See Also:

Patch (4.46 KB, patch)
2012-10-05 04:09 PDT, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-10-05 04:03:42 PDT
/webkit2/WebKitWebView/populate-menu:                                ASSERTION FAILED: iconRecord || m_retainedPageURLs.contains(pageURLOriginal)

../../Source/WebCore/loader/icon/IconDatabase.cpp(266) : virtual WebCore::Image* WebCore::IconDatabase::synchronousIconForPageURL(const WTF::String&, const WebCore::IntSize&)
Comment 1 Carlos Garcia Campos 2012-10-05 04:09:49 PDT
Created attachment 167305 [details]
Comment 2 WebKit Review Bot 2012-10-05 04:11:49 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 3 Martin Robinson 2012-10-05 08:18:22 PDT
Comment on attachment 167305 [details]

Okay. But before you land, do you mind having Mario look over this too?
Comment 4 Mario Sanchez Prada 2012-10-08 01:55:23 PDT
Comment on attachment 167305 [details]

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

The patch looks good to me too. A pity I didn't realize of this issue when writing the original patch, quite happy Carlos figured it out and fixed it, though.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:151
>      PendingIconRequestVector* icons = database->priv->pendingIconRequests.get(pageURL);

Completely off-topic thing: you might consider renaming this 'icons' variable before landing to something more adequate, such as 'pendingIconRequests', or something like that.

> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:156
> +    RefPtr<cairo_surface_t> icon = getIconSurfaceSynchronously(database, pageURL, &error.outPtr());

I like this refactoring: taking this call out of the loop. Good catch.
Comment 5 Carlos Garcia Campos 2012-10-08 02:28:34 PDT
Committed r130625: <http://trac.webkit.org/changeset/130625>