Bug 97966 – [WK2][GTK] Fix issues with WebKitFaviconDatabase in debug builds

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Mario Sanchez Prada]

After landing the patch for bug 96476 a couple of issues were spotted in debug builds:

The first one is that the ASSERT(!pageURL.isNull()) statement in webkitFaviconDatabaseGetFavicon() is wrong and should be replaced by an early return, since it won't be true whenever we ask a webview for its favicon before actually loading anything into it.

The second issue is that we must find a way to delete the temporary directory used to store the database in the unit tests *after* the IconDatabase from WebCore is closed (now it's closed when the WebKitFaviconDatabase is finalized), otherwise we will keep spotting the following disk I/O error in debug build when running TestWebKitFaviconDatabase in debug build:

   TEST: ./Tools/gtk/../Scripts/../../WebKitBuild/Debug/Programs/WebKit2APITests/TestWebKitFaviconDatabase... (pid=27818)
     /webkit2/WebKitFaviconDatabase/set-directory:                        OK
     /webkit2/WebKitFaviconDatabase/get-favicon:                          OK
     /webkit2/WebKitFaviconDatabase/get-favicon-uri:                      OK
     /webkit2/WebKitFaviconDatabase/clear-database:                       OK
   ERROR: Could not create PageURL table in database (1802) - disk I/O error
   ../../Source/WebCore/loader/icon/IconDatabase.cpp(1133) : void WebCore::createDatabaseTables(WebCore::SQLiteDatabase&)

Since those are related issues that must be fixed asap, I'm filing this bug now to keep track of them and propose (hopefully in 1-2 days from now) a fix for both of them

[Carlos Garcia Campos]

(In reply to comment #0)
> After landing the patch for bug 96476 a couple of issues were spotted in debug builds:
> 
> The first one is that the ASSERT(!pageURL.isNull()) statement in webkitFaviconDatabaseGetFavicon() is wrong and should be replaced by an early return, since it won't be true whenever we ask a webview for its favicon before actually loading anything into it.

I don't think the assert is necessarily wrong, you should check in webkit_web_view_get_favicon() that the active URI is not NULL before calling webkitFaviconDatabaseGetFavicon(). so I think the early return should be in webkit_web_view_get_favicon().

> The second issue is that we must find a way to delete the temporary directory used to store the database in the unit tests *after* the IconDatabase from WebCore is closed (now it's closed when the WebKitFaviconDatabase is finalized), otherwise we will keep spotting the following disk I/O error in debug build when running TestWebKitFaviconDatabase in debug build:
> 
>    TEST: ./Tools/gtk/../Scripts/../../WebKitBuild/Debug/Programs/WebKit2APITests/TestWebKitFaviconDatabase... (pid=27818)
>      /webkit2/WebKitFaviconDatabase/set-directory:                        OK
>      /webkit2/WebKitFaviconDatabase/get-favicon:                          OK
>      /webkit2/WebKitFaviconDatabase/get-favicon-uri:                      OK
>      /webkit2/WebKitFaviconDatabase/clear-database:                       OK
>    ERROR: Could not create PageURL table in database (1802) - disk I/O error
>    ../../Source/WebCore/loader/icon/IconDatabase.cpp(1133) : void WebCore::createDatabaseTables(WebCore::SQLiteDatabase&)
> 
> Since those are related issues that must be fixed asap, I'm filing this bug now to keep track of them and propose (hopefully in 1-2 days from now) a fix for both of them

First issue can be fixed while landing patch for bug #96477

[Mario Sanchez Prada]

Created attachment 166382 [details]
Patch proposal

This patch seems to fix these issues once and for all. I've tested it both in Release and Debug build and couldn't spot those issues anymore.

[Mario Sanchez Prada]

Created attachment 166383 [details]
Patch proposal (without the early return)

(In reply to comment #1)
> I don't think the assert is necessarily wrong, you should check in 
> webkit_web_view_get_favicon() that the active URI is not NULL before calling 
> webkitFaviconDatabaseGetFavicon(). so I think the early return should be in 
> webkit_web_view_get_favicon().

Hrm... I haven't seen this comment before uploading the patch, but you are probably right about it.

> > The second issue is that we must find a way to delete the temporary directory used to store the database in the unit tests *after* the IconDatabase from WebCore is closed (now it's closed when the WebKitFaviconDatabase is finalized), otherwise we will keep spotting the following disk I/O error in debug build when running TestWebKitFaviconDatabase in debug build:
> > 
> >    TEST: ./Tools/gtk/../Scripts/../../WebKitBuild/Debug/Programs/WebKit2APITests/TestWebKitFaviconDatabase... (pid=27818)
> >      /webkit2/WebKitFaviconDatabase/set-directory:                        OK
> >      /webkit2/WebKitFaviconDatabase/get-favicon:                          OK
> >      /webkit2/WebKitFaviconDatabase/get-favicon-uri:                      OK
> >      /webkit2/WebKitFaviconDatabase/clear-database:                       OK
> >    ERROR: Could not create PageURL table in database (1802) - disk I/O error
> >    ../../Source/WebCore/loader/icon/IconDatabase.cpp(1133) : void WebCore::createDatabaseTables(WebCore::SQLiteDatabase&)
> > 
> > Since those are related issues that must be fixed asap, I'm filing this bug now to keep track of them and propose (hopefully in 1-2 days from now) a fix for both of them
> 
> First issue can be fixed while landing patch for bug #96477

Yes. I'm attaching a new patch now without the ASSERT, and will add the early return in patch for 96477, as you suggested.

Feel free to r- any of the two patches then (and hopefully r+ the other!)

[WebKit Review 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

[Mario Sanchez Prada]

Committed r129992: <http://trac.webkit.org/changeset/129992>