Mark transactions inactive during key serialization

[nolanlawson]

Closes #476

The following tasks have been completed:

• Confirmed there are no ReSpec/BikeShed errors or warnings.
• Modified Web platform tests (Mark IndexedDB transactions inactive during key serialization web-platform-tests/wpt#55660)

Implementation commitment:

• WebKit (https://bugs.webkit.org/show_bug.cgi?id=)
• Chromium (https://bugs.chromium.org/p/chromium/issues/detail?id=)
• Gecko (Already passes 2/5 tests) (https://bugzilla.mozilla.org/show_bug.cgi?id=)

---

Preview | Diff

[nolanlawson]

Note I only added the minimal logic to mark the transaction inactive when a key is provided to IDBObjectStore#add/put. I did not:

• mark the transaction inactive anywhere else that we convert a key to a value, e.g. indexedDB.cmp or IDBCursor#continue, because this matches Firefox's implementation (and Chromium's as well – both browsers just throw InternalError: too much recursion / RangeError: Maximum call stack size exceeded
• refactor to reuse the existing logic from clone – this seemed like overkill since the logic was only needed in one place.

[asutherland]

Thanks for calling out other places where the spec converts a key to a value! (And for filing the spec issue in the first place!)

For places like indexedDB.cmp and the IDBKeyRange methods (only/lowerBound/upperBound/bound/includes) where we don't have a transaction context to mark inactive and there's no complex state management going on it seems fine to not change the logic.

But for continue and continuePrimaryKey we do explicitly have transactions and we do check that they are active and I think there is enough potentially interesting state management going on that it makes sense to consistently apply the same rationale we're using for add/put. In particular, it seems like we could be in an upgrade transaction when calling one of the continue methods and then nefarious code could attempt to delete the object store or index that the cursor is against with that method on the stack. I do think it's much less likely for code to be written in a way that could cause a security bug in this situation, but I do think it still applies that there's no reasonable use-case for content to be doing tricky things here and I do think it also simplifies things from a spec perspective.

For example, step 3 of both continue methods is to throw if the underlying source/object store has been deleted. If we don't mark the transaction as inactive, then deleteObjectStore currently is defined to synchronously delete the store; we "destroy store" without ever going "in parallel" or doing async hand-waving. deleteIndex also synchronously does "destroy index" although the 2nd para afterwards does do some hand-waving: "Although this method does not return an IDBRequest object, the index destruction itself is processed as an asynchronous request within the upgrade transaction." So we would side-step these edge-cases where the index/objectStore could conceptually be deleted partway through the algorithm and any need to test/specify that by making the transaction inactive during the key conversions.

[nolanlawson]

Thanks for the feedback @asutherland! I just checked, and it appears that none of Firefox/Chromium/WebKit actually throw a TransactionInactiveError in the case of continue/continuePrimaryKey (see test draft below).

Click to see test draft

promise_test(async testCase => {
  const db = await createDatabase(testCase, database => {
    database.createObjectStore('store');
  });

  const transaction = db.transaction(['store'], 'readwrite');
  const objectStore = transaction.objectStore('store');

  objectStore.put({}, 0);
  objectStore.put({}, 1);
  const cursor = await new Promise((resolve, reject) => {
    const cursorReq = objectStore.openCursor();
    cursorReq.onerror = reject;
    cursorReq.onsuccess = e => resolve(e.target.result);
  });

  let getterCalled = false;
  const activeKey = ['value that should not be used'];
  Object.defineProperty(activeKey, '0', {
    enumerable: true,
    get: testCase.step_func(() => {
      getterCalled = true;
      assert_throws_dom('TransactionInactiveError', () => {
        objectStore.get('key');
      }, 'transaction should not be active during key serialization');
      return 'value that should not be used';
    }),
  });
  cursor.continue(activeKey);
  await promiseForTransaction(testCase, transaction);
  db.close();

  assert_true(getterCalled,
    "activeKey's getter should be called during test");
}, 'Transaction inactive during key serialization in IDBCursor.continue()');

promise_test(async testCase => {
  const db = await createDatabase(testCase, database => {
    const objectStore = database.createObjectStore('store');
    objectStore.createIndex('idx', 'name');
  });

  const transaction = db.transaction(['store'], 'readwrite');
  const objectStore = transaction.objectStore('store');

  objectStore.put({ name: 'a' }, 0);
  objectStore.put({ name: 'b' }, 1);
  const idx = objectStore.index('idx')
  const cursor = await new Promise((resolve, reject) => {
    const cursorReq = idx.openCursor();
    cursorReq.onerror = reject;
    cursorReq.onsuccess = e => resolve(e.target.result);
  });

  let getterCalled = false;
  const activeKey = ['value that should not be used'];
  Object.defineProperty(activeKey, '0', {
    enumerable: true,
    get: testCase.step_func(() => {
      getterCalled = true;
      assert_throws_dom('TransactionInactiveError', () => {
        objectStore.get('key');
      }, 'transaction should not be active during key serialization');
      return 'value that should not be used';
    }),
  });
  cursor.continuePrimaryKey(activeKey, 0);
  await promiseForTransaction(testCase, transaction);
  db.close();

  assert_true(getterCalled,
    "activeKey's getter should be called during test");
}, 'Transaction inactive during key serialization in IDBCursor.continuePrimaryKey()');

Firefox does throw for put/add, though, which is why I originally scoped the fix to just that. Are you proposing that we handle continue/continuePrimaryKey in the spec despite Firefox's current behavior?

[nolanlawson]

After re-reading your comment, I realized you weren't talking specifically about Firefox's implementation. I agree we can make this work with IDBCursor#continue / continuePrimaryKey as well. Updated the spec language and updated the tests at web-platform-tests/wpt#55660 .

[asutherland]

After re-reading your comment, I realized you weren't talking specifically about Firefox's implementation.

Yes.

I agree we can make this work with IDBCursor#continue / continuePrimaryKey as well. Updated the spec language and updated the tests at web-platform-tests/wpt#55660 .

Thank you; I like the introduction of the wrapper, this seems very clean!

@evanstade and @SteveBeckerMSFT in #476 I think we were initially only talking about matching Firefox; are you okay with the (consistent) expansion to also cover the cursor continue methods?

[evanstade]

There are a lot of methods that take a key as an argument and are associated with a transaction --- many of the ones in ObjectStore, not just put and add. Why are those safe as-is if we think IDBCursor#continue should be hardened?

Just based on the name alone, converting a value to a key during a transaction sounds like something that should universally be used over converting a value to a key if there is an associated transaction. Otherwise we should come up with a name for it that better distinguishes when it should be referenced in the spec.

[asutherland]

Yes, I agree we should expand the mechanism to cover convert a value to a key range too if you're on board since the same rationale applies. I somehow had a lot of tunnel vision going on and ignored that algorithm and its many uses.

[evanstade]

Yeah it makes sense to apply the change more broadly.

(Sorry for slow reply.)

[nolanlawson]

Sorry for the delay. I've made the following changes:

• Renamed the existing algorithm to "recursively convert a value to a key" and made the new "convert a value to a key" take an optional transaction. ("Recursively..." is not an amazing name, and I'm open to suggestions. 🙂)
  • Note: I found this tricky to refactor given the necessary try/catch logic and recursion in the existing algorithm; I hope it's okay to have two separate algorithms.
• Refactored "is a potentially valid key range" and "convert a value to a key range" to pass in the transaction.
• The IDBKeyRange methods (e.g. only/lowerBound) do not pass transaction since they aren't explicitly associated with a transaction. Same for indexedDB.cmp().
• Similarly, "extract a key from a value using a key path" does not pass transaction since there is no explicit transaction, and plus its inputs are already "sanitized" in the sense that they are derived from the store's key path rather than arbitrary user input.

The WPT tests will be a bit tricky to cover all these new scenarios, so I wanted to check if the spec changes look good first. (For example, "Creating a request to retrieve multiple items" calls the new transaction-checking logic multiple times, so you could imagine writing a test where the getter uses a counter to only trigger the TransactionInactiveError on the 1st, 2nd, or 3rd time.)

[nolanlawson]

Friendly ping @asutherland @evanstade 🙂

[SteveBeckerMSFT]

@nolanlawson thanks for the PR! I'll also review this soon.