Bug 152698 – Modern IDB: Transactions from a previous page can leak forward to the next

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Modern IDB: Transactions from a previous page can leak forward to the next

Noticed this while exploring why storage/indexeddb/keypath-basics.html causes downstream tests to crash. And IDBTransaction from keypath-basics.html is still sending operations to the server even after DRT has navigated to the next test.

Yikes!

[Brady Eidson]

Fixing this *affects* various other tests with TestExpectations entries, but I'll leave their entries as-is for now.

I managed to write a test that reliably fails because of this bug and will include it with the patch, which is upcoming.

[Brady Eidson]

Created attachment 268268 [details]
Patch v1

[Alex Christensen]

Comment on attachment 268268 [details]
Patch v1

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

r=me once my question about the removed assertion is answered.

> Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:273
> +        if (transaction)

Can there be null entries in m_activeTransactions? This check might be unnecessary because we know id is a valid key with a value in m_activeTransactions.  But it's also good to check pointers before dereferencing them.

> Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:218
>      // Since we're aborting, this abortOnServerAndCancelRequests() operation should be the only
>      // in-progress operation, and it should be impossible to have queued any further operations.
> -    ASSERT(m_transactionOperationMap.size() == 1);
>      ASSERT(m_transactionOperationQueue.isEmpty());

This comment needs updating.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:-978
> -        ASSERT(&m_versionChangeTransaction->databaseConnection() == m_versionChangeDatabaseConnection);

I don't understand why this assertion is being removed.  Please justify.

[Brady Eidson]

(In reply to comment #3)
> Comment on attachment 268268 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268268&action=review
> 
> r=me once my question about the removed assertion is answered.
> 
> > Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:273
> > +        if (transaction)
> 
> Can there be null entries in m_activeTransactions? This check might be
> unnecessary because we know id is a valid key with a value in
> m_activeTransactions.  But it's also good to check pointers before
> dereferencing them.
> 

There cannot be a null entry, but calling stop() on one transaction might cause another to be removed from the set.

So we build up the initial list of known ids, but have to make sure the transaction is still there for each id we stop().

> > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:218
> >      // Since we're aborting, this abortOnServerAndCancelRequests() operation should be the only
> >      // in-progress operation, and it should be impossible to have queued any further operations.
> > -    ASSERT(m_transactionOperationMap.size() == 1);
> >      ASSERT(m_transactionOperationQueue.isEmpty());
> 
> This comment needs updating.

Yup.

> 
> > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:-978
> > -        ASSERT(&m_versionChangeTransaction->databaseConnection() == m_versionChangeDatabaseConnection);
> 
> I don't understand why this assertion is being removed.  Please justify.

Because m_versionChangeDatabaseConnection can now be null when this code is run.

I could change the assert to that effect.

[Brady Eidson]

Created attachment 268283 [details]
Patch for landing

[Brady Eidson]

(In reply to comment #4)
> (In reply to comment #3)
> > > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:-978
> > > -        ASSERT(&m_versionChangeTransaction->databaseConnection() == m_versionChangeDatabaseConnection);
> > 
> > I don't understand why this assertion is being removed.  Please justify.
> 
> Because m_versionChangeDatabaseConnection can now be null when this code is
> run.
> 
> I could change the assert to that effect.

In the patch for landing, I changed the assert to for the connections to be equal only if the version change connection is not nullptr.

[WebKit Commit Bot]

Comment on attachment 268283 [details]
Patch for landing

Clearing flags on attachment: 268283

Committed r194587: <http://trac.webkit.org/changeset/194587>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.