Bug 92897 – IndexedDB: Frontend and plumbing for integer versions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[David Grogan]

IndexedDB: Frontend and plumbing for integer versions

[David Grogan]

Created attachment 155858 [details]
Patch

[David Grogan]

Created attachment 155881 [details]
Patch

[David Grogan]

Josh, Alec, could you look at this?

This turns on integer versions in the front end.  This is mostly plumbing, but there are a few tricky bits.  E.g. not creating two IDBDatabaseBackendProxy objects for the same database.

[David Grogan]

Oh, and after this patch, we should be close enough to remove the prefix.  There are still a few FAILs in the output, but nothing too serious.

[David Grogan]

Created attachment 158433 [details]
Patch

[David Grogan]

Created attachment 158438 [details]
Patch

[David Grogan]

Created attachment 158443 [details]
Patch

[Joshua Bell]

Comment on attachment 158433 [details]
Patch

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

First batch of comments.

> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:314
> +    enqueueEvent(IDBUpgradeNeededEvent::create(existingVersion, requestedVersion, eventNames().versionchangeEvent));

Just checking - this is using IDBUpgradeNeededEvent for its (oldversion/newversion) payload; it may appear to script as an Event.type = "versionchange" (e.g. here) or an Event.type = "upgradeneeded"?

> Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

Date for new file: 2012

> Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp:63
> +    ASSERT(!m_errorCode && m_errorMessage.isNull() && !m_result);

Call to shouldEnqueueEvent() here?

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:286
> +        idbDatabase->registerFrontendCallbacks();

The registerFrontendCallbacks() call is in both the if and else paths, factor out?

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:527
> +    ASSERT_WITH_MESSAGE(m_readyState == PENDING || m_didFireUpgradeNeededEvent, "When queueing event %s, m_readyState was %d", event->type().string().utf8().data(), m_readyState);

This is odd. Is m_readyState not PENDING between "upgradeneeded" and success? I would have assumed that like "blocked", the IDBRequest doesn't flip to "done" until the "success" fires, in which case this isn't necessary. (But code is needed to say "if upgradeneeded, don't set done because we'll fire again". That would also eliminate the need to override shouldEnqueueEvent()

Hrm... it looks like the spec doesn't actually specify when the "done flag" should be set for an IDBOpenDBRequest - bleah.

> Source/WebCore/Modules/indexeddb/IDBRequest.h:106
> +    void clearTransactionProperty();

Naming nit: Maybe just clearTransaction()?

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:387
> +    // FIXME: Try to construct a test where |this| outlives openDBRequest and we

Is clearing the request->transaction reference out required by the spec?

> Source/WebCore/Modules/indexeddb/IDBUpgradeNeededEvent.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

Year for new file: 2012.

> Source/WebCore/Modules/indexeddb/IDBUpgradeNeededEvent.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

Year for new file: 2012.

[David Grogan]

Comment on attachment 158433 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:314
>> +    enqueueEvent(IDBUpgradeNeededEvent::create(existingVersion, requestedVersion, eventNames().versionchangeEvent));
> 
> Just checking - this is using IDBUpgradeNeededEvent for its (oldversion/newversion) payload; it may appear to script as an Event.type = "versionchange" (e.g. here) or an Event.type = "upgradeneeded"?

event.type will yield "versionchange" but String(event) will yield "[object IDBUpgradeNeededEvent]".  I'm not sure how important it is to make String(event) return "[object IDBVersionChangeEvent]".

>> Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp:2
>> + * Copyright (C) 2011 Google Inc. All rights reserved.
> 
> Date for new file: 2012

done.

>> Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp:63
>> +    ASSERT(!m_errorCode && m_errorMessage.isNull() && !m_result);
> 
> Call to shouldEnqueueEvent() here?

Done, thanks.

>> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:286
>> +        idbDatabase->registerFrontendCallbacks();
> 
> The registerFrontendCallbacks() call is in both the if and else paths, factor out?

Done.

>> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:527
>> +    ASSERT_WITH_MESSAGE(m_readyState == PENDING || m_didFireUpgradeNeededEvent, "When queueing event %s, m_readyState was %d", event->type().string().utf8().data(), m_readyState);
> 
> This is odd. Is m_readyState not PENDING between "upgradeneeded" and success? I would have assumed that like "blocked", the IDBRequest doesn't flip to "done" until the "success" fires, in which case this isn't necessary. (But code is needed to say "if upgradeneeded, don't set done because we'll fire again". That would also eliminate the need to override shouldEnqueueEvent()
> 
> Hrm... it looks like the spec doesn't actually specify when the "done flag" should be set for an IDBOpenDBRequest - bleah.

4.8.9.3 is
Fire a upgradeneeded event targeted at request. The event must use the IDBVersionChangeEvent interface and have the oldVersion property set to old version and have the newVersion property set to version. The readyState on the request is set to "done".

I interpreted the last sentence to mean that the done flag should be set to true when upgradeneeded is fired. Indeed, request.result is supposed to be available to the upgradeneeded handler. There's nothing to indicate that it should be reverted back to false, so I'm guessing it's supposed to stay true. I haven't checked FF though.

An additional wrinkle is that a complete event is fired at the version change transaction between upgradeneeded and success. The availability of request.result when that event is handled needs to be considered.  In this patch, clearTransactionProperty sets readyState = PENDING after complete is dispatched, just to avoid more uses of m_didFireUpgradeNeededEvent.

>> Source/WebCore/Modules/indexeddb/IDBRequest.h:106
>> +    void clearTransactionProperty();
> 
> Naming nit: Maybe just clearTransaction()?

Because this function did more than clear the transaction (it also resets readyState), I changed it to transactionDidDispatchCompleteOrAbort. Even less wieldy. Perhaps I should change it back to clearTransaction, not clear readyState, and add more m_readyState == PENDING || m_didFireUpgradeNeededEvent in place of m_readyState == PENDING.

>> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:387
>> +    // FIXME: Try to construct a test where |this| outlives openDBRequest and we
> 
> Is clearing the request->transaction reference out required by the spec?

Yeah, the last step in '"versionchange" transaction steps' (4.8.12, mentioned in the changelog) is:

When the transaction is finished, if these steps are run asynchronously, immediately set request's transaction property to null. This must be done in the same task as the task firing the complete or abort event, but after those events has been fired.

>> Source/WebCore/Modules/indexeddb/IDBUpgradeNeededEvent.cpp:2
>> + * Copyright (C) 2011 Google Inc. All rights reserved.
> 
> Year for new file: 2012.

done.

>> Source/WebCore/Modules/indexeddb/IDBUpgradeNeededEvent.h:2
>> + * Copyright (C) 2011 Google Inc. All rights reserved.
> 
> Year for new file: 2012.

done.

[David Grogan]

Created attachment 158467 [details]
Patch

[Joshua Bell]

(In reply to comment #9)
> event.type will yield "versionchange" but String(event) will yield "[object IDBUpgradeNeededEvent]".  I'm not sure how important it is to make String(event) return "[object IDBVersionChangeEvent]".

IMHO we can defer fixing until we remove setVersion().

Does the WebKitIDL attribute [InterfaceName=IDBVersionChangeEvent] specified on the interface happen to "just work"?
 
> > Hrm... it looks like the spec doesn't actually specify when the "done flag" should be set for an IDBOpenDBRequest - bleah.
> 
> 4.8.9.3 is
> Fire a upgradeneeded event targeted at request. The event must use the IDBVersionChangeEvent interface and have the oldVersion property set to old version and have the newVersion property set to version. The readyState on the request is set to "done".

Thanks, missed that.
 
> I interpreted the last sentence to mean that the done flag should be set to true when upgradeneeded is fired. 

Agreed.

> Indeed, request.result is supposed to be available to the upgradeneeded handler. 

Yeah, which explains it.

> There's nothing to indicate that it should be reverted back to false, so I'm guessing it's supposed to stay true. I haven't checked FF though.

I would agree that staying true makes sense...
 
> An additional wrinkle is that a complete event is fired at the version change transaction between upgradeneeded and success. The availability of request.result when that event is handled needs to be considered.  In this patch, clearTransactionProperty sets readyState = PENDING after complete is dispatched, just to avoid more uses of m_didFireUpgradeNeededEvent.

Bleah, messy whatever way we go. Bring the ambiguity up on the spec list?

It would be nice if we could flip it back to "pending" before enqueuing the success, since that simplifies the state machine of IDBRequest.

It looks like (in the latest patch) the m_didFireUpgradeNeededEvent flag is used only for asserting? Nice if we can keep it like that (although I hate tracking debug-only state).

> >> Source/WebCore/Modules/indexeddb/IDBRequest.h:106
> >> +    void clearTransactionProperty();
> > 
> > Naming nit: Maybe just clearTransaction()?
> 
> Because this function did more than clear the transaction (it also resets readyState), I changed it to transactionDidDispatchCompleteOrAbort.

FWIW the internal state enum in IDBTransaction.h calls this state "Finished", if that helps naming.

[David Grogan]

(In reply to comment #11)
> (In reply to comment #9)
> > event.type will yield "versionchange" but String(event) will yield "[object IDBUpgradeNeededEvent]".  I'm not sure how important it is to make String(event) return "[object IDBVersionChangeEvent]".
> 
> Does the WebKitIDL attribute [InterfaceName=IDBVersionChangeEvent] specified on the interface happen to "just work"?

Seems to. Good stuff. Thanks.

> > An additional wrinkle is that a complete event is fired at the version change transaction between upgradeneeded and success. The availability of request.result when that event is handled needs to be considered.  In this patch, clearTransactionProperty sets readyState = PENDING after complete is dispatched, just to avoid more uses of m_didFireUpgradeNeededEvent.
> 
> Bleah, messy whatever way we go. Bring the ambiguity up on the spec list?

By ambiguity, you mean the value of readyState when the "complete" event is handled?

> 
> It would be nice if we could flip it back to "pending" before enqueuing the success, since that simplifies the state machine of IDBRequest.

We currently flip back to pending before *dispatching* the success event, but not before enqueueing it.  Flipping back to pending before enqueuing the success event would mean that the "complete" handler would not be able to access request.result, correct?

> It looks like (in the latest patch) the m_didFireUpgradeNeededEvent flag is used only for asserting? Nice if we can keep it like that (although I hate tracking debug-only state).

Yeah, it's just used in ASSERTs.

> 
> > >> Source/WebCore/Modules/indexeddb/IDBRequest.h:106
> > >> +    void clearTransactionProperty();
> > > 
> > > Naming nit: Maybe just clearTransaction()?
> > 
> > Because this function did more than clear the transaction (it also resets readyState), I changed it to transactionDidDispatchCompleteOrAbort.
> 
> FWIW the internal state enum in IDBTransaction.h calls this state "Finished", if that helps naming.

[Joshua Bell]

(In reply to comment #12)
> 
> By ambiguity, you mean the value of readyState when the "complete" event is handled?

Yes.
 
> > 
> > It would be nice if we could flip it back to "pending" before enqueuing the success, since that simplifies the state machine of IDBRequest.
> 
> We currently flip back to pending before *dispatching* the success event, but not before enqueueing it.  Flipping back to pending before enqueuing the success event would mean that the "complete" handler would not be able to access request.result, correct?

Right, so depends on the discussion of readyState vs. "complete". Right now the assertions in IDBRequest can say "if you're going to enqueue or dispatch, you'd better be PENDING". I wasn't suggesting changes, just expressing the hope we can keep things simple.

(Given how messy IDBRequest::dispatchEvent is already, we can probably do a cleanup pass after everything is in; expose virtual helper functions that categorize events and what can be done when, etc.)

[David Grogan]

Created attachment 158686 [details]
Patch

[David Grogan]

(In reply to comment #11)
> > >> Source/WebCore/Modules/indexeddb/IDBRequest.h:106
> > >> +    void clearTransactionProperty();
> > > 
> > > Naming nit: Maybe just clearTransaction()?
> > 
> > Because this function did more than clear the transaction (it also resets readyState), I changed it to transactionDidDispatchCompleteOrAbort.
> 
> FWIW the internal state enum in IDBTransaction.h calls this state "Finished", if that helps naming.

I want to preserve the indication that it happens after the events are dispatched.  m_state = Finished happens before the events are dispatched.  Right now the name is transactionDidFinishAndDispatch.  Still ugh.

[David Grogan]

Created attachment 158726 [details]
Patch

[David Grogan]

Created attachment 158960 [details]
Patch

[David Grogan]

Tony, could you review this?

It's huge but mostly simple.

[Tony Chang]

Comment on attachment 158960 [details]
Patch

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

If jsbell is happy, seems fine to me.

> LayoutTests/ChangeLog:11
> +        * storage/indexeddb/intversion-abort-in-initial-upgradeneeded-expected.txt:
> +        * storage/indexeddb/intversion-and-setversion-expected.txt:
> +        * storage/indexeddb/intversion-blocked-expected.txt:
> +        * storage/indexeddb/intversion-close-between-events-expected.txt:

Nit: Maybe explain some of the changes here.

[David Grogan]

Created attachment 158981 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 158981 [details]
Patch for landing

Clearing flags on attachment: 158981

Committed r125850: <http://trac.webkit.org/changeset/125850>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Grogan]

*** Bug 89505 has been marked as a duplicate of this bug. ***