Crash under IDBServer::IDBConnectionToClient::identifier() const

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

3 WebCore: WebCore::IDBServer::IDBConnectionToClient::identifier() const <==
3 WebCore: WebCore::IDBResourceIdentifier::IDBResourceIdentifier(WebCore::IDBServer::IDBConnectionToClient const&)
3 WebCore: WebCore::IDBTransactionInfo::versionChange(WebCore::IDBServer::IDBConnectionToClient const&, WebCore::IDBDatabaseInfo const&, unsigned long long)
3 WebCore: WebCore::IDBServer::UniqueIDBDatabaseConnection::createVersionChangeTransaction(unsigned long long)
3 WebCore: WebCore::IDBServer::UniqueIDBDatabase::startVersionChangeTransaction()
3 WebCore: WebCore::IDBServer::UniqueIDBDatabase::handleCurrentOperation()
3 WebCore: WebCore::IDBServer::UniqueIDBDatabase::handleDatabaseOperations()
pruning: 2 WebCore: WebCore::IDBServer::UniqueIDBDatabase::operationAndTransactionTimerFired()
pruning: 1 WebCore: WebCore::IDBServer::UniqueIDBDatabase::executeNextDatabaseTaskReply()

[Sihui Liu]

<rdar://problem/48203102>

[Sihui Liu]

Created attachment 362455 [details]
Patch

[Geoffrey Garen]

Can you test this with an API test that allocates a WebView, starts a long-running IDB transaction, and then deallocates the WebView?

[Geoffrey Garen]

Comment on attachment 362455 [details]
Patch

r=me

Since this is a regression, and our initial attempt at a test did not crash, let's get this fix in as soon as we can. We can pursue a test case separately.

[Sihui Liu]

Created attachment 362678 [details]
Patch

[Ryosuke Niwa]

The patch is failing to build:

In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/unified-sources/UnifiedSource134.cpp:7:
In file included from ./Modules/indexeddb/server/MemoryObjectStore.cpp:39:
In file included from ./Modules/indexeddb/server/UniqueIDBDatabase.h:34:
In file included from ./Modules/indexeddb/server/ServerOpenDBRequest.h:30:
./Modules/indexeddb/server/IDBConnectionToClient.h:88:30: error: expected ';' at end of declaration list
    bool m_isClosed { false }
                             ^
                             ;

[Sihui Liu]

Created attachment 362699 [details]
Patch

[Geoffrey Garen]

Comment on attachment 362699 [details]
Patch

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

It seems like this patch is specific to requests to open a database. Is that sufficient? What about other database operations that might happen after a connection has closed?

Is there some central bottleneck (maybe handleCurrentOperation() or operationAndTransactionTimerFired() or handleDatabaseOperations()?) that can check for a closed connection, instead of checking at all relevant call sites?

> Source/WebCore/Modules/indexeddb/server/IDBConnectionToClient.h:82
> +    bool isConenctionClosed() { return m_isClosed; }

We usually name the getter to match the data member: bool isClosed() { return m_isClosed; }.

If you prefer "isConnectionClosed", you can name the data member that way too. But I think isClosed is better. The object is the connection, so putting "connection" in all its function names is redundant. (Relatedly, connectionToClientClosed() should probably be renamed to close() or setClosed().)

[Sihui Liu]

(In reply to Geoffrey Garen from comment #8)
> Comment on attachment 362699 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362699&action=review
> 
> It seems like this patch is specific to requests to open a database. Is that
> sufficient? What about other database operations that might happen after a
> connection has closed?
> 
> Is there some central bottleneck (maybe handleCurrentOperation() or
> operationAndTransactionTimerFired() or handleDatabaseOperations()?) that can
> check for a closed connection, instead of checking at all relevant call
> sites?
> 

My idea was to clear the requests of closed connection when the UniqueIDBDatabase was notified by UniqueIDBDatabaseConnection with connectionClosedFromClient. But it turned out UniqueIDBDatabase may being created without having UniqueIDBDatabaseConnection (UniqueIDBDatabase is opening its backingstore in background thread). So this didn't work.

handleDatabaseOperations() finds next available request and handleCurrentOperation() finds next available transaction, so it seems more reasonable to check on handleDatabaseOperations(). But when operationAndTransactionTimerFired() is called with non-null m_currentOpenDBRequest, it didn't call handleDatabaseOperations(). 

This patch should at least fix cases exposed by the crash reports: inside handleDatabaseOperations() and operationAndTransactionTimerFired().

Database operations correspond to transactions, so they will be taken care by the transaction abortion steps in UniqueIDBDatabase::connectionClosedFromClient.

> > Source/WebCore/Modules/indexeddb/server/IDBConnectionToClient.h:82
> > +    bool isConenctionClosed() { return m_isClosed; }
> 
> We usually name the getter to match the data member: bool isClosed() {
> return m_isClosed; }.
> 
> If you prefer "isConnectionClosed", you can name the data member that way
> too. But I think isClosed is better. The object is the connection, so
> putting "connection" in all its function names is redundant. (Relatedly,
> connectionToClientClosed() should probably be renamed to close() or
> setClosed().)
>

Okay.

[Sihui Liu]

Created attachment 362747 [details]
Patch

[Geoffrey Garen]

Comment on attachment 362747 [details]
Patch

I think you could write a helper function like this:

void clearStaleOpenDBRequests()
{
    if (m_currentOpenDBRequest && m_currentOpenDBRequest->connection().isClosed())
        m_currentOpenDBRequest = nullptr;

    while (!m_pendingOpenDBRequests.isEmpty() && m_pendingOpenDBRequests.first()->connection().isClosed())
        m_pendingOpenDBRequests.removeFirst();

    if (!m_currentOpenDBRequest && !m_pendingOpenDBRequests.isEmpty())
        m_currentOpenDBRequest = m_pendingOpenDBRequests.takeFirst();
}


Then you can call the function before the first use of m_currentOpenDBRequest inside handleDatabaseOperations() and operationAndTransactionTimerFired(). If you do that, you don't need to change any other code.

Also, I think there's a small bug in the current version of this patch, where operationAndTransactionTimerFired() forgets to set m_currentOpenDBRequest to nullptr if the connection is closed.

[Sihui Liu]

Created attachment 362765 [details]
Patch

[Sihui Liu]

(In reply to Geoffrey Garen from comment #11)
> Comment on attachment 362747 [details]
> Patch
> 
> I think you could write a helper function like this:
> 
> void clearStaleOpenDBRequests()
> {
>     if (m_currentOpenDBRequest &&
> m_currentOpenDBRequest->connection().isClosed())
>         m_currentOpenDBRequest = nullptr;
> 
>     while (!m_pendingOpenDBRequests.isEmpty() &&
> m_pendingOpenDBRequests.first()->connection().isClosed())
>         m_pendingOpenDBRequests.removeFirst();
> 
>     if (!m_currentOpenDBRequest && !m_pendingOpenDBRequests.isEmpty())
>         m_currentOpenDBRequest = m_pendingOpenDBRequests.takeFirst();
> }
> 
> 
> Then you can call the function before the first use of
> m_currentOpenDBRequest inside handleDatabaseOperations() and
> operationAndTransactionTimerFired(). If you do that, you don't need to
> change any other code.
> 
It seems we cannot use this helper function because it always sets m_currentOpenDBRequest if it is null. But in UniqueIDBDatabase::handleDatabaseOperations(), it's possible that we let m_currentOpenDBRequest stay null when there is m_versionChangeDatabaseConnection or m_versionChangeTransaction.

> Also, I think there's a small bug in the current version of this patch,
> where operationAndTransactionTimerFired() forgets to set
> m_currentOpenDBRequest to nullptr if the connection is closed.
>
True. Fix it by using an else in operationAndTransactionTimerFired().

[Geoffrey Garen]

Comment on attachment 362765 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 362765 [details]
Patch

Clearing flags on attachment: 362765

Committed r241967: <https://trac.webkit.org/changeset/241967>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.