Bug 224299 – Make WebIDBServer not block main thread during initialization

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

...

[Sihui Liu]

Created attachment 425434 [details]
Patch

[Geoffrey Garen]

Comment on attachment 425434 [details]
Patch

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

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:55
>  WebIDBServer::~WebIDBServer()

Does WebIDBServer do something to ensure that its WebCore::IDBServer::IDBServer thread has fully exited before we run this destructor? If not, WebCore::IDBServer::IDBServer may use-after-free m_serverLock.

I see code in close() that will post a task to destroy the WebCore::IDBServer::IDBServer. But I don't see us waiting on that task. And I don't know if we guarantee a close() before destruction. (If so, we should probably RELEASE_ASSERT that here.)

> Source/WebKitLegacy/Storage/InProcessIDBServer.h:135
> +    Lock m_serverLock;

This is maybe not correct, or at least it's a little scary -- because we passed a reference to m_serverLock to m_server, and this C++ declaration order means that m_serverLock's destructor will run before m_server's. So, for at least a time, m_server will hold a reference to a destructed m_serverLock.

Let's change the declaration order so that m_serverLock outlives m_server.

[Sihui Liu]

Comment on attachment 425434 [details]
Patch

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

>> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:55
>>  WebIDBServer::~WebIDBServer()
> 
> Does WebIDBServer do something to ensure that its WebCore::IDBServer::IDBServer thread has fully exited before we run this destructor? If not, WebCore::IDBServer::IDBServer may use-after-free m_serverLock.
> 
> I see code in close() that will post a task to destroy the WebCore::IDBServer::IDBServer. But I don't see us waiting on that task. And I don't know if we guarantee a close() before destruction. (If so, we should probably RELEASE_ASSERT that here.)

Yes, in close() it sets the callback of CrossThreadHandler, and the callback is the last thing to run before thread exits. 
The callback dispatches a function that should hold the last reference of WebIDBServer to main runloop, so WebIDBServer goes away after thread.

(I think we can simplify close() a bit. Will do a followup.)

>> Source/WebKitLegacy/Storage/InProcessIDBServer.h:135
>> +    Lock m_serverLock;
> 
> This is maybe not correct, or at least it's a little scary -- because we passed a reference to m_serverLock to m_server, and this C++ declaration order means that m_serverLock's destructor will run before m_server's. So, for at least a time, m_server will hold a reference to a destructed m_serverLock.
> 
> Let's change the declaration order so that m_serverLock outlives m_server.

Sure.

[Sihui Liu]

Created attachment 425466 [details]
Patch

[Geoffrey Garen]

Comment on attachment 425466 [details]
Patch

r=me

Would be nice to RELEASE_ASSERT(!m_server) in the WebIDBServer destructor, to prove that we have indeed called close().

Similary, would be nice to move the call to setCompletionCallback() into the WebIDBServer server constructor. That way, the lifetime is more obvious, and we always get object lifetime right even if we forget to call close().

[Sihui Liu]

Created attachment 425877 [details]
Patch for landing

[EWS]

Committed r275891 (236456@main): <https://commits.webkit.org/236456@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425877 [details].

[Radar WebKit Bug Importer]

<rdar://problem/76596644>