Bug 92558 - IndexedDB: Core upgradeneeded logic
Summary: IndexedDB: Core upgradeneeded logic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Grogan
URL:
Keywords:
Depends on: 93055
Blocks: 92883
  Show dependency treegraph
 
Reported: 2012-07-27 15:46 PDT by David Grogan
Modified: 2012-08-03 17:28 PDT (History)
6 users (show)

See Also:


Attachments
Patch (26.17 KB, patch)
2012-07-27 15:46 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (28.42 KB, patch)
2012-07-27 15:57 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (28.52 KB, patch)
2012-07-27 17:28 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (28.66 KB, patch)
2012-07-30 17:19 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (34.87 KB, patch)
2012-07-31 16:03 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (35.05 KB, patch)
2012-07-31 18:53 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (35.08 KB, patch)
2012-08-01 16:06 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch for landing (35.52 KB, patch)
2012-08-02 16:36 PDT, David Grogan
no flags Details | Formatted Diff | Diff
Patch (36.10 KB, patch)
2012-08-03 13:58 PDT, David Grogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2012-07-27 15:46:09 PDT
IndexedDB: Core upgradeneeded logic
Comment 1 David Grogan 2012-07-27 15:46:34 PDT
Created attachment 155071 [details]
Patch
Comment 2 David Grogan 2012-07-27 15:57:37 PDT
Created attachment 155080 [details]
Patch
Comment 3 David Grogan 2012-07-27 16:03:18 PDT
Josh and Alec, could you take a look at this?

This patch has code that only runs in the backend, and is broken off from bug 89505.

I could break this up even further, first doing IDBFactoryBackendImpl and then IDBDatabaseBackendImpl if desired.
Comment 4 Alec Flett 2012-07-27 16:16:39 PDT
Comment on attachment 155080 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:59
> +class IDBDatabaseBackendImpl::PendingOpenWithVersionCall : public RefCounted<PendingOpenWithVersionCall> {

Any reason not to just put the class definition inside the class? I didn't even know this syntax was legal ! (I guess it's kind of like a namespace...)

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:425
> +        // The frontend takes care of the following clause, the backend just

This sentence is slightly confusing - is there an "and then.." or something? I'm confused... or something like ", and then the frontend will only deliver to connections with closePending set"? (addition of 'and' and 'only')

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:446
> +    RefPtr<IDBTransactionBackendInterface> transactionInterface = this->transaction(objectStoreNames.get(), IDBTransaction::VERSION_CHANGE, ec);

don't need "this->"

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:466
> +            ASSERT(!m_intVersion);

Not sure what this is asserting for - is this different from  NoIntVersion?
Comment 5 Alec Flett 2012-07-27 16:17:22 PDT
my comments are pretty minor not understanding most of it - perhaps monday you can give us an overview, and help educate at least me about some of the states and issues this is tackling?
Comment 6 David Grogan 2012-07-27 17:15:05 PDT
Comment on attachment 155080 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:59
>> +class IDBDatabaseBackendImpl::PendingOpenWithVersionCall : public RefCounted<PendingOpenWithVersionCall> {
> 
> Any reason not to just put the class definition inside the class? I didn't even know this syntax was legal ! (I guess it's kind of like a namespace...)

They would bloat the header file, cause long local recompiles, etc.  Though I don't know how big of a deal that actually is.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:425
>> +        // The frontend takes care of the following clause, the backend just
> 
> This sentence is slightly confusing - is there an "and then.." or something? I'm confused... or something like ", and then the frontend will only deliver to connections with closePending set"? (addition of 'and' and 'only')

Tried to reword it, let me know if it's clearer.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:446
>> +    RefPtr<IDBTransactionBackendInterface> transactionInterface = this->transaction(objectStoreNames.get(), IDBTransaction::VERSION_CHANGE, ec);
> 
> don't need "this->"

Removed.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:466
>> +            ASSERT(!m_intVersion);
> 
> Not sure what this is asserting for - is this different from  NoIntVersion?

Ah, thanks, it should be ASSERT(m_intVersion == NoIntVersion).  This means that I have no layout test testing this scenario, I will add one.
Comment 7 David Grogan 2012-07-27 17:28:57 PDT
Created attachment 155096 [details]
Patch
Comment 8 Joshua Bell 2012-07-29 19:20:32 PDT
Comment on attachment 155096 [details]
Patch

Looks like a great chunk to break off and get landed. Nits, but also two things which look incorrect to me.

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

> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:54
> +        ASSERT_NOT_REACHED();

Why this instead of pure virtual, like the others?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:239
> +        callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UNKNOWN_ERR, String::format("You can't use the setVersion function if you've already set the version through an open call.  The current integer version is %"PRId64, m_intVersion)));

Shouldn't this method return here if the error event is fired?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:289
> +    database->m_intVersion = version;

For kicks, toss an ASSERT in here that version > oldVersion?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:473
> +        m_pendingOpenWithVersionCalls.append(PendingOpenWithVersionCall::create(callbacks, version));

This line here looks a little odd - I'm not sure why we're add a pending call in this case. If it's intentional, can you explain?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:530
> +    // FIXME: Instead of relying on transactions.size(), make connectionCount

Ugh, yeah, this is tricky. Thanks for the comment.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:283
> +    return true;

If you touch this patch again, toss in a FIXME/bug reference here.
Comment 9 David Grogan 2012-07-30 15:53:46 PDT
Comment on attachment 155096 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:54
>> +        ASSERT_NOT_REACHED();
> 
> Why this instead of pure virtual, like the others?

Don't recall, changed.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:239
>> +        callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UNKNOWN_ERR, String::format("You can't use the setVersion function if you've already set the version through an open call.  The current integer version is %"PRId64, m_intVersion)));
> 
> Shouldn't this method return here if the error event is fired?

Egads, yes.  Added a layout test that exposes this bug.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:289
>> +    database->m_intVersion = version;
> 
> For kicks, toss an ASSERT in here that version > oldVersion?

SG

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:473
>> +        m_pendingOpenWithVersionCalls.append(PendingOpenWithVersionCall::create(callbacks, version));
> 
> This line here looks a little odd - I'm not sure why we're add a pending call in this case. If it's intentional, can you explain?

It was intentional, but it's not working out well enough.  The idea was that by the time that pending call is serviced, the existing version would be the one it requested and a success event would be delivered.  Instead, I'm going to replace it with a PendingOpenWithVersionCallSecondHalf queue that gets serviced first in processPendingCalls.

>> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:283
>> +    return true;
> 
> If you touch this patch again, toss in a FIXME/bug reference here.

Done.
Comment 10 David Grogan 2012-07-30 17:19:46 PDT
Created attachment 155404 [details]
Patch
Comment 11 David Grogan 2012-07-30 17:20:42 PDT
Comment on attachment 155096 [details]
Patch

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

>>> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:54
>>> +        ASSERT_NOT_REACHED();
>> 
>> Why this instead of pure virtual, like the others?
> 
> Don't recall, changed.

To avoid having to update the FakeBackingStore in this patch.  Changed back.
Comment 12 David Grogan 2012-07-31 15:02:29 PDT
(In reply to comment #11)
> (From update of attachment 155096 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155096&action=review
> 
> >>> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:54
> >>> +        ASSERT_NOT_REACHED();
> >> 
> >> Why this instead of pure virtual, like the others?
> > 
> > Don't recall, changed.
> 
> To avoid having to update the FakeBackingStore in this patch.  Changed back.

Scratch that, FakeBackingStore is already in this patch.
Comment 13 David Grogan 2012-07-31 16:03:12 PDT
Created attachment 155659 [details]
Patch
Comment 14 David Grogan 2012-07-31 18:53:22 PDT
Created attachment 155701 [details]
Patch
Comment 15 David Grogan 2012-07-31 18:56:27 PDT
Tony, could you review this?

It's the first of the patches mentioned in bug 92560.
Comment 16 David Grogan 2012-08-01 14:14:28 PDT
Ojan, could you review this?
Comment 17 Joshua Bell 2012-08-01 15:37:07 PDT
Comment on attachment 155701 [details]
Patch

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

Still LGTM

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:-290
> -    ASSERT(connectionCount() <= 1);

It's a shame to lose this strong assertion about state, but I can't think of an equivalent. Looking forward to simplifying when setVersion is gone.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:339
> +    ASSERT(m_pendingSecondHalfOpenWithVersionCalls.size() <= 1);

Given this, does it need to be a vector? (Maybe FIXME for later to clarify the state machine the spec implies.)

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:423
> +    // Now that we give max priority to open calls that follow upgradeneeded

Reword to remove the "Now that...", e.g. "We give max priority to open calls that follow upgradeneeded events; trigger the rest of the queues being service when those open calls are finished."
Comment 18 David Grogan 2012-08-01 16:06:36 PDT
Created attachment 155914 [details]
Patch
Comment 19 David Grogan 2012-08-01 16:08:01 PDT
(In reply to comment #17)
> (From update of attachment 155701 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155701&action=review
> 
> Still LGTM
> 
> > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:-290
> > -    ASSERT(connectionCount() <= 1);
> 
> It's a shame to lose this strong assertion about state, but I can't think of an equivalent. Looking forward to simplifying when setVersion is gone.
> 
> > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:339
> > +    ASSERT(m_pendingSecondHalfOpenWithVersionCalls.size() <= 1);
> 
> Given this, does it need to be a vector? (Maybe FIXME for later to clarify the state machine the spec implies.)

Probably not, there's some comments about this in the ChangeLog, but I added a FIXME.

> > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:423
> > +    // Now that we give max priority to open calls that follow upgradeneeded
> 
> Reword to remove the "Now that...", e.g. "We give max priority to open calls that follow upgradeneeded events; trigger the rest of the queues being service when those open calls are finished."

Done.
Comment 20 Ojan Vafai 2012-08-02 15:58:05 PDT
Comment on attachment 155914 [details]
Patch

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

Just a bunch of style nits

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:325
> +        // If this was an open-with-version call, there should be a "second

Make this a FIXME maybe?

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

It's a little confusing to have this without an associated FIXME.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:458
> +    // The spec dictates we wait until all the version change events are

Should this be a FIXME?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:470
> +    RefPtr<DOMStringList> objectStoreNames = DOMStringList::create();

Nit: I'd put a line-break before this line just for readability after the early return.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:478
> +    if (!transaction->scheduleTask(createCallbackTask(&IDBDatabaseBackendImpl::setIntVersionInternal, database, requestedVersion, callbacks, transaction),
> +                                   createCallbackTask(&IDBDatabaseBackendImpl::resetVersion, database, m_version))) {

This might be a little easier to read if you created the callback tasks before the if-statement.

> Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:168
> +        databaseBackend = IDBDatabaseBackendImpl::create(name, backingStore.get(), m_transactionCoordinator.get(), this, uniqueIdentifier);

Nit: I'd prefer a line-break before this line for readability.

> Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:177
> +    if (version == IDBDatabaseMetadata::NoIntVersion)

Nit: would prefer a line-break before this line as well.
Comment 21 David Grogan 2012-08-02 16:35:51 PDT
Comment on attachment 155914 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:325
>> +        // If this was an open-with-version call, there should be a "second
> 
> Make this a FIXME maybe?

SG

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:447
>> +    ASSERT_NOT_REACHED();
> 
> It's a little confusing to have this without an associated FIXME.

Agreed, added.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:458
>> +    // The spec dictates we wait until all the version change events are
> 
> Should this be a FIXME?

Done.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:470
>> +    RefPtr<DOMStringList> objectStoreNames = DOMStringList::create();
> 
> Nit: I'd put a line-break before this line just for readability after the early return.

Done.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:478
>> +                                   createCallbackTask(&IDBDatabaseBackendImpl::resetVersion, database, m_version))) {
> 
> This might be a little easier to read if you created the callback tasks before the if-statement.

Done.

>> Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:168
>> +        databaseBackend = IDBDatabaseBackendImpl::create(name, backingStore.get(), m_transactionCoordinator.get(), this, uniqueIdentifier);
> 
> Nit: I'd prefer a line-break before this line for readability.

Done.

>> Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:177
>> +    if (version == IDBDatabaseMetadata::NoIntVersion)
> 
> Nit: would prefer a line-break before this line as well.

Done.
Comment 22 David Grogan 2012-08-02 16:36:26 PDT
Created attachment 156200 [details]
Patch for landing
Comment 23 WebKit Review Bot 2012-08-02 17:36:16 PDT
Comment on attachment 156200 [details]
Patch for landing

Clearing flags on attachment: 156200

Committed r124540: <http://trac.webkit.org/changeset/124540>
Comment 24 WebKit Review Bot 2012-08-02 17:36:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 WebKit Review Bot 2012-08-02 18:19:14 PDT
Re-opened since this is blocked by 93055
Comment 26 David Grogan 2012-08-03 13:58:54 PDT
Created attachment 156451 [details]
Patch
Comment 27 David Grogan 2012-08-03 14:19:03 PDT
Does anyone care to comment on (1) below?  If I don't hear anything by monday sometime I'll resubmit.

There were 3 compile problems on windows

1) There's no PRId64 printf conversion specifier for int64_t, so IDBDatabaseBackendImpl.cpp now casts to long long and uses %lld.  I still want to store integer versions as int64_t to match the WebIDL definition of long long and so that we know exactly how many bytes we have to write to leveldb.

2) IDBLevelDBBackingStore::createIDBDatabaseMetadata had a non-const parameter and a derived class had const

3) IDBCallbacks.h needed some #includes
Comment 28 Joshua Bell 2012-08-03 16:14:15 PDT
(In reply to comment #27)
> Does anyone care to comment on (1) below?  If I don't hear anything by monday sometime I'll resubmit.
> 
> There were 3 compile problems on windows
> 
> 1) There's no PRId64 printf conversion specifier for int64_t, so IDBDatabaseBackendImpl.cpp now casts to long long and uses %lld.  I still want to store integer versions as int64_t to match the WebIDL definition of long long and so that we know exactly how many bytes we have to write to leveldb.

SGTM - verify it compiles everywhere with a git-try?

> 
> 2) IDBLevelDBBackingStore::createIDBDatabaseMetadata had a non-const parameter and a derived class had const
> 
> 3) IDBCallbacks.h needed some #includes
Comment 29 David Grogan 2012-08-03 16:15:49 PDT
(In reply to comment #28)
> (In reply to comment #27)
> > Does anyone care to comment on (1) below?  If I don't hear anything by monday sometime I'll resubmit.
> > 
> > There were 3 compile problems on windows
> > 
> > 1) There's no PRId64 printf conversion specifier for int64_t, so IDBDatabaseBackendImpl.cpp now casts to long long and uses %lld.  I still want to store integer versions as int64_t to match the WebIDL definition of long long and so that we know exactly how many bytes we have to write to leveldb.
> 
> SGTM - verify it compiles everywhere with a git-try?

I did win_rel and mac_rel, but now that you mention it, I should really try everything.

> 
> > 
> > 2) IDBLevelDBBackingStore::createIDBDatabaseMetadata had a non-const parameter and a derived class had const
> > 
> > 3) IDBCallbacks.h needed some #includes
Comment 30 WebKit Review Bot 2012-08-03 17:28:42 PDT
Comment on attachment 156451 [details]
Patch

Clearing flags on attachment: 156451

Committed r124675: <http://trac.webkit.org/changeset/124675>
Comment 31 WebKit Review Bot 2012-08-03 17:28:47 PDT
All reviewed patches have been landed.  Closing bug.