Bug 143245 - Crash when WebCore::SQLiteFileSystem::openDatabase is called from multiple threads
Summary: Crash when WebCore::SQLiteFileSystem::openDatabase is called from multiple th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-30 18:35 PDT by Michael Catanzaro
Modified: 2015-09-13 16:14 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.26 KB, patch)
2015-08-27 17:42 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.69 KB, patch)
2015-08-31 13:12 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (3.04 KB, patch)
2015-09-06 07:48 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (3.20 KB, patch)
2015-09-08 14:29 PDT, Michael Catanzaro
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2015-03-30 18:35:06 PDT
With WebKitGTK+ 2.8.0, the UI process likes to crash when multiple threads call WebCore::SQLiteFileSystem::openDatabase at the same time.  I never understood sqlite3 thread safety but a reasonable guess is that we're doing it wrong.

Backtraces:

https://bugzilla.redhat.com/attachment.cgi?id=1001406
https://bugzilla.redhat.com/attachment.cgi?id=1008458
Comment 1 Carlos Garcia Campos 2015-04-01 06:29:16 PDT
The question here is why there are two work queue threads for local storage if there's only one web context.
Comment 2 Carlos Garcia Campos 2015-04-01 06:41:31 PDT
Ok, it's the inspector web context. It happens since the inspector uses its own web context (r173929). It's because WebProcessProxy::getLaunchOptions() creates the inspector context to check if the current context is the inspector one or not , to add "inspector-process" extra config. I still don't know why sqlite crashes when trying to open the same database from multiple processes though, I don't even know if sqlite is supposed to be thread-safe. But we could just move the inspector-process thing to mac and ios platformGetLaunchOptions() methods, to avoid creating the two web contexts at the same time (and also to avoid the creation of the inspector web context even if the inspector is never used). Another alternative to avoid this could be using a different way to identify the inspector context without having to create the context for that. 
I also wonder if we should avoid creating the local storage database when localStorage is disabled in settings, but that would be a different issue.
Comment 3 Michael Catanzaro 2015-04-04 12:37:40 PDT
Different but very related issue: I notice that one WebProcessPool is always leaked (using a debug build) when starting any application that uses WebKit, sometimes two. I never bothered to track it down, but surely it is that inspector "context." I don't think it's OK for WebInspectorProxy::inspectorProcessPool to intentionally leak the inspector context because WebProcessPool is fast-allocated. Also, WebInspectorProxy::inspectorProcessPool is not thread-safe; I presume the double leak occurs when multiple web processes are started at once. In platform-specific code I would fix that using GOnce and a RefPtr, like we do for the default web context, but I'm not sure what the appropriate idiom would be in WebInspectorProxy.cpp.

Maybe I should file another bug for this, but I guess it may be obsoleted by a fix for this bug.

(In reply to comment #2)
> I also wonder if we should avoid creating the local storage database when
> localStorage is disabled in settings, but that would be a different issue.

Yes....

Regarding thread safety: https://sqlite.org/threadsafe.html

In particular, "In serialized mode, SQLite can be safely used by multiple threads with no restriction" and "The default mode is serialized." Hm. I did a few git greps and am pretty sure we never change the default mode, which suggests this may be a bug in SQLite. It would probably be good to explicitly select the threading mode we need, because the default can be changed when compiling sqlite3, so we don't actually know for sure which mode we're using. I presume all sane Linux distros will default to serialized mode, though.
Comment 4 Carlos Garcia Campos 2015-04-13 04:37:55 PDT
hmm, Anders made the inspector process pool to be created lazily in r182447, that will definitely help for this problem.
Comment 5 Michael Catanzaro 2015-05-30 18:35:42 PDT
Hm, turns out the crash can occur when openDatabase is being called from only one thread.
Comment 6 Michael Catanzaro 2015-05-30 19:30:21 PDT
Ah, but another thread was calling LocalStorageDatabaseTracker::importOriginIdentifiers (backtrace in the downstream GNOME Builder bug).
Comment 7 Michael Catanzaro 2015-05-31 07:16:48 PDT
OK, but the question is: why was another thread calling LocalStorageDatabaseTracker::importOriginIdentifiers? This second crash is in GNOME Builder, which does not use the enable-developer-extras settings, so there should be no web inspector and no inspector web context. I guess the inspector web context is always created no matter what, at least in 2.8.
Comment 8 Michael Catanzaro 2015-06-15 08:27:35 PDT
I tested a patch in Fedora to see if forcing the database to open in serialized mode would make any difference, but the crash still occurs. I think this is surely an SQLite bug: they promise complete thread-safety.
Comment 9 Michael Catanzaro 2015-08-27 17:42:21 PDT
Created attachment 260100 [details]
Patch
Comment 10 Michael Catanzaro 2015-08-31 13:12:56 PDT
Created attachment 260311 [details]
Patch
Comment 11 Zan Dobersek 2015-08-31 23:30:32 PDT
Comment on attachment 260311 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260311&action=review

As it's been noted in the RH Bugzilla, sqlite3_initialize() is not thread-safe until after their mutex system is initialized, i.e. where the current crashes are occurring. I'd link the code but there doesn't appear to be any online code viewer available for SQLite.

I'll let Brady or someone chime in for the final review.

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:63
> +    std::call_once(flag, []() {

You can omit the empty parameter list: [] { ... }

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:65
> +        // It should be safe to call this outside of std::call_once, since it is documented to be
> +        // completely threadsafe. But it is not threadsafe in practice. See bug #143245.

The comment doesn't lie, but IMO it would be better to find or file the corresponding bug upstream and link it here.
Comment 12 Michael Catanzaro 2015-09-01 08:27:23 PDT
(In reply to comment #11)
> > Source/WebCore/platform/sql/SQLiteDatabase.cpp:63
> > +    std::call_once(flag, []() {
> 
> You can omit the empty parameter list: [] { ... }

OK

> > Source/WebCore/platform/sql/SQLiteDatabase.cpp:65
> > +        // It should be safe to call this outside of std::call_once, since it is documented to be
> > +        // completely threadsafe. But it is not threadsafe in practice. See bug #143245.
> 
> The comment doesn't lie, but IMO it would be better to find or file the
> corresponding bug upstream and link it here.

Their bugtracker is restricted [1]. I will send a mail their list, and update the comment later once the mail appears and if a bug is ever filed.

[1] https://www.sqlite.org/src/wiki?name=Bug+Reports
Comment 13 Michael Catanzaro 2015-09-04 06:51:57 PDT
There will be no bug report to SQLite, as we are instructed to send mail to <sqlite-users@sqlite.org>, but this address just rejects any mail sent to it, rather than queuing them for moderation as they claim will occur [1]. They'll have to fix this if they want bug reports, I suppose.

[1] https://www.sqlite.org/src/wiki?name=Bug+Reports
Comment 14 Darin Adler 2015-09-04 07:23:20 PDT
(In reply to comment #13)
> There will be no bug report to SQLite, as we are instructed to send mail to
> <sqlite-users@sqlite.org>, but this address just rejects any mail sent to
> it, rather than queuing them for moderation as they claim will occur [1].
> They'll have to fix this if they want bug reports, I suppose.
> 
> [1] https://www.sqlite.org/src/wiki?name=Bug+Reports

We can’t find anyone willing to join their mailing list?
Comment 15 Darin Adler 2015-09-04 07:24:16 PDT
If someone writes the bug report, I’d be happy to join the sqlite-users mailing list and then send the bug report to the list.
Comment 16 Michael Catanzaro 2015-09-04 08:44:29 PDT
Thanks Darin. Your patience for mailing lists is greater than mine. This is the bug report I wrote:

Hi,

This email is regarding a WebKit bug, "Crash when
WebCore::SQLiteFileSystem::openDatabase is called from multiple
threads" [1], which seems to be caused by an issue in SQLite. In short,
we've noticed many applications that use WebKit crash when
sqlite3_initialize is called simultaneously in multiple threads
[2][3][4][5]; despite the fact that sqlite3_initialize is documented to
be thread-safe [6], it's not [7]. I am hoping to commit a workaround in
WebKit to call sqlite3_initialize manually exactly once before WebKit
uses sqlite, using std::once to avoid the thread safety problems. Based
on your documentation, it seems like it's a good idea for WebKit to
call sqlite3_initialize manually regardless, but the use of std::once
should not be needed.

I am just checking to see if this is perhaps already a known issue.

Michael

[1] https://bugs.webkit.org/show_bug.cgi?id=143245
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1201823
[3] https://bugzilla.redhat.com/show_bug.cgi?id=1217952
[4] https://bugzilla.redhat.com/show_bug.cgi?id=1228391
[5] https://bugzilla.redhat.com/show_bug.cgi?id=1207221
[6] https://sqlite.org/c3ref/initialize.html
[7] https://bugs.webkit.org/show_bug.cgi?id=143245#c11
Comment 17 Richard Hipp 2015-09-05 19:58:02 PDT
Tnx for the bug report.  This is the first time we've heard of this issue.

I have checked in a change to SQLite that might make sqlite3_initialize() closer to being threadsafe.  https://www.sqlite.org/src/info/11a9a786ec06403a

You would do well to continue to invoke sqlite3_initialize() using std::call_once() if that is working for you, though.
Comment 18 Michael Catanzaro 2015-09-06 07:36:47 PDT
Thanks Richard!
Comment 19 Michael Catanzaro 2015-09-06 07:48:08 PDT
Created attachment 260709 [details]
Patch
Comment 20 Michael Catanzaro 2015-09-08 14:29:30 PDT
Created attachment 260794 [details]
Patch
Comment 21 Michael Catanzaro 2015-09-08 18:38:44 PDT
Committed r189526: <http://trac.webkit.org/changeset/189526>
Comment 22 Michael Catanzaro 2015-09-13 16:14:28 PDT
Note for future reference: Scott Hess from Google subsequently argued on the mailing list that the fix to sqlite3_initialize is insufficient. The list archives are restricted to list members for whatever reason, so no link.