Bug 227771 – Return error for statement if the transaction it belongs to is rolled back

• Component: WebCore Misc.
• Status: NEW

[Sihui Liu]

...

[Sihui Liu]

Created attachment 433080 [details]
Patch

[Sihui Liu]

Created attachment 433125 [details]
Patch

[Sihui Liu]

Created attachment 433156 [details]
Patch

[Sihui Liu]

Created attachment 433196 [details]
Patch

[Sihui Liu]

Created attachment 433200 [details]
Patch

[Chris Dumez]

Comment on attachment 433200 [details]
Patch

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

I don't really understand the purpose of this patch and there is no test to show the usefulness either. Here are some random comments though.

> Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:-209
> -        m_sqliteTransaction->stop();

Why is this no longer stopping the transaction? I am assuming it is not needed because the destructor already takes care of stopping the transaction? Either way, I think a comment in the changelog for this function would be helpful understand such changes.

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:739
> +    return !m_writeTransactionInProgress || !m_writeTransactionInProgress->rolledBackBySQLite();

I don't understand this logic. So if `!m_writeTransactionInProgress` (meaning there is no write transaction in progress), then I have a valid transaction?

I would have expected:
return m_writeTransactionInProgress && !m_writeTransactionInProgress->rolledBackBySQLite();

Either there is a bug or at the very least, this code is confusing.

> Source/WebCore/platform/sql/SQLiteDatabase.h:163
> +    SQLiteTransaction* writeTransactionInProgress() const { return m_writeTransactionInProgress; }

Having a getter start with "write" seems a bit confusing to me.

> Source/WebCore/platform/sql/SQLiteDatabase.h:179
> +    SQLiteTransaction* m_writeTransactionInProgress { nullptr };

We store store raw pointers as data members anymore. This is a security bug waiting to happen. If you really need a pointer, it should probably be a WeakPtr.

> Source/WebCore/platform/sql/SQLiteTransaction.h:47
> +    bool rolledBackBySQLite() const;

How is this new name a progression? Seems to go against WebKit coding style that states we should use prefixes for booleans.

[Chris Dumez]

Comment on attachment 433200 [details]
Patch

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

>> Source/WebCore/platform/sql/SQLiteDatabase.h:179
>> +    SQLiteTransaction* m_writeTransactionInProgress { nullptr };
> 
> We store store raw pointers as data members anymore. This is a security bug waiting to happen. If you really need a pointer, it should probably be a WeakPtr.

I meant: we avoid storing raw pointers as data members nowadays.

[Sihui Liu]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 433200 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433200&action=review
> 
> I don't really understand the purpose of this patch and there is no test to
> show the usefulness either. Here are some random comments though.

For a explicit transaction, we usually begin the transaction, run some statements, and commit/abort the transaction. Currently we rely on the caller of SQLiteTransaction to track the state. If the caller fails to check the state, the statements may run as separate transactions . This patch aims to ensure the statements inside a write transaction are executed together. It does not handle read transactions as the consequences are not as bad if read statements are run separately.

This will help us to implement aborting all SQLiteTransactions synchronously on suspension: all subsequent statements in the same transaction scope will fail.

> 
> > Source/WebCore/Modules/webdatabase/SQLTransaction.cpp:-209
> > -        m_sqliteTransaction->stop();
> 
> Why is this no longer stopping the transaction? I am assuming it is not
> needed because the destructor already takes care of stopping the
> transaction? Either way, I think a comment in the changelog for this
> function would be helpful understand such changes.

Will add.

> 
> > Source/WebCore/platform/sql/SQLiteDatabase.cpp:739
> > +    return !m_writeTransactionInProgress || !m_writeTransactionInProgress->rolledBackBySQLite();
> 
> I don't understand this logic. So if `!m_writeTransactionInProgress`
> (meaning there is no write transaction in progress), then I have a valid
> transaction?
> 
> I would have expected:
> return m_writeTransactionInProgress &&
> !m_writeTransactionInProgress->rolledBackBySQLite();
> 
> Either there is a bug or at the very least, this code is confusing.

This is for checking in step(); the statement cannot be stepped if you are in a state where you begin a write transaction and the transaction is invalid.
The statement can be executed if you don't start explicit transaction, or it's a read transaction.

> 
> > Source/WebCore/platform/sql/SQLiteDatabase.h:163
> > +    SQLiteTransaction* writeTransactionInProgress() const { return m_writeTransactionInProgress; }
> 
> Having a getter start with "write" seems a bit confusing to me.

Will rename it to hasWriteTransactionInProgress.

> 
> > Source/WebCore/platform/sql/SQLiteDatabase.h:179
> > +    SQLiteTransaction* m_writeTransactionInProgress { nullptr };
> 
> We store store raw pointers as data members anymore. This is a security bug
> waiting to happen. If you really need a pointer, it should probably be a
> WeakPtr.

Will update.

> 
> > Source/WebCore/platform/sql/SQLiteTransaction.h:47
> > +    bool rolledBackBySQLite() const;
> 
> How is this new name a progression? Seems to go against WebKit coding style
> that states we should use prefixes for booleans.

Will revert.

[Sihui Liu]

Created attachment 433230 [details]
Patch

[Sihui Liu]

Created attachment 433231 [details]
Patch

[Sihui Liu]

Created attachment 433232 [details]
Patch

[Sihui Liu]

Created attachment 433465 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/80596229>