Bug 150148 – Modern IDB: Add basic transaction aborting

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

[Brady Eidson]

Modern IDB: Add basic transaction aborting

The only transaction that can be created right now is the version change transaction, and the only thing it can change right now is the version itself. So let's add aborting of the version change.

[Brady Eidson]

Created attachment 263696 [details]
Patch v1

[Brady Eidson]

Created attachment 263699 [details]
Patch v1

[Alex Christensen]

Comment on attachment 263696 [details]
Patch v1

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

Looks mostly good.  A few comments.  Merge conflict is probably in project.pbxproj
Also, with this new MemoryIDBBackingStore stuff, where does it actually store the information?  I see calls to commit, but I'm not sure where they go.

> Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:204
> +    ASSERT(m_activeTransactions.contains(transaction.info().identifier())
> +        || m_committingTransactions.contains(transaction.info().identifier())
> +        || m_abortingTransactions.contains(transaction.info().identifier()));

It should be in only one of these sets, right?  Could you assert that the sum is one?

> Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:55
>      , m_operationTimer(*this, &IDBTransaction::operationTimerFired)
>  
>  {
> +    m_activationTimer = std::make_unique<Timer>(*this, &IDBTransaction::activationTimerFired);

Could you initialize m_activationTimer like you do m_operationTimer?

> Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:105
> +    if (isFinishedOrFinishing()) {
> +        ec = INVALID_STATE_ERR;

How would this be reached?  Can it be tested?

> Source/WebCore/Modules/indexeddb/shared/IDBError.cpp:69
> +        static NeverDestroyed<String> entry = ASCIILiteral("Operation was called on an object on which it is not allowed or at a time when it is not allowed.");

Should these errors be localized?  Where do we see these errors?  There aren't any tests that run into errors.

[Alex Christensen]

Comment on attachment 263699 [details]
Patch v1

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

> Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.h:40
> +class MemoryIDBBackingStore;
> +
> +class MemoryBackingStoreTransaction {
> +    friend std::unique_ptr<MemoryBackingStoreTransaction> std::make_unique<MemoryBackingStoreTransaction>(MemoryIDBBackingStore&, const IDBTransactionInfo&);

It looks like the EFL EWS wants WebCore::IDBTransactionInfo.  I'm not sure why it doesn't like MemoryIDBBackingStore.  It's declared right there.

[Brady Eidson]

The EFL build failure appears to be yet-another-disappointing compiler limitation that clang handles fine. :(

[Brady Eidson]

(In reply to comment #5)
> The EFL build failure appears to be yet-another-disappointing compiler
> limitation that clang handles fine. :(

(Which will likely be fixable by fully qualifying WebCore::IDBTransactionInfo)

[Brady Eidson]

I didn't even see that Alex had taken a look, including noting the WebCore:: qualification.  *sigh*

[Brady Eidson]

(In reply to comment #3)
> Comment on attachment 263696 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263696&action=review
> 
> Looks mostly good.  A few comments.  Merge conflict is probably in
> project.pbxproj
> Also, with this new MemoryIDBBackingStore stuff, where does it actually
> store the information?  I see calls to commit, but I'm not sure where they
> go.

No information is stored yet.
Iterative process.

Important new addition here is the backing store transaction, which *does* do something interesting on abort.

> 
> > Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:204
> > +    ASSERT(m_activeTransactions.contains(transaction.info().identifier())
> > +        || m_committingTransactions.contains(transaction.info().identifier())
> > +        || m_abortingTransactions.contains(transaction.info().identifier()));
> 
> It should be in only one of these sets, right?  Could you assert that the
> sum is one?

You just requested an #ifndef NDEBUG block of actual code for a very simple assertion.

Remember that you did this to yourself.

> 
> > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:55
> >      , m_operationTimer(*this, &IDBTransaction::operationTimerFired)
> >  
> >  {
> > +    m_activationTimer = std::make_unique<Timer>(*this, &IDBTransaction::activationTimerFired);
> 
> Could you initialize m_activationTimer like you do m_operationTimer?

m_operationTimer exists for the lifetime of the object.
m_activationTimer is a one-off (note it deletes itself once it's fired)

> 
> > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:105
> > +    if (isFinishedOrFinishing()) {
> > +        ec = INVALID_STATE_ERR;
> 
> How would this be reached?

A transaction that has already being aborted being asked to abort again.

> Can it be tested?

Yes, by doing the above.

> 
> > Source/WebCore/Modules/indexeddb/shared/IDBError.cpp:69
> > +        static NeverDestroyed<String> entry = ASCIILiteral("Operation was called on an object on which it is not allowed or at a time when it is not allowed.");
> 
> Should these errors be localized?  

No, we don't localize error strings that are only meant for developers in the console that are explicitly called for in the spec.

> Where do we see these errors?  There aren't any tests that run into errors.

Any time a test runs into errors they can get the description.
We do have tests that run into errors, though they're testing other things and not the contents of the errors.

Now's not quite the right time to try testing all the descriptions because not all of them can be hit yet.
Good idea to test them all - https://bugs.webkit.org/show_bug.cgi?id=150402

[Brady Eidson]

Created attachment 263710 [details]
Patch v2

[Darin Adler]

Comment on attachment 263710 [details]
Patch v2

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

> Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.h:40
> +    friend std::unique_ptr<MemoryBackingStoreTransaction> std::make_unique<MemoryBackingStoreTransaction>(MemoryIDBBackingStore&, const WebCore::IDBTransactionInfo&);

EFL bot build failure:

../../Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.h:40:107: error: 'MemoryIDBBackingStore' has not been declared
     friend std::unique_ptr<MemoryBackingStoreTransaction> std::make_unique<MemoryBackingStoreTransaction>(MemoryIDBBackingStore&, const WebCore::IDBTransactionInfo&);

Strange because I see a forward declaration of this class two lines above!

[Brady Eidson]

Created attachment 263712 [details]
Patch v3 - Try to fix EFL again.

[Brady Eidson]

Created attachment 263713 [details]
Patch v4 - Try to fix EFL again

[Brady Eidson]

Created attachment 263716 [details]
Patch v5 - `git add` fail

[Brady Eidson]

https://trac.webkit.org/changeset/191400