Bug 92558 – IndexedDB: Core upgradeneeded logic

• Component: New Bugs
• Status: RESOLVED FIXED

[David Grogan]

IndexedDB: Core upgradeneeded logic

[David Grogan]

Created attachment 155071 [details]
Patch

[David Grogan]

Created attachment 155080 [details]
Patch

[David Grogan]

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.

[Alec Flett]

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?

[Alec Flett]

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?

[David Grogan]

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.

[David Grogan]

Created attachment 155096 [details]
Patch

[Joshua Bell]

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.

[David Grogan]

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.

[David Grogan]

Created attachment 155404 [details]
Patch

[David Grogan]

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.

[David Grogan]

(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.

[David Grogan]

Created attachment 155659 [details]
Patch

[David Grogan]

Created attachment 155701 [details]
Patch

[David Grogan]

Tony, could you review this?

It's the first of the patches mentioned in bug 92560.

[David Grogan]

Ojan, could you review this?

[Joshua Bell]

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."

[David Grogan]

Created attachment 155914 [details]
Patch

[David Grogan]

(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.

[Ojan Vafai]

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.

[David Grogan]

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.

[David Grogan]

Created attachment 156200 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 156200 [details]
Patch for landing

Clearing flags on attachment: 156200

Committed r124540: <http://trac.webkit.org/changeset/124540>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 93055

[David Grogan]

Created attachment 156451 [details]
Patch

[David Grogan]

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

[Joshua Bell]

(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

[David Grogan]

(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

[WebKit Review Bot]

Comment on attachment 156451 [details]
Patch

Clearing flags on attachment: 156451

Committed r124675: <http://trac.webkit.org/changeset/124675>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.