Bug 180518 – Crash in WebCore::SQLiteStatement::prepare() from ServiceWorker I/O Thread

• Component: Service Workers
• Status: NEW

[Daniel Bates]

In the EWS results (attachment #328671 [details]) after processing the patch on bug #180512, I noticed that the following tests are crashing in the ServiceWorker I/O Thread inside WebCore::SQLiteStatement::prepare():

http/tests/inspector/network/resource-response-service-worker.html
imported/w3c/web-platform-tests/streams/readable-byte-streams/brand-checks.serviceworker.https.html	

Paraphrasing the crash log for http/tests/inspector/network/resource-response-service-worker.html (both crashes have the same backtrace and exception code):

[[
Process:               com.apple.WebKit.Storage.Development [77629]
Path:                  /Volumes/VOLUME/*/WebKit.framework/Versions/A/XPCServices/com.apple.WebKit.Storage.xpc/Contents/MacOS/com.apple.WebKit.Storage.Development
Identifier:            com.apple.WebKit.Storage.Development
Version:               605+ (605.1.16+)
Code Type:             X86-64 (Native)
Parent Process:        ??? [1]
User ID:               501

Date/Time:             2017-12-06 18:18:26.347 -0800
OS Version:            Mac OS X 10.11.6 (15G17023)
Report Version:        11
Anonymous UUID:        18EE2525-DEB2-9EB7-627F-5FA60686285B

Time Awake Since Boot: 8200 seconds

System Integrity Protection: disabled

Crashed Thread:        3  ServiceWorker I/O Thread

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000018
Exception Note:        EXC_CORPSE_NOTIFY

Thread 3 Crashed:: ServiceWorker I/O Thread
0   com.apple.WebCore             	0x0000000105c17ec7 WebCore::SQLiteStatement::prepare() + 23 (atomic:879)
1   com.apple.WebCore             	0x0000000105c1794f WebCore::SQLiteDatabase::executeCommand(WTF::String const&) + 47 (SQLiteStatement.cpp:133)
2   com.apple.WebCore             	0x0000000105c1b061 WebCore::SQLiteTransaction::begin() + 129 (SQLiteTransaction.cpp:67)
3   com.apple.WebCore             	0x0000000105f86277 WebCore::RegistrationDatabase::doPushChanges(WTF::Vector<WebCore::ServiceWorkerContextData, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&&) + 55 (memory:2635)
4   com.apple.JavaScriptCore      	0x0000000109230f5f WTF::CrossThreadTaskHandler::taskRunLoop() + 127 (memory:2656)
5   com.apple.JavaScriptCore      	0x00000001092692a4 WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) + 228 (memory:2657)
6   com.apple.JavaScriptCore      	0x000000010926a899 WTF::wtfThreadEntryPoint(void*) + 9 (ThreadingPthreads.cpp:223)
7   libsystem_pthread.dylib       	0x00007fff951ed99d _pthread_body + 131
8   libsystem_pthread.dylib       	0x00007fff951ed91a _pthread_start + 168
9   libsystem_pthread.dylib       	0x00007fff951eb351 thread_start + 13
]]

[youenn fablet]

I also got the same crash in EWS bot in processing results of https://bugs.webkit.org/show_bug.cgi?id=179641

[Chris Dumez]

What makes sure m_database is destroyed on the background thread? I see this assertion  ASSERT(!m_database); in the ~RegistrationDatabase() destructor (which gets called on the main thread) but I do not see the code that takes care of clearing m_database on the BG thread.

[Brady Eidson]

(In reply to Chris Dumez from comment #2)
> What makes sure m_database is destroyed on the background thread? I see this
> assertion  ASSERT(!m_database); in the ~RegistrationDatabase() destructor
> (which gets called on the main thread) but I do not see the code that takes
> care of clearing m_database on the BG thread.

The RegistrationDatabase is never destroyed.

[Brady Eidson]

(In reply to Brady Eidson from comment #3)
> (In reply to Chris Dumez from comment #2)
> > What makes sure m_database is destroyed on the background thread? I see this
> > assertion  ASSERT(!m_database); in the ~RegistrationDatabase() destructor
> > (which gets called on the main thread) but I do not see the code that takes
> > care of clearing m_database on the BG thread.
> 
> The RegistrationDatabase is never destroyed.

Elaborating: Are SWServers ever destroyed?
If not, then RegistrationStores aren't, and RegistrationDatabases aren't.

[Brady Eidson]

I actually think what's happening is a pushChanges() is happening without an m_database

[Brady Eidson]

Indeed, there's no protection against database operations happening without a valid database. Adding that soon.

[Brady Eidson]

(Would be great to know why the DB isn't being created)

[Brady Eidson]

Created attachment 328833 [details]
Patch

[Chris Dumez]

Comment on attachment 328833 [details]
Patch

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

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:204
> +    if (!m_successfullyOpened) {

If you call pushChanges() right away after constructing the RegistrationDatabase, m_successfullyOpened may not have been set to true yet :/

[Brady Eidson]

(In reply to Chris Dumez from comment #9)
> Comment on attachment 328833 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328833&action=review
> 
> > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:204
> > +    if (!m_successfullyOpened) {
> 
> If you call pushChanges() right away after constructing the
> RegistrationDatabase, m_successfullyOpened may not have been set to true yet
> :/

Such a thing does not happen, though.

[Brady Eidson]

(In reply to Brady Eidson from comment #10)
> (In reply to Chris Dumez from comment #9)
> > Comment on attachment 328833 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=328833&action=review
> > 
> > > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:204
> > > +    if (!m_successfullyOpened) {
> > 
> > If you call pushChanges() right away after constructing the
> > RegistrationDatabase, m_successfullyOpened may not have been set to true yet
> > :/
> 
> Such a thing does not happen, though.

Correction: *should* not happen

[Chris Dumez]

Comment on attachment 328833 [details]
Patch

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

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:205
> +        LOG_ERROR("Attempt to push changes to SW registration database that is not open");

Should be RELEASE_LOG_ERROR.

[Brady Eidson]

Going with the one in 180587 for now, will revisit later. (I really need to get 180573 resolved)

[Chris Dumez]

Brady and I think the real underlying issue is https://bugs.webkit.org/show_bug.cgi?id=180589.