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

See Also:

Attachments
Patch (4.46 KB, patch)
2012-10-05 04:09 PDT, Carlos Garcia Campos 		mrobinson: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

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]
Patch
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]
Patch

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]
Patch

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>