Bug 28918 – Add support for Database.readTransaction()

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

[Dumitru Daniliuc]

The spec says that the Database class should have a readTransaction() method. We should add bindings + other necessary support for this method to WebCore.

[Dumitru Daniliuc]

Created attachment 39018 [details]
patch

[Michael Nordman]

Haven't looked thru the whole patch yet... but in the v8 custom bindings, CALLBACK_FUNC_DECL(DatabaseTransaction) has some early returns that are not v8::Undefined(). Looks like the new code eats those return values and always returns undefined.

[Dumitru Daniliuc]

Created attachment 39023 [details]
patch

Same patch with minor fixes to V8 bindings (forgot to port them from my Chromium client).

[Michael Nordman]

This looks pretty nice. So the method is available, and a flag is plumbed throughout, and the authorizer enforces readonly'ness. I take it you're saving allowing concurrent r/o transactions on the same db thread for a later patch?

http://www.sqlite.org/lang_reindex.html
It looks like REINDEX is a write operation, but I think its being allowed for readOnly transactions... is this what you intended and why?

What does the HTML5 draft say about the behavior of a readOnly transaction when a write operation is attempted? As coded, looks like the statement fails, but the transaction is left open and other statements can be executed in that transaction. Does that match the spec'd behavior? I can imagine another behavior, the transaction fails and is closed when a write operation is attempted... just checking.

7979     void transaction(PassRefPtr<SQLTransactionCallback> callback, PassRefPtr<SQLTransactionErrorCallback> errorCallback,
80                       PassRefPtr<VoidCallback> successCallback);
 80                      PassRefPtr<VoidCallback> successCallback, bool readOnly = false);

Are there any callsites that don't explicitly pass a value for the readOnly parameter? Consider removing the default param value.

BTW... have you seen the SAVEPOINT statement in newer versions of SQLite? Looks like support in sqlite3  for transaction nesting. Wondering if that capability should be reflected in the HTML5 api in some way?
http://www.sqlite.org/lang_savepoint.html

[Dumitru Daniliuc]

(In reply to comment #4)
> This looks pretty nice. So the method is available, and a flag is plumbed
> throughout, and the authorizer enforces readonly'ness. I take it you're saving
> allowing concurrent r/o transactions on the same db thread for a later patch?

Yes. The goal is to be able to allow multiple read transactions at the same time.

> http://www.sqlite.org/lang_reindex.html
> It looks like REINDEX is a write operation, but I think its being allowed for
> readOnly transactions... is this what you intended and why?

I wasn't sure what to do with REINDEX. On one hand, it seems to just recreate some existing indexes (and it doesn't seem to trigger any other "write" operations like DELETE, or UPDATE; at least not for the SQL statement I have in the test). On the other hand, it does look like a write operation.

I changed the authorizer to not allow REINDEX in read-only transactions.

> What does the HTML5 draft say about the behavior of a readOnly transaction when
> a write operation is attempted? As coded, looks like the statement fails, but
> the transaction is left open and other statements can be executed in that
> transaction. Does that match the spec'd behavior? I can imagine another
> behavior, the transaction fails and is closed when a write operation is
> attempted... just checking.

Here's what the spec says about statements that fail:

1. If the statement had an associated error callback, then queue a task to invoke that error callback with the SQLTransaction object and a newly constructed SQLError object that represents the error that caused these substeps to be run as the two arguments, respectively, and wait for the task to be run.
2. If the error callback returns false, then move on to the next statement, if any, or onto the next overall step otherwise.
3. Otherwise, the error callback did not return false, or there was no error callback. Jump to the last step in the overall steps.

So it seems to me like the read transactions in the test should've failed after the first statement with an error. I'm not sure why it didn't happen (at the first look the code seems to do what the spec says); I'm looking into it.

> 7979     void transaction(PassRefPtr<SQLTransactionCallback> callback,
> PassRefPtr<SQLTransactionErrorCallback> errorCallback,
> 80                       PassRefPtr<VoidCallback> successCallback);
>  80                      PassRefPtr<VoidCallback> successCallback, bool
> readOnly = false);
> 
> Are there any callsites that don't explicitly pass a value for the readOnly
> parameter? Consider removing the default param value.

done.

> BTW... have you seen the SAVEPOINT statement in newer versions of SQLite? Looksf that capability
> like support in sqlite3  for transaction nesting. Wondering i
> should be reflected in the HTML5 api in some way?
> http://www.sqlite.org/lang_savepoint.html

Michael and I talked a bit about this, and I think the conclusions were:
1. It's not in the SQLite version we currently use.
2. Doesn't seem to add anything new (could be replaced by multiple transactions?), but might be useful to app developers anyway.
3. Should probably talk to app developers first before proposing to add it to the spec.
4. Might be better to wait until all other issues are resolved and we have a more "final" version of the spec that's approved by everybody.

[Dumitru Daniliuc]

Created attachment 39085 [details]
patch

[Dumitru Daniliuc]

Created attachment 39098 [details]
patch

Updated the test to not fail transactions when a statement fails (used to have the same behavior before because of a bug in JSC and V8 that will be fixed in another bug).

[Michael Nordman]

With this change we're changing the semantics of what 'readOnly' currently means to the authorizer. 

The existing behavior is to support safari's private browsing mode and generally (but no exactly) constrains any side effects of a statement to not be visible outside of the connection to the database. Accordingly, creating and deleting 'temp' objects only visible on that connection are allowable for private browsing. The new readonly transaction semantics constrain any side effects to not be visible outside of a transaction, which also excludes creating and deleting 'temp' objects. There are also differences in how VIEWS are treated, for private browsing purposes these can be created and deleted, but in readonly transaction semantics these schema changes can't be made. 

Unless we can reconcile the two meanings of 'readOnly', we may need two separate flags.


91 static JSValue DatabaseTransactionHelper(... RefPtr<Database> database, bool readOnly)

I think Database* would be better.

104     executeStatement(tx, "DROP TABLE TestTempTable;", "SQLITE_DROP_TEMP_TABLE");
105     executeStatement(tx, "DROP TRIGGER TestTempTrigger;", "SQLITE_DROP_TEMP_TRIGGER");
106     executeStatement(tx, "DROP VIEW TestTempView;", "SQLITE_DROP_TEMP_VIEW");

These lines look like they intend to operate on TEMP objects, but I don't think they actually do.

[Dumitru Daniliuc]

Created attachment 39209 [details]
patch

Same patch, with resolved ChangeLog files.

(In reply to comment #8)
> With this change we're changing the semantics of what 'readOnly' currently
> means to the authorizer. 
> 
> The existing behavior is to support safari's private browsing mode and
> generally (but no exactly) constrains any side effects of a statement to not be
> visible outside of the connection to the database. Accordingly, creating and
> deleting 'temp' objects only visible on that connection are allowable for
> private browsing. The new readonly transaction semantics constrain any side
> effects to not be visible outside of a transaction, which also excludes
> creating and deleting 'temp' objects. There are also differences in how VIEWS
> are treated, for private browsing purposes these can be created and deleted,
> but in readonly transaction semantics these schema changes can't be made. 
> 
> Unless we can reconcile the two meanings of 'readOnly', we may need two
> separate flags.

I talked to Brady about this. The only differences were the REINDEX and (CREATE|DROP) [TEMP] VIEW commands. According to Brady, REINDEX was an omission and should've been fixed, and since VIEWs result in new temp files, they have no place in private browsing. Basically, as the flag (readOnlyMode) in the code suggests, private browsing should really be a read-only mode. So I believe the subsets of allowed operations in readTransaction() and private browsing are identical.

> 91 static JSValue DatabaseTransactionHelper(... RefPtr<Database> database, bool
> readOnly)
> 
> I think Database* would be better.

done.

> 104     executeStatement(tx, "DROP TABLE TestTempTable;",
> "SQLITE_DROP_TEMP_TABLE");
> 105     executeStatement(tx, "DROP TRIGGER TestTempTrigger;",
> "SQLITE_DROP_TEMP_TRIGGER");
> 106     executeStatement(tx, "DROP VIEW TestTempView;",
> "SQLITE_DROP_TEMP_VIEW");
> 
> These lines look like they intend to operate on TEMP objects, but I don't think
> they actually do.

They do, these entities are created using 'CREATE TEMP <something>'. The syntax for dropping TEMP tables/triggers/views is the same as the syntax for dropping permanent entities.

[Dumitru Daniliuc]

Created attachment 39219 [details]
patch

Making it more obvious that SQLITE_CREATE_TEMP_* are not allowed in read-only transactions and private browsing mode.

[Dimitri Glazkov (Google)]

Comment on attachment 39219 [details]
patch

These are all style nits. Overall, looks great!

> +        * bindings/js/JSDatabaseCustom.cpp:
> +        (WebCore::DatabaseTransactionHelper):
> +        (WebCore::JSDatabase::transaction):
> +        (WebCore::JSDatabase::readTransaction):
> +        * bindings/v8/custom/V8CustomBinding.h:
> +        * bindings/v8/custom/V8DatabaseCustom.cpp:
> +        (WebCore::DatabaseTransactionHelper):
> +        (WebCore::DatabaseReadTransaction):
> +        * storage/Database.cpp:
> +        (WebCore::Database::changeVersion):
> +        (WebCore::Database::transaction):
> +        * storage/Database.h:
> +        * storage/Database.idl:
> +        * storage/DatabaseAuthorizer.cpp:
> +        (WebCore::DatabaseAuthorizer::createView):
> +        (WebCore::DatabaseAuthorizer::createTempView):
> +        (WebCore::DatabaseAuthorizer::dropView):
> +        (WebCore::DatabaseAuthorizer::dropTempView):
> +        * storage/DatabaseAuthorizer.h:
> +        * storage/SQLTransaction.cpp:
> +        (WebCore::SQLTransaction::create):
> +        (WebCore::SQLTransaction::SQLTransaction):
> +        (WebCore::SQLTransaction::executeSQL):
> +        (WebCore::SQLTransaction::acquireLock):
> +        * storage/SQLTransaction.h:
> +        * storage/SQLTransactionCoordinator.cpp:
> +        (WebCore::SQLTransactionCoordinator::acquireLock):
> +        * storage/SQLTransactionCoordinator.h:

Would be good to have short descriptions in the ChangeLog.

> -JSValue JSDatabase::transaction(ExecState* exec, const ArgList& args)
> +static JSValue DatabaseTransactionHelper(ExecState* exec, const ArgList& args, Database* database, bool readOnly)

camelCase, and badName :)

How about createTransaction?

>  
> -CALLBACK_FUNC_DECL(DatabaseTransaction)
> +static v8::Handle<v8::Value> DatabaseTransactionHelper(const v8::Arguments& args, bool readOnly)

Ditto.

> +    m_transactionQueue.append(SQLTransaction::create(this, callback, errorCallback, successCallback, ChangeVersionWrapper::create(oldVersion, newVersion), false));

It's not obvious what "false" is for here. How about we just make readOnly a default param and omit the "false" here?

> +    static PassRefPtr<SQLTransaction> create(Database*, PassRefPtr<SQLTransactionCallback>, PassRefPtr<SQLTransactionErrorCallback>,
> +                                             PassRefPtr<VoidCallback>, PassRefPtr<SQLTransactionWrapper>, bool readOnly);

bool readOnly = false

> -void SQLTransactionCoordinator::acquireLock(SQLTransaction* transaction)
> +void SQLTransactionCoordinator::acquireLock(SQLTransaction* transaction, bool)

use UNUSED_PARAM here, bool by itself is lonely.

> +function runTest() {

brace on new line.

[Dumitru Daniliuc]

Created attachment 39231 [details]
patch

Addressed Dimitri's comments.

[Dimitri Glazkov (Google)]

Comment on attachment 39231 [details]
patch

r=me.

[Eric Seidel (no email)]

Comment on attachment 39231 [details]
patch

Rejecting patch 39231 from commit-queue.

This patch will require manual commit. WebKitTools/Scripts/build-webkit failed with exit code 1

[Eric Seidel (no email)]

It appears this does not build on the Mac in release mode.  Sadly build logs are not yet available from the commit-bot.

[Dumitru Daniliuc]

(In reply to comment #15)
> It appears this does not build on the Mac in release mode.  Sadly build logs
> are not yet available from the commit-bot.

I'll upload another version once I fix it for Mac.

[Dumitru Daniliuc]

Created attachment 39285 [details]
patch

Mac complained that a few named parameters were not used. Should be fixed now, I managed to successfully build WebKit on Mac in release mode with this patch patched in.

[Dimitri Glazkov (Google)]

Comment on attachment 39285 [details]
patch

ookay.

[WebKit Commit Bot]

Comment on attachment 39285 [details]
patch

Rejecting patch 39285 from commit-queue.

This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Eric Seidel (no email)]

Comment on attachment 39285 [details]
patch

Have I mentioned I'm starting to hate our media support? ;)
compositing/self-painting-layers.html -> crashed
You've been bitten by bug 28845.

[Eric Seidel (no email)]

I jest about the media support hatred... but I do really look forward to having stable tests again. :)

[WebKit Commit Bot]

Comment on attachment 39285 [details]
patch

Clearing flags on attachment: 39285

Committed r48227: <http://trac.webkit.org/changeset/48227>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.