Indexed DB: More exception precedence tests

Indexed DB: More exception precedence tests

For methods previously lacking such tests, add tests that cover which
exception is thrown when a method is called with two invalid
conditions at the same time.

R=pwnall@chromium.org
BUG=597303
Committed: https://crrev.com/1998b62c571a941c26fdc8bd1cda5fe2193d21d4
Cr-Commit-Position: refs/heads/master@{#425785}

[jsbell, 2016-10-13 19:49:22]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 19:49:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2016-10-13 19:53:17]

pwnall@ - please take a look?

Intent is to land these here, then upstream to WPT.

I need to check in FF nightly; everything should pass according to code
inspection but (1) continuePrimaryKey isn't in FF stable yet and (2) the
create/deleteObjectStore methods appear to fail.

[commit-bot: I haz the power, 2016-10-13 20:41:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 20:41:08]

Dry run: This issue passed the CQ dry run.

[pwnall, 2016-10-14 07:57:05]

Thank you for writing these super-tedious tests!

To add insult to the injury, now you'll have to explain to me the subtlety
behind pin_transaction,  and read my long comments.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-advance-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-advance-exception-order.html:5:
<script src="../../resources/testharness.js"></script>
I think it'd be nice to add author information, to make it easier to track these
tests when they come back to us from WPT. Some examples:
*
https://github.com/w3c/web-platform-tests/blob/56ea52b8668ceee63ec196c59dd11a...
*
https://github.com/w3c/web-platform-tests/blob/56ea52b8668ceee63ec196c59dd11a...

If you disagree, please teach me why, so I can avoid the work in the future :D

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-advance-exception-order.html:18:
const finish = pin_transaction(store);
I don't understand the necessity of the pin_transaction / setTimeout combination
here. It seems to me that we want to trigger two error conditions, where the 2nd
one is that the transaction is inactive.  It seems to me that the transaction
would be inactive with or without pin_transaction. pin_transaction decides if
the transaction is still running or not, which doesn't seem to matter for this
test.

FWIW: I tried commenting out this line and the finish() call. The test still
passed in Chrome Canary + FF Nightly. I changed the cursor.advance() call, and
got TransactionInactiveError, as expected. So, this doesn't seem necessary.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-advance-exception-order.html:41:
const finish = pin_transaction(store);
Same as above, except I commented out setTimeout to make sure that the test
works as intended.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-advance-exception-order.html:75:
const finish = pin_transaction(store);
Same as above.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-advance-exception-order.html:82:
cursor.continue();
Is there any value in using advance() instead of continue() here, to reduce the
number of concepts used in this test?

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-delete-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-delete-exception-order.html:66:
cursor.continue();
I think this continue() call will get you in the InvalidStateError #1 case. How
about removing it?

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-update-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-update-exception-order.html:66:
cursor.continue();
I think this continue() call also gets you in the InvalidStateError #1 case.
Please remove?

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbdatabase-createObjectStore-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbdatabase-createObjectStore-exception-order.html:19:
setTimeout(t.step_func(() => {
This alternative seems easier to reason about:

indexeddb_test(
  (t, db, txn) => {
    db.createObjectStore('s');

    txn.abort();
    setTimeout(t.step_func(() => {
      assert_throws(
        'InvalidStateError', () => { db.createObjectStore('s2'); },
        '"running an upgrade transaction" check (InvalidStateError) ' +
        'should precede "not active" check (TransactionInactiveError)');

      t.done();
    }), 0);
  },
  (t, db) => {},
  'IDBDatabase.createObjectStore exception order: ' +
  'InvalidStateError vs. TransactionInactiveError'
);

It passes in Chrome Canary and FF Nightly. Commenting out the setTimeout
triggers the TransactionInactiveError. WDYT?

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbdatabase-createObjectStore-exception-order.html:39:
assert_throws(
Firefox Nightly 52.0a1 (2016-10-13) fails this check.

I think Firefox conflates active transactions with running transactions. It
seems like they have a TLS variable indicating the currently running transaction
for a thread [1]. Their "get the database's currently running transaction"
(needed for Step 2 in createObjectStore's spec) seems to be implemented as "get
the thread's current transaction and check that it belongs to the database" [2].
The current transaction TLS variable is managed by an AutoSetCurrentTransaction
[3] class, which in turn is used by a ResultHelper [4] class. Last, I think
ResultHelper is only used when a request result is created and an event is
dispatched. This assertion is a bit hard to check, because it requires going
through all the code snippets that create ResultHelpers [5], but I'm reasonably
sure that it's true, based on some spot-checking.

On a brighter note,  I was able to get the desired behavior for this test with
the following changes:

testharness-helpers.js:10:
                upgrade_func(t, open_request.result, open_request.transaction);

idbdatabase-createObjectStore-exception-order.html: (comments left to emphasize
diff)
indexeddb_test(
  (t, db, txn) => {
    const store = db.createObjectStore('s');
    //const finish = pin_transaction(store);

    txn.abort();
    //setTimeout(t.step_func(() => {
      assert_throws(
        'TransactionInactiveError',
        () => { db.createObjectStore('s2', {keyPath: '-invalid-'}); },
        '"not active" check (TransactionInactiveError) should precede ' +
        '"valid key path" check (SyntaxError)');
      //finish();
      t.done();
    //}), 0);
  },
  (t, db) => {},
  'IDBDatabase.createObjectStore exception order: ' +
  'TransactionInactiveError vs. SyntaxError'
);

My approach side-steps Firefox's bug by testing for the currently running
transaction synchronously inside the onupgradeneeded handler.

FWIW, I tested my idea in both Chrome Canary and FF Nightly. The code above
passes. Removing the txn.abort() call yields a SyntaxError.


In order to get Firefox to pass a test that uses transaction pinning, I think
that that the easiest way forward may be to collapse the InvalidStateError and
TransactionInactiveError cases to a single exception type.


[1]
https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBTransaction.c...
[2]
https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBDatabase.cpp#423
[3]
https://dxr.mozilla.org/mozilla-central/search?q=%2Bfunction-ref%3A%22mozilla...
[4]
https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22mozilla%3A%3...
[5]
https://dxr.mozilla.org/mozilla-central/search?q=%2Btype-ref%3A%22mozilla%3A%...

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbdatabase-deleteObjectStore-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbdatabase-deleteObjectStore-exception-order.html:19:
setTimeout(t.step_func(() => {
Same concerns as for the first test in createObjectStore apply. Proposed
alternative:

indexeddb_test(
  (t, db, txn) => {
    db.createObjectStore('s');

    txn.abort();
    setTimeout(t.step_func(() => {
      assert_throws(
        'InvalidStateError', () => { db.deleteObjectStore('s'); },
        '"running an upgrade transaction" check (InvalidStateError) ' +
        'should precede "not active" check (TransactionInactiveError)');

      t.done();
    }), 0);
  },
  (t, db) => {},
  'IDBDatabase.deleteObjectStore exception order: ' +
  'InvalidStateError vs. TransactionInactiveError'
);

Tested in Chrome Canary / FF Nightly. Commenting out setTimeout yields
TransactionInactiveError.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbdatabase-deleteObjectStore-exception-order.html:39:
assert_throws(
Firefox Nightly 52.0a1 (2016-10-13) fails this check.

I think this comes down to the same problem of the "currently running"
transaction. Proposed fix:

indexeddb_test(
  (t, db, txn) => {
    const store = db.createObjectStore('s');
    //const finish = pin_transaction(store);

    txn.abort();
    //setTimeout(t.step_func(() => {
      assert_throws(
        'TransactionInactiveError', () => { db.deleteObjectStore('s2'); },
        '"not active" check (TransactionInactiveError) should precede ' +
        '"name in database" check (NotFoundError)');
      //finish();
      t.done();
    //}), 0);
  },
  (t, db) => {},
  'IDBDatabase.deleteObjectStore exception order: ' +
  'TransactionInactiveError vs. NotFoundError'
);

Same testing as before. Commenting out txn.abort() yields NotFoundError.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbdatabase-deleteObjectStore-exception-order.html:40:
'TransactionInactiveError', () => { db.deleteObjectStore('s'); },
I'm pretty sure we want to try to delete s2 here, in order to trigger a
NotFoundError.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbindex-query-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbindex-query-exception-order.html:28:
const finish = pin_transaction(store);
I also don't understand why pinning is necessary. I ran the test with this line
and the finish line commented out, and it passed in Chrome Canary / FF Nightly.
After commenting out the deleteIndex() call, I got TransactionInactiveError.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbindex-query-exception-order.html:56:
const finish = pin_transaction(store);
Same comment as above, except this time I commented out setTimeout and got
DataError.

I have a hunch that I'll observe the same effects in the files below, so I'll
stop making this comment. I assume that I'll have a understanding of what's
going on after I see your responses.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-add-put-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-add-put-exception-order.html:40:
const tx = db.transaction('s');
Do you think it's worth adding 'readonly' here, to emphasize the transaction
mode?

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-clear-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-clear-exception-order.html:38:
const tx = db.transaction('s');
Consider adding 'readonly' to emphasize the transaction mode?

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-delete-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-delete-exception-order.html:38:
const tx = db.transaction('s');
'readonly'?

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-delete-exception-order.html:61:
const tx = db.transaction('s');
'readonly'?

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-deleteIndex-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-deleteIndex-exception-order.html:31:
'InvalidStateError vs. TransactionInactiveError #1'
Should this be InvalidStateError #1 vs TransactionInactiveError?

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-deleteIndex-exception-order.html:55:
'InvalidStateError vs. TransactionInactiveError #2'
Should this be InvalidStateError #2 vs TransactionInactiveError?

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/resources/testharness-helpers.js
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/testharness-helpers.js:10:
upgrade_func(t, open_request.result);
This is where I added the open_request.transaction argument. It shouldn't hurt
the other tests, and having access to the transaction might be handy.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/testharness-helpers.js:43:
function pin_transaction(store) {
The docs do state that the point of it is to keep the transaction running. I
think they should also talk about the subtlety that the pinned transaction isn't
necessarily active while it's running. Specifically, I think that transactions
are only active right after they are created and while their event handlers run,
because of step 1 in [1],  step 4 in [2] and step 4 in [3].

[1] https://w3c.github.io/IndexedDB/#transaction-lifetime
[2] https://w3c.github.io/IndexedDB/#fire-success-event
[3] https://w3c.github.io/IndexedDB/#fire-error-event

If I understand the code correctly, it seems that pin_transaction ensures that
implementations can distinguish between alive but inactive transactions and
finished transactions. However, many of the exception conditions checked above
seem to only care about whether an active transaction exists or not, so the
distinction may be moot. In that case, using pin_transaction seems to make the
test harder to reason about.

Specifically, I think that only createObjectStore and deleteObjectStore
distinguish between running and active transactions, because they reference the
database's currently running versionchange transaction. It seems like pinning is
unnecessary in the other tests, and hits a Firefox bug in those two tests.

[jsbell, 2016-10-17 19:41:28]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 19:41:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2016-10-17 19:42:04]

Thanks for the VERY thorough review, and diving into the FF behavior.

Please take another look?

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-advance-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-advance-exception-order.html:5:
<script src="../../resources/testharness.js"></script>
On 2016/10/14 07:57:04, pwnall wrote:
> I think it'd be nice to add author information, to make it easier to track
these
> tests when they come back to us from WPT. Some examples:
> *
>
https://github.com/w3c/web-platform-tests/blob/56ea52b8668ceee63ec196c59dd11a...
> *
>
https://github.com/w3c/web-platform-tests/blob/56ea52b8668ceee63ec196c59dd11a...
> 
> If you disagree, please teach me why, so I can avoid the work in the future :D

Not something we'd normally do for blink and not a requirement for wpt either;
I'm not sure it adds benefit over easily reviewable version history.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-advance-exception-order.html:18:
const finish = pin_transaction(store);
On 2016/10/14 07:57:04, pwnall wrote:
> I don't understand the necessity of the pin_transaction / setTimeout
combination
> here. It seems to me that we want to trigger two error conditions, where the
2nd
> one is that the transaction is inactive.  It seems to me that the transaction
> would be inactive with or without pin_transaction. pin_transaction decides if
> the transaction is still running or not, which doesn't seem to matter for this
> test.

You're right - it doesn't matter for this test. Without pinning here the
transaction could be inactive for two reasons: it's still running but it's not
in a callback, or it's finished. That distinction doesn't matter for this test.
I'll simplify where possible.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-advance-exception-order.html:41:
const finish = pin_transaction(store);
On 2016/10/14 07:57:04, pwnall wrote:
> Same as above, except I commented out setTimeout to make sure that the test
> works as intended.

Done.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-advance-exception-order.html:75:
const finish = pin_transaction(store);
On 2016/10/14 07:57:04, pwnall wrote:
> Same as above.

Done.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-advance-exception-order.html:82:
cursor.continue();
On 2016/10/14 07:57:04, pwnall wrote:
> Is there any value in using advance() instead of continue() here, to reduce
the
> number of concepts used in this test?

Sure, switched. (I was considering `continue()` to be more primitive, but you're
right.)

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-delete-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-delete-exception-order.html:66:
cursor.continue();
On 2016/10/14 07:57:04, pwnall wrote:
> I think this continue() call will get you in the InvalidStateError #1 case.
How
> about removing it?

You're right! Removed.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-update-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbcursor-update-exception-order.html:66:
cursor.continue();
On 2016/10/14 07:57:04, pwnall wrote:
> I think this continue() call also gets you in the InvalidStateError #1 case.
> Please remove?

Done.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbdatabase-createObjectStore-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbdatabase-createObjectStore-exception-order.html:19:
setTimeout(t.step_func(() => {
Done, w/ slight tweaks.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbdatabase-createObjectStore-exception-order.html:39:
assert_throws(
Awesome investigation. Done, w/ slight tweaks.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbdatabase-deleteObjectStore-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbdatabase-deleteObjectStore-exception-order.html:19:
setTimeout(t.step_func(() => {
Done, w/ tweaks.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbdatabase-deleteObjectStore-exception-order.html:39:
assert_throws(
Done, w/ tweaks.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbdatabase-deleteObjectStore-exception-order.html:40:
'TransactionInactiveError', () => { db.deleteObjectStore('s'); },
On 2016/10/14 07:57:04, pwnall wrote:
> I'm pretty sure we want to try to delete s2 here, in order to trigger a
> NotFoundError.

Done.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbindex-query-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbindex-query-exception-order.html:28:
const finish = pin_transaction(store);
On 2016/10/14 07:57:05, pwnall wrote:
> I also don't understand why pinning is necessary. I ran the test with this
line
> and the finish line commented out, and it passed in Chrome Canary / FF
Nightly.
> After commenting out the deleteIndex() call, I got TransactionInactiveError.

Done.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbindex-query-exception-order.html:56:
const finish = pin_transaction(store);
On 2016/10/14 07:57:05, pwnall wrote:
> Same comment as above, except this time I commented out setTimeout and got
> DataError.
> 
> I have a hunch that I'll observe the same effects in the files below, so I'll
> stop making this comment. I assume that I'll have a understanding of what's
> going on after I see your responses. 

Done.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-add-put-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-add-put-exception-order.html:40:
const tx = db.transaction('s');
On 2016/10/14 07:57:05, pwnall wrote:
> Do you think it's worth adding 'readonly' here, to emphasize the transaction
> mode?

Done.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-clear-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-clear-exception-order.html:38:
const tx = db.transaction('s');
On 2016/10/14 07:57:05, pwnall wrote:
> Consider adding 'readonly' to emphasize the transaction mode?

Done.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-delete-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-delete-exception-order.html:38:
const tx = db.transaction('s');
On 2016/10/14 07:57:05, pwnall wrote:
> 'readonly'?

Done.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-delete-exception-order.html:61:
const tx = db.transaction('s');
On 2016/10/14 07:57:05, pwnall wrote:
> 'readonly'?

Done.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-deleteIndex-exception-order.html
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-deleteIndex-exception-order.html:31:
'InvalidStateError vs. TransactionInactiveError #1'
On 2016/10/14 07:57:05, pwnall wrote:
> Should this be InvalidStateError #1 vs TransactionInactiveError?

Done.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/idbobjectstore-deleteIndex-exception-order.html:55:
'InvalidStateError vs. TransactionInactiveError #2'
On 2016/10/14 07:57:05, pwnall wrote:
> Should this be InvalidStateError #2 vs TransactionInactiveError?

Done.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/storage/indexeddb/resources/testharness-helpers.js
(right):

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/testharness-helpers.js:10:
upgrade_func(t, open_request.result);
On 2016/10/14 07:57:05, pwnall wrote:
> This is where I added the open_request.transaction argument. It shouldn't hurt
> the other tests, and having access to the transaction might be handy.

Done.

https://codereview.chromium.org/2415203002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/testharness-helpers.js:43:
function pin_transaction(store) {
On 2016/10/14 07:57:05, pwnall wrote:
> The docs do state that the point of it is to keep the transaction running. I
> think they should also talk about the subtlety that the pinned transaction
isn't
> necessarily active while it's running. 

Agreed, but...

> Specifically, I think that only createObjectStore and deleteObjectStore
> distinguish between running and active transactions, because they reference
the
> database's currently running versionchange transaction. It seems like pinning
is
> unnecessary in the other tests, and hits a Firefox bug in those two tests.

Yeah, so we can probably remove. This is necessary in tests that e.g. want to
specifically probe this state, but it's not necessary for the new tests that
were using this helper.

[jsbell, 2016-10-17 20:20:23]

All the new tests pass in FF stable now. \o/

[pwnall, 2016-10-17 20:21:39]

LGTM, thank you for the explanations and for the quick turnaround!

[commit-bot: I haz the power, 2016-10-17 21:04:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-17 21:04:39]

Dry run: This issue passed the CQ dry run.

[jsbell, 2016-10-17 21:33:39]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2016-10-17 21:34:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 21:40:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-17 21:43:29]

Description was changed from

==========
Indexed DB: More exception precedence tests

For methods previously lacking such tests, add tests that cover which
exception is thrown when a method is called with two invalid
conditions at the same time.

R=pwnall@chromium.org
BUG=597303
==========

to

==========
Indexed DB: More exception precedence tests

For methods previously lacking such tests, add tests that cover which
exception is thrown when a method is called with two invalid
conditions at the same time.

R=pwnall@chromium.org
BUG=597303

Committed: https://crrev.com/1998b62c571a941c26fdc8bd1cda5fe2193d21d4
Cr-Commit-Position: refs/heads/master@{#425785}
==========

[commit-bot: I haz the power, 2016-10-17 21:43:30]

Patchset 2 (id:??) landed as
https://crrev.com/1998b62c571a941c26fdc8bd1cda5fe2193d21d4
Cr-Commit-Position: refs/heads/master@{#425785}