Bug 97966

Summary: [WK2][GTK] Fix issues with WebKitFaviconDatabase in debug builds
Product: WebKit Reporter: Mario Sanchez Prada <mario>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, gustavo, mrobinson, webkit.review.bot
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch proposal
cgarcia: review-
Patch proposal (without the early return) cgarcia: review+

Mario Sanchez Prada
Reported 2012-09-29 08:28:10 PDT
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
Attachments
Patch proposal (7.52 KB, patch)
2012-09-30 06:15 PDT, Mario Sanchez Prada 		cgarcia: review-
DetailsFormatted DiffDiff
Patch proposal (without the early return) (6.84 KB, patch)
2012-09-30 06:27 PDT, Mario Sanchez Prada 		cgarcia: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Carlos Garcia Campos
Comment 1 2012-09-30 00:56:18 PDT
(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
Comment 2 2012-09-30 06:15:30 PDT
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
Comment 3 2012-09-30 06:27:42 PDT
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
Comment 4 2012-09-30 06:29:53 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
Mario Sanchez Prada
Comment 5 2012-09-30 07:55:57 PDT
Committed r129992: <http://trac.webkit.org/changeset/129992>
Note You need to log in before you can comment on or make changes to this bug.