Bug 143245 – Crash when WebCore::SQLiteFileSystem::openDatabase is called from multiple threads

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Michael Catanzaro]

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

[Carlos Garcia Campos]

The question here is why there are two work queue threads for local storage if there's only one web context.

[Carlos Garcia Campos]

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.

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

hmm, Anders made the inspector process pool to be created lazily in r182447, that will definitely help for this problem.

[Michael Catanzaro]

Hm, turns out the crash can occur when openDatabase is being called from only one thread.

[Michael Catanzaro]

Ah, but another thread was calling LocalStorageDatabaseTracker::importOriginIdentifiers (backtrace in the downstream GNOME Builder bug).

[Michael Catanzaro]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 260100 [details]
Patch

[Michael Catanzaro]

Created attachment 260311 [details]
Patch

[Zan Dobersek]

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.

[Michael Catanzaro]

(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

[Michael Catanzaro]

There will be no bug report to SQLite, as we are instructed to send mail to <[e-mail]>, 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

[Darin Adler]

(In reply to comment #13)
> There will be no bug report to SQLite, as we are instructed to send mail to
> <[e-mail]>, 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?

[Darin Adler]

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.

[Michael Catanzaro]

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

[Richard Hipp]

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.

[Michael Catanzaro]

Thanks Richard!

[Michael Catanzaro]

Created attachment 260709 [details]
Patch

[Michael Catanzaro]

Created attachment 260794 [details]
Patch

[Michael Catanzaro]

Committed r189526: <http://trac.webkit.org/changeset/189526>

[Michael Catanzaro]

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.