[webkit-reviews] review requested: [Bug 97966] [WK2][GTK] Fix issues with WebKitFaviconDatabase in debug builds : [Attachment 166383] Patch proposal (without the early return)

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  for review:
Bug 97966: [WK2][GTK] Fix issues with WebKitFaviconDatabase in debug builds
https://bugs.webkit.org/show_bug.cgi?id=97966

Attachment 166383: Patch proposal (without the early return)
https://bugs.webkit.org/attachment.cgi?id=166383&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
(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/TestWeb
KitFaviconDatabase... (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!)