The spec says that the Database class should have a readTransaction() method. We should add bindings + other necessary support for this method to WebCore.
Created attachment 39018 [details] patch
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.
Created attachment 39023 [details] patch Same patch with minor fixes to V8 bindings (forgot to port them from my Chromium client).
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
(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.
Created attachment 39085 [details] patch
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).
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.
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.
Created attachment 39219 [details] patch Making it more obvious that SQLITE_CREATE_TEMP_* are not allowed in read-only transactions and private browsing mode.
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.
Created attachment 39231 [details] patch Addressed Dimitri's comments.
Comment on attachment 39231 [details] patch r=me.
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
It appears this does not build on the Mac in release mode. Sadly build logs are not yet available from the commit-bot.
(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.
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.
Comment on attachment 39285 [details] patch ookay.
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
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.
I jest about the media support hatred... but I do really look forward to having stable tests again. :)
Comment on attachment 39285 [details] patch Clearing flags on attachment: 39285 Committed r48227: <http://trac.webkit.org/changeset/48227>
All reviewed patches have been landed. Closing bug.