Today I got a crash when I tried to LD_PRELOAD a debug build of WebKit and forgot to also preload jsc. The crash was obviously caused by a mismatch between those, but it did provide an interesting insight about our web database initialization. As can be seen bellow when trying to set the database path we end up initializing the tracker with the path pointing to a file in /, which is not optimal, maybe we can do better. Program received signal SIGSEGV, Segmentation fault. 0x00007fffeefaf701 in ?? () from /lib/x86_64-linux-gnu/libc.so.6 (gdb) bt #0 0x00007fffeefaf701 in ?? () from /lib/x86_64-linux-gnu/libc.so.6 #1 0x00007fffef5478f2 in g_uri_unescape_segment (escaped_string=0x15 <Address 0x15 out of bounds>, escaped_string_end=0x0, illegal_characters=0x0) at gurifuncs.c:96 #2 0x00007ffff5991428 in WebCore::fileSystemRepresentation (path="/Databases.db") at ../../Source/WebCore/platform/gtk/FileSystemGtk.cpp:60 #3 0x00007ffff5991559 in WebCore::fileExists (path="/Databases.db") at ../../Source/WebCore/platform/gtk/FileSystemGtk.cpp:83 #4 0x00007ffff509768e in WebCore::SQLiteFileSystem::ensureDatabaseFileExists (fileName="/Databases.db", checkPathOnly=false) at ../../Source/WebCore/platform/sql/SQLiteFileSystem.cpp:106 #5 0x00007ffff52f0bae in WebCore::DatabaseTracker::openTrackerDatabase (this=0x7fffe4350d80, createIfDoesNotExist=false) at ../../Source/WebCore/storage/DatabaseTracker.cpp:116 #6 0x00007ffff52f204f in WebCore::DatabaseTracker::populateOrigins (this=0x7fffe4350d80) at ../../Source/WebCore/storage/DatabaseTracker.cpp:342 #7 0x00007ffff52f09a2 in WebCore::DatabaseTracker::DatabaseTracker (this=0x7fffe4350d80, databasePath="") at ../../Source/WebCore/storage/DatabaseTracker.cpp:88 #8 0x00007ffff52f087c in WebCore::DatabaseTracker::tracker () at ../../Source/WebCore/storage/DatabaseTracker.cpp:75 #9 0x00007ffff47cc606 in webkit_set_web_database_directory_path (path=0x9ecb00 "/home/kov/.local/share/webkit/databases") at ../../Source/WebKit/gtk/webkit/webkitwebdatabase.cpp:496 #10 0x00007ffff47c239c in webkitInit () at ../../Source/WebKit/gtk/webkit/webkitglobals.cpp:324 #11 0x00007ffff47d8eed in webkit_web_settings_class_init (klass=0x9ec860) at ../../Source/WebKit/gtk/webkit/webkitwebsettings.cpp:203 #12 0x00007ffff47d89cf in webkit_web_settings_class_intern_init (klass=0x9ec860) at ../../Source/WebKit/gtk/webkit/webkitwebsettings.cpp:68 #13 0x00007ffff0032c57 in type_class_init_Wm (pclass=0x6d1ae0, node=0xa5b890) at gtype.c:2219 #14 g_type_class_ref (type=<optimized out>) at gtype.c:2925 #15 0x00007ffff001b38c in g_object_newv (object_type=10860688, n_parameters=0, parameters=0x0) at gobject.c:1608 #16 0x00007ffff001b8cc in g_object_new (object_type=10860688, first_property_name=0x0) at gobject.c:1532 #17 0x00007ffff47db254 in webkit_web_settings_new () at ../../Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1288 #18 0x000000000046d741 in ephy_embed_prefs_init () at ephy-embed-prefs.c:480 #19 0x0000000000466f9d in ephy_embed_single_initialize (single=0xa520c0) at ephy-embed-single.c:423 #20 0x0000000000467b45 in impl_get_embed_single (shell=0x8b6000) at ephy-embed-shell.c:224 #21 0x00000000004324fa in impl_get_embed_single (embed_shell=0x8b6000) at ephy-shell.c:580 #22 0x0000000000437542 in ephy_window_constructor (type=<optimized out>, n_construct_properties=<optimized out>, construct_params=<optimized out>) at ephy-window.c:3419 #23 0x00007ffff001adc4 in g_object_newv (object_type=<optimized out>, n_parameters=9193632, parameters=<optimized out>) at gobject.c:1703 #24 0x00007ffff001b5a6 in g_object_new_valist (object_type=9185984, first_property_name=<optimized out>, var_args=0x7fffffffd858) at gobject.c:1820 #25 0x00007ffff001b8b4 in g_object_new (object_type=9185984, first_property_name=0x48fd47 "chrome") at gobject.c:1535 #26 0x0000000000437f85 in ephy_window_new_with_chrome (chrome=15, is_popup=0) at ephy-window.c:3613 #27 0x0000000000431c32 in ephy_shell_new_tab_full (shell=<optimized out>, parent_window=0x0, previous_embed=0x0, request=0x0, flags=1025, chrome=15, is_popup=0, user_time=630077808) at ephy-shell.c:764 #28 0x000000000045097a in session_command_dispatch (session=<optimized out>) at ephy-session.c:726 #29 0x00007fffef51d13a in g_main_dispatch (context=0x719c60) at gmain.c:2510 #30 g_main_context_dispatch (context=0x719c60) at gmain.c:3047 #31 0x00007fffef51d500 in g_main_context_iterate (dispatch=1, block=<optimized out>, context=0x719c60, self=<optimized out>) at gmain.c:3118 #32 g_main_context_iterate (context=0x719c60, block=<optimized out>, dispatch=1, self=<optimized out>) at gmain.c:3055 #33 0x00007fffef51d5c4 in g_main_context_iteration (context=0x719c60, may_block=1) at gmain.c:3179 #34 0x00007ffff0526124 in g_application_run (application=0x8b6000, argc=<optimized out>, argv=0x7fffffffdd08) at gapplication.c:1496 #35 0x000000000043060f in main (argc=1, argv=0x7fffffffdd08) at ephy-main.c:469
Created attachment 125237 [details] Untested (even unbuilt) patch I am still finishing my build to test this, but thought I'd upload to get it ran through the EWS while I sleep =P =)
Comment on attachment 125237 [details] Untested (even unbuilt) patch This isn't enough, unfortunately, I'll upload another patch that fixes the whole thing and so we can discuss.
Created attachment 125497 [details] Patch
Now we get to decide =) pros: * it fixes something that works only because of a side effect of the user not having root privileges to open the file in / * it fixes the fact that we were never getting populateOrigins called * it passes all the tests and maintains API promises cons: * ugly ifdefs * WebKitGTK+-specific code paths which might not be needed in WebKit2GTK+, since we can make the API promises more akin to the real world
Comment on attachment 125497 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125497&action=review > Source/WebKit/gtk/webkit/webkitwebdatabase.cpp:504 > + static bool initialized = false; > + > WTF::String corePath = WTF::String::fromUTF8(path); > + > + if (!initialized) { > + initialized = true; > + WebCore::DatabaseTracker::initializeTracker(corePath); > + return; > + } > + I'm not sure I fully understand what's going on here. Wouldn't it be sufficient just to do WebCore::DatabaseTracker::initializeTracker the first time and avoid all of the WebCore changes? It's really sad that our API supports adjusting the database location, but not the tracker location. I feel like this could lead to issues if an application wants to change the database location mid-run.
Let me describe what I found: DatabaseTracker does not allow you to set the location after the database has been opened. It turns out that just creating a DatabaseTracker instace with a valid path where the user has permissions to write to causes the database to be opened: DatabaseTracker's constructor calls populateOrigins, which will open the database. Right now what we do is we unconditionally set a path to the DatabaseTracker during webkit_init() it goes like this: GOwnPtr<gchar> databaseDirectory(g_build_filename(g_get_user_data_dir(), "webkit", "databases", NULL)); webkit_set_web_database_directory_path(databaseDirectory.get()); Now look at webkit_set_web_database_directory_path: WTF::String corePath = WTF::String::fromUTF8(path); WebCore::DatabaseTracker::tracker().setDatabaseDirectoryPath(corePath); That tracker() call in there causes the DatabaseTracker to be constructed with an empty directory path, so when populateOrigins tries to open the database it fails (since the user does not have permission to create files in /). Then setDatabaseDirectoryPath is called with the intended path and it works, because the database has not been opened. If we run as root this breaks. After that happens it's still possible to call webkit_set_web_database_directory_path(), because setDatabaseDirectoryPath() does not cause the database to be opened. If you load a page and then open the web inspector to look at web databases the database will then be opened, but populateOrigins() won't be called as it should. We can drop some of the WebCore changes if we're OK with not getting populateOrigins() to run when the database is open, for instance. But if we want to also leave the populateOrigins() call inside the DatabaseTracker constructor we'll have to remove the call to webkit_set_web_database_directory_path() from webkit_init and set a default path when the database is opened instead - if none is set.
Comment on attachment 125497 [details] Patch I wonder if we can do two things here: 1. Create a bug for handling setting the path to the database tracker twice. 2. Fix the bug in WebKitGTK+ by delaying the tracker initialization until the first frame starts loading. At that point if the user has set a path we can use that, if not we can use the default path.
I suggest we do not spend time on this, but use this knowledge for the new webkit2 API, when and if we come to it.