Bug 89377 – IndexedDB: Move method precondition checks to front end objects

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joshua Bell]

Currently, most IDB object implementations look like this (pseudocode):

IDBFooThingy::foo(param1, param2, ec) {
  request = IDBRequest::create(...);
  m_backend->foo(param1, param2, request, ec);
  if (ec) { // reporting this back is extra work
    request.markEarlyDeath(); // Complicated extra state
  }
  return request;
}

IDBFooThingyBackendImpl::foo(param1, param2, callbacks, ec) {
  if (param1.isBad()) { // Would have been nice to detect this earlier
    ec = SOME_ERR;
    return;
  }
  if (!param2.isGood()) { // Ditto.
    ec = OTHER_ERR;
    return;
  }
  if (m_transaction->scheduleTask(...)) {
    ec = TRANSACTION_INACTIVE_ERR; // Can't we know this earlier?
  }
}

Now that the metadata for stores/indexes is on the front end, this logic can be moved to the front end so that parameters can be validated prior to IPC. Additionally, the front end knows when the transaction is active, so in most cases the scheduling can be avoided.

[Joshua Bell]

Created attachment 148887 [details]
Patch

[Joshua Bell]

Patch depends on the 88467, but here's some of the back->front migration work.

[Joshua Bell]

Comment on attachment 148887 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:136
> +    if (!m_transaction->isActive()) {

The transaction internal |active| flag used here is implemented in a non-spec-compliant way. http://webkit.org/b/89379 will land a real implementation. Just enough logic exists to move these checks from the back-end to the front-end.

As seen below, a transaction will be born with active=true and will have active=false when either abort is called explicitly or when an complete or abort event is fired at it.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:150
> +    RefPtr<IDBObjectStore> objectStore = effectiveObjectStore();

Logic moved from IDBObjectStoreBackendImpl::put() to IDBCursor::update().

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:163
> +    ASSERT(!ec);

This pattern (that the back end calls are guaranteed to succeed, so requests don't need "early death" states) is used throughout now.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:175
> +    if (!m_transaction->isActive()) {

Previously, m_request was cleared out when the transaction is finished. This narrows the scope further (and http://webkit.org/b/89379 will take it even farther)

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:198
> +    if (key && !key->isValid()) {

As seen in such patches as http://webkit.org/b/86123

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:213
> +    if (!m_request->resetReadyState(m_transaction.get())) {

WebKit style - return early in error case.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:266
> +PassRefPtr<IDBObjectStore> IDBCursor::effectiveObjectStore()

The "effective object store" terminology is per spec.

> Source/WebCore/Modules/indexeddb/IDBCursor.h:94
> +    virtual bool isKeyCursor() const { return true; }

"Key" cursors and "value" cursors are distinguished in the back end by type flags, but on the front end by different classes. This seemed like the easiest way to expose this distinction, but alternatives welcome.

> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:123
> +        ASSERT_NOT_REACHED();

... and this pattern is now common on the back end - the only reason task scheduling can fail is if the back end is inactive, but the front end now tracks this state.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:132
> +    if (!usesInLineKeys && !hasKeyGenerator && !key) {

Logic moved from IDBObjectStoreBackendImpl::add() to IDBObjectStore::add() - it should also match the ordering in the spec for these clauses.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:161
> +    // TODO: Pass through keyPathKey as key

Oops, need to change to a FIXME. Changing what's passed through will be a follow-up patch.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:189
> +    const IDBKeyPath& keyPath = m_metadata.keyPath;

Right now, the logic is duplicated between add() and put() - I'll refactor to a common method that takes the enum flag.

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:327
> +        m_readyState = EarlyDeath;

The public method for setting this state goes away, but we still need it for this case (context stopping while request is pending).

[Joshua Bell]

Created attachment 149139 [details]
Patch

[Joshua Bell]

alecflett@, dgrogan@ - how's this look?

tony@ - r?

[Tony Chang]

Comment on attachment 149139 [details]
Patch

David or Alec, can one or both of you review this?

[Alec Flett]

Comment on attachment 149139 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:151
> +    const IDBKeyPath& keyPath = objectStore->metadata().keyPath;

Minor, but in general I'd sooner see calling objectStore->keyPath() so most callers aren't dependent on the actual use of metadata objects.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:213
> +    if (!m_request->resetReadyState(m_transaction.get())) {

Sanity check just because it wasn't mentioned: the FIXME is ok to remove?

> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:123
> +        ASSERT_NOT_REACHED();

Why wouldn't this be reached? Is there something else we can also assert here, like m_transaction->isActive()?

> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:186
> +        ASSERT_NOT_REACHED();

Same here as my previous comment

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:166
> +        ASSERT_NOT_REACHED();

another.. you get the idea... :)

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:125
> +    ASSERT(!ec);

Assert does seem dangerous here - maybe worth a comment (I see the m_transaction->isActive()

(looks like you got a lot of them, but I wonder how many of the ones that are left could be removed? Mostly in the interest of getting rid of an extra state for IDBRequest)

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:-160
> -    if (putMode != CursorUpdate) {

this is great, and vastly simplifies some of my put() work coming up!

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:434
> +    ASSERT(transaction->mode() == IDBTransaction::VERSION_CHANGE);

this is the sort of ASSERT I was wondering if we could throw above with all the ASSERT_NOT_REACHED stuff - i.e. making sure we're in a specific state

> Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:445
> +        ASSERT_NOT_REACHED();

Can we remove this entirely then?

[Joshua Bell]

Comment on attachment 149139 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:151
>> +    const IDBKeyPath& keyPath = objectStore->metadata().keyPath;
> 
> Minor, but in general I'd sooner see calling objectStore->keyPath() so most callers aren't dependent on the actual use of metadata objects.

Unfortunately, objectStore->keyPath() implements the IDL which produces an IDBAny which is a one-way transform. Otherwise I'd agree.

>> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:213
>> +    if (!m_request->resetReadyState(m_transaction.get())) {
> 
> Sanity check just because it wasn't mentioned: the FIXME is ok to remove?

Hrm, good point. I thought it was a dead comment, but I now understand the question. I'll put it back, and undo this unrelated cleanup.

>> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:123
>> +        ASSERT_NOT_REACHED();
> 
> Why wouldn't this be reached? Is there something else we can also assert here, like m_transaction->isActive()?

Re: the logic: IDBTransactionBackendImpl::scheduleTask only fails if the transaction is finished. The front end should never call into this while the transaction is inactive (via the isActive tests). Once the transaction is finished it can never be active again.

However (as we discussed offline), there's a scenario where this isn't true in a multiprocess port. If the transaction aborts while the front-end->back-end call is "in flight" the transaction could be in a "finished" state before this is reached, and we need to signal the front-end as the code previously did. So I'm going to remove these assertions and restore the error handling path on the front-end.

>> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:125
>> +    ASSERT(!ec);
> 
> Assert does seem dangerous here - maybe worth a comment (I see the m_transaction->isActive()
> 
> (looks like you got a lot of them, but I wonder how many of the ones that are left could be removed? Mostly in the interest of getting rid of an extra state for IDBRequest)

This is now reverted. The extra state needs to stay, alas.

[Joshua Bell]

Created attachment 149394 [details]
Patch

[Alec Flett]

Comment on attachment 149394 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBTransaction.h:71
> +    // FIXME: Implement internal |active| flag per spec. http://webkit.org/b/89379

Last thing: Is this fixme just cruft from development? if not, how are we divergent from the spec at this point - put at least a simple explanation here..

[Joshua Bell]

(In reply to comment #10)

> > Source/WebCore/Modules/indexeddb/IDBTransaction.h:71
> > +    // FIXME: Implement internal |active| flag per spec. http://webkit.org/b/89379
> 
> Last thing: Is this fixme just cruft from development? if not, how are we divergent from the spec at this point - put at least a simple explanation here..

The details on how we diverge from the spec are captured in the linked bug but I can clarify the comment somewhat.

[Joshua Bell]

Created attachment 149549 [details]
Patch

[Joshua Bell]

> The details on how we diverge from the spec are captured in the linked bug but I can clarify the comment somewhat.

Actually, I just removed the comment; the subtle details are more than belongs in a FIXME when there's already a bug filed.

[Alec Flett]

ok, sorry I wasn't sure if the details in the other bug were a superset of what is left after this patch... LGTM

[WebKit Review Bot]

Comment on attachment 149549 [details]
Patch

Clearing flags on attachment: 149549

Committed r121291: <http://trac.webkit.org/changeset/121291>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.