Bug 55095 – IndexedDB: fire versionchange events when calling setVersion

• Component: New Bugs
• Status: RESOLVED FIXED

[David Grogan]

IndexedDB: fire versionchange events when calling setVersion

[David Grogan]

Created attachment 83566 [details]
obsoleted Patch 1

[David Grogan]

Created attachment 83570 [details]
obsolete patch 2

[Jeremy Orlow]

Comment on attachment 83570 [details]
obsolete patch 2

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

overall looks pretty good...minus the connection id thing

> LayoutTests/storage/indexeddb/set_version_queue.html:71
> +    versionChangeNotifications.push("connection " + connectionNum +

no need to wrap..and you could maybe just put it in the closure above?

> Source/WebCore/ChangeLog:12
> +        4) When a connections call setVersion, backend calls

When a connection calls setVersion, the backend calls

> Source/WebCore/storage/IDBDatabase.cpp:177
> +bool IDBDatabase::dispatchEvent(PassRefPtr<Event> event)

Is there any reason the default dispatchEvent won't do?

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:50
> +    int connectionId() { return m_connectionId; }

newline before private..

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:218
> +void IDBDatabaseBackendImpl::open(int connectionId, PassRefPtr<IDBDatabaseCallbacks> callbacks)

You should just have one version of this function.  And it should simply take in the PassRefPtr

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:229
> +void IDBDatabaseBackendImpl::close(int connectionId)

This should take in an IDBDatabaseCallbacks ptr...and assert it's there before you remove it.

> Source/WebCore/storage/IDBDatabaseBackendImpl.h:101
> +    int m_nextId;

no longer necessary

> Source/WebCore/storage/IDBDatabaseBackendImpl.h:106
> +    typedef HashMap<int, RefPtr<IDBDatabaseCallbacks> > DatabaseCallbacksMap;

This can probably just be a HashSet.  Actually use a LinkHashSet so that you can iterate over them deterministically.  Make a comment that this is important.

> Source/WebCore/storage/IDBDatabaseCallbacks.h:34
> +#include "SerializedScriptValue.h"

not needed...you likely will need PlatformString. if you delete tho.

> Source/WebCore/storage/IDBRequest.cpp:154
> +

extra newline

> Source/WebKit/chromium/public/WebIDBDatabaseCallbacks.h:38
> +

no newlines

> Source/WebKit/chromium/src/IDBDatabaseCallbacksProxy.h:55
> +

no double newline

[David Grogan]

Comment on attachment 83570 [details]
obsolete patch 2

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

Switching away from connectionId isn't going smoothly.  I misunderstood the alternative (using the callbacks objects themselves as ids) you described, but I'm not sure where :(  I outlined the changes below that I think you talked about.  If I do them, it doesn't seem possible to compare the set of IDBDatabaseCallbacks (really IDBDatabaseCallbacksProxy) objects known to IDBDatabaseBackendImpl with the objects coming in to IDBDatabaseBackendImpl::close and IDBDatabaseBackendImpl::setVersion.  Could you let me know where I went wrong?

> Source/WebCore/storage/IDBDatabase.cpp:107
> +    m_backend->setVersion(version, request, m_connectionId, ec);

"m_connectionId" would change to "this"

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:165
> +void IDBDatabaseBackendImpl::setVersion(const String& version, PassRefPtr<IDBCallbacks> prpCallbacks, int connectionId, ExceptionCode& ec)

Say I change "int connectionId" to "PassRefPtr<IDBDatabaseCallbacks> prpDBCallbacks", [next comment]

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:174
> +        if (it->first != connectionId)

how do I preserve this check to not fire a versionchange event at the connection that initiated the version change?  It seems like I need to compare a RefPtr<IDBDatabaseCallbacksProxy> with each element in a ListHashSet<RefPtr<IDBDatabaseCallbacksProxy> >, but the IDBDatabaseCallbacksProxy objects are all different even though they wrap some of the same IDBDatabase objects.

>> Source/WebCore/storage/IDBDatabaseBackendImpl.h:106
>> +    typedef HashMap<int, RefPtr<IDBDatabaseCallbacks> > DatabaseCallbacksMap;
> 
> This can probably just be a HashSet.  Actually use a LinkHashSet so that you can iterate over them deterministically.  Make a comment that this is important.

What should the exact type of this be?  Right now I've got ListHashSet<RefPtr<IDBDatabaseCallbacks> >

> Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:84
> +void WebIDBDatabaseImpl::setVersion(const WebString& version, WebIDBCallbacks* callbacks, int connectionId, WebExceptionCode& ec)

If I understand you correctly, "int connectionId" would change to "WebIDBDatabaseCallbacks* dbCallbacks"

> Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:86
> +    m_databaseBackend->setVersion(version, IDBCallbacksProxy::create(callbacks), connectionId, ec);

and "connectionId" would change to "IDBDatabaseCallbacksProxy::create(dbCallbacks)"

[Jeremy Orlow]

(In reply to comment #4)
> (From update of attachment 83570 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83570&action=review
> 
> Switching away from connectionId isn't going smoothly.  I misunderstood the alternative (using the callbacks objects themselves as ids) you described, but I'm not sure where :(  I outlined the changes below that I think you talked about.  If I do them, it doesn't seem possible to compare the set of IDBDatabaseCallbacks (really IDBDatabaseCallbacksProxy) objects known to IDBDatabaseBackendImpl with the objects coming in to IDBDatabaseBackendImpl::close and IDBDatabaseBackendImpl::setVersion.  Could you let me know where I went wrong?
> 
> > Source/WebCore/storage/IDBDatabase.cpp:107
> > +    m_backend->setVersion(version, request, m_connectionId, ec);
> 
> "m_connectionId" would change to "this"
> 
> > Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:165
> > +void IDBDatabaseBackendImpl::setVersion(const String& version, PassRefPtr<IDBCallbacks> prpCallbacks, int connectionId, ExceptionCode& ec)
> 
> Say I change "int connectionId" to "PassRefPtr<IDBDatabaseCallbacks> prpDBCallbacks", [next comment]
> 
> > Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:174
> > +        if (it->first != connectionId)
> 
> how do I preserve this check to not fire a versionchange event at the connection that initiated the version change?  It seems like I need to compare a RefPtr<IDBDatabaseCallbacksProxy> with each element in a ListHashSet<RefPtr<IDBDatabaseCallbacksProxy> >, but the IDBDatabaseCallbacksProxy objects are all different even though they wrap some of the same IDBDatabase objects.

I have some ideas....let's talk tomorrow.  It may not be practical tho...

[David Grogan]

Created attachment 84099 [details]
Patch

[David Grogan]

Comment on attachment 83570 [details]
obsolete patch 2

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

>> LayoutTests/storage/indexeddb/set_version_queue.html:71
>> +    versionChangeNotifications.push("connection " + connectionNum +
> 
> no need to wrap..and you could maybe just put it in the closure above?

done

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:50
>> +    int connectionId() { return m_connectionId; }
> 
> newline before private..

done

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:218
>> +void IDBDatabaseBackendImpl::open(int connectionId, PassRefPtr<IDBDatabaseCallbacks> callbacks)
> 
> You should just have one version of this function.  And it should simply take in the PassRefPtr

done

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:229
>> +void IDBDatabaseBackendImpl::close(int connectionId)
> 
> This should take in an IDBDatabaseCallbacks ptr...and assert it's there before you remove it.

done

>> Source/WebCore/storage/IDBDatabaseBackendImpl.h:101
>> +    int m_nextId;
> 
> no longer necessary

done

>> Source/WebCore/storage/IDBDatabaseCallbacks.h:34
>> +#include "SerializedScriptValue.h"
> 
> not needed...you likely will need PlatformString. if you delete tho.

done

>> Source/WebCore/storage/IDBRequest.cpp:154
>> +
> 
> extra newline

done

>> Source/WebKit/chromium/public/WebIDBDatabaseCallbacks.h:38
>> +
> 
> no newlines

done

>> Source/WebKit/chromium/src/IDBDatabaseCallbacksProxy.h:55
>> +
> 
> no double newline

done

[David Grogan]

Comment on attachment 84099 [details]
Patch

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

The spec doesn't seem to anticipate a connection calling setVersion but closing before its versionchange events get sent to the other connections or its blocked event getting fired.  Should those events fire?  The spec, in 4.7.4, is clear that such a connection shouldn't receive the versionchange events from OTHER connections, but doesn't mention its own events.

The code in this patch prevents the blocked event from being delivered to the closed connection (because the text of 4.7.4 and 4.7.5 could, possibly, be tortured to indicate this is proper) but allows the other connections to receive versionchange events originating from the closed connection.  I suspect that they shouldn't receive such events, though.

> Source/WebCore/storage/IDBDatabase.cpp:168
> +void IDBDatabase::enqueueEvent(PassRefPtr<Event> event)

this override seems necessary to call event->setTarget(this);  True?

> Source/WebCore/storage/IDBDatabase.cpp:179
> +    if (m_noNewTransactions)

This behavior is in the spec.

But is it kosher to withhold events in ::dispatchEvent or do I need to use something like the local event queue from your outstanding transaction patch?

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:177
> +    if (m_databaseCallbacksSet.size() > 1) {

I'm not sure how correct this is.  The spec says:
4.7.4: fire versionchange events
4.7.5: "If running asynchronously and any of the connections in openDatabases are still not closed, fire a blocked event at request."  That "still" seems to indicate that the other connections should be given a chance to close before firing onblocked at the connection calling setVersion.  But this implementation does no such thing, it just fires away.  Does the spec wording need to be changed, or does this code?

[Jeremy Orlow]

Comment on attachment 84099 [details]
Patch

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

almost there

> Source/WebCore/storage/IDBDatabase.h:90
> +    virtual bool dispatchEvent(PassRefPtr<Event>);

usually we dont' interleave virtual and non-virtual...but this is going away anyway

> Source/WebCore/storage/IDBDatabaseCallbacks.h:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

2011

> Source/WebCore/storage/IDBDatabaseCallbacks.h:10
> + * 2.  Redistributions in binary form must reproduce the above copyright

Use the 2 clause license here and elsewhere: http://www.webkit.org/coding/bsd-license.html

> Source/WebKit/chromium/public/WebIDBDatabaseCallbacks.h:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

2011

> Source/WebKit/chromium/src/IDBDatabaseCallbacksProxy.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

2011

> Source/WebKit/chromium/src/IDBDatabaseCallbacksProxy.cpp:34
> +#if ENABLE(INDEXED_DATABASE)

move between #includes

> Source/WebKit/chromium/src/IDBDatabaseCallbacksProxy.h:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

2011

> Source/WebKit/chromium/src/IDBDatabaseCallbacksProxy.h:32
> +#include "IDBDatabaseCallbacks.h"

move inside guard

> Source/WebKit/chromium/src/IDBDatabaseCallbacksProxy.h:37
> +#include <wtf/PassRefPtr.h>

do you need this?

> Source/WebKit/chromium/src/WebIDBDatabaseCallbacksImpl.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

2011

> Source/WebKit/chromium/src/WebIDBDatabaseCallbacksImpl.cpp:29
> +

extra newline

> Source/WebKit/chromium/src/WebIDBDatabaseCallbacksImpl.h:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

2011

> Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:102
> +    // use saved callbacks

Use full sentences.  Don't say what you're doing, say why.

> Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:108
> +    m_databaseCallbacks = IDBDatabaseCallbacksProxy::create(callbacks);

ASSERT(!m_databaseCallbacks) first

[David Grogan]

Created attachment 84351 [details]
Patch

[David Grogan]

Comment on attachment 84099 [details]
Patch

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

>> Source/WebCore/storage/IDBDatabase.h:90
>> +    virtual bool dispatchEvent(PassRefPtr<Event>);
> 
> usually we dont' interleave virtual and non-virtual...but this is going away anyway

I kept it around to help make multi-process and single-process chrome do the same things.

>> Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:102
>> +    // use saved callbacks
> 
> Use full sentences.  Don't say what you're doing, say why.

D'oh sorry, I meant to flesh this out.

[David Grogan]

Comment on attachment 84351 [details]
Patch

If this looks ok I'll break out the public webkit interface stuff.

[Jeremy Orlow]

(In reply to comment #11)
> (From update of attachment 84099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84099&action=review
> 
> >> Source/WebCore/storage/IDBDatabase.h:90
> >> +    virtual bool dispatchEvent(PassRefPtr<Event>);
> > 
> > usually we dont' interleave virtual and non-virtual...but this is going away anyway
> 
> I kept it around to help make multi-process and single-process chrome do the same things.

Why was this necessary.  In the future, when you hit stuff that might require a complicated workaround like this, please just ask before writing the code...and explain somewhere why you did what you did.  (Probably in the change log and here.)
 
> >> Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:102
> >> +    // use saved callbacks
> > 
> > Use full sentences.  Don't say what you're doing, say why.
> 
> D'oh sorry, I meant to flesh this out.

[Jeremy Orlow]

Comment on attachment 84351 [details]
Patch

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

(In reply to comment #12)
> (From update of attachment 84351 [details])
> If this looks ok I'll break out the public webkit interface stuff.

Please break it out now as this stuff needs to be 100% landed in 3 days and it'll require a webkit roll in between.

> Source/WebKit/chromium/public/WebIDBDatabaseCallbacks.h:32
> +namespace WebKit {

newline after

[Jeremy Orlow]

Comment on attachment 84351 [details]
Patch

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

> Source/WebCore/storage/IDBDatabase.cpp:145
> +    EventQueue* eventQueue = static_cast<Document*>(scriptExecutionContext())->eventQueue();

assert it's a document before static casting (IsDocument())

> Source/WebCore/storage/IDBDatabase.cpp:146
> +    for (size_t i = 0; i < m_enqueuedEvents.size(); ++i) {

Is this really necessary?  What's the worst case without it?

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:170
> +        callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::NOT_ALLOWED_ERR, "Connection was closed before set version transaction was created"));

Did we agree on NOT_ALLOWED_ERR?  ABORT_ERR might make more sense?

Is this possible?  Seems like it wouldn't be and that we should be blocking it in the frontend.  If so, just do an assert.  If not, why isn't it?

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:173
> +    for (DatabaseCallbacksSet::const_iterator it = m_databaseCallbacksSet.begin(); it != m_databaseCallbacksSet.end(); ++it) {

Are we supposed to send this even when nothing's blocked?

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:234
>          RefPtr<PendingSetVersionCall> pendingSetVersionCall = m_pendingSetVersionCalls.takeFirst();

Can you update the code I just landed to use takeFirst (in EventQueue).  That seems better.

> Source/WebCore/storage/IDBRequest.cpp:193
> +

newline seems kinda random...but no big deal

> Source/WebKit/chromium/src/IDBDatabaseProxy.cpp:95
> +void IDBDatabaseProxy::setVersion(const String& version, PassRefPtr<IDBCallbacks> callbacks, PassRefPtr<IDBDatabaseCallbacks> dbcallbacks, ExceptionCode& ec)

databaseCallbacks  (don't abbreviate and camelCase)

here and elsewhere

> Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:102
> +    // Use the callbacks that ::open gave us to ensure that the backend in

ASSERT they're not null before passing in.  (The only time we wouldn't assert is if we were about to do something that'd _crash_ if it was null.)

[David Grogan]

(In reply to comment #14)
> (From update of attachment 84351 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84351&action=review
> 
> (In reply to comment #12)
> > (From update of attachment 84351 [details] [details])
> > If this looks ok I'll break out the public webkit interface stuff.
> 
> Please break it out now as this stuff needs to be 100% landed in 3 days and it'll require a webkit roll in between.
> 
> > Source/WebKit/chromium/public/WebIDBDatabaseCallbacks.h:32
> > +namespace WebKit {
> 
> newline after

sorry for missing this newline, thanks for adding it

[David Grogan]

Created attachment 84446 [details]
Patch

[David Grogan]

Comment on attachment 84351 [details]
Patch

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

>> Source/WebCore/storage/IDBDatabase.cpp:145
>> +    EventQueue* eventQueue = static_cast<Document*>(scriptExecutionContext())->eventQueue();
> 
> assert it's a document before static casting (IsDocument())

done

>> Source/WebCore/storage/IDBDatabase.cpp:146
>> +    for (size_t i = 0; i < m_enqueuedEvents.size(); ++i) {
> 
> Is this really necessary?  What's the worst case without it?

A closed database would get a versionchange event, forbidden by the note in step 4 here:
http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#dfn-steps-for-running-a-version_change-transaction

I can remove it if you think it adds too much ugliness.

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:170
>> +        callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::NOT_ALLOWED_ERR, "Connection was closed before set version transaction was created"));
> 
> Did we agree on NOT_ALLOWED_ERR?  ABORT_ERR might make more sense?
> 
> Is this possible?  Seems like it wouldn't be and that we should be blocking it in the frontend.  If so, just do an assert.  If not, why isn't it?

I just used NOT_ALLOWED_ERR as a placeholder, figuring I'd get back to it or you'd bring it up if there was something better.  ABORT_ERR is better, changing.

This is possible and happens in the layout test.  I don't think we can easily check for it in the frontend but could be mistaken.  It happens because a setVersion _transaction_ isn't created until the setversion call no longer blocked.  E.g.:

connection1.open
connection2.open
connection1.setversion
connection2.setversion

function onConnection1Blocked() {
  connection2.close();
}
... bunch of stuff ...
connection1.close() // only now does connection2's pendingSetVersionCall get serviced and potentially turned into a setVersion transaction.

I think this is also consistent with the every-request-should-get-an-error-or-success-call idea.

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:173
>> +    for (DatabaseCallbacksSet::const_iterator it = m_databaseCallbacksSet.begin(); it != m_databaseCallbacksSet.end(); ++it) {
> 
> Are we supposed to send this even when nothing's blocked?

If nothing's blocked then m_databaseCallbacksSet.size() will be 1 and the control flow will not get passed the if statement on the line below, so nothing will be sent.

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:234
>>          RefPtr<PendingSetVersionCall> pendingSetVersionCall = m_pendingSetVersionCalls.takeFirst();
> 
> Can you update the code I just landed to use takeFirst (in EventQueue).  That seems better.

Compiler error.  Turns out takeFirst is only defined for WTF::Deque, not WTF::ListHashSet.

>> Source/WebKit/chromium/src/IDBDatabaseProxy.cpp:95
>> +void IDBDatabaseProxy::setVersion(const String& version, PassRefPtr<IDBCallbacks> callbacks, PassRefPtr<IDBDatabaseCallbacks> dbcallbacks, ExceptionCode& ec)
> 
> databaseCallbacks  (don't abbreviate and camelCase)
> 
> here and elsewhere

done

>> Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:102
>> +    // Use the callbacks that ::open gave us to ensure that the backend in
> 
> ASSERT they're not null before passing in.  (The only time we wouldn't assert is if we were about to do something that'd _crash_ if it was null.)

Added ASSERT.

Why wouldn't we assert if a null would cause a crash?

[David Grogan]

Comment on attachment 84099 [details]
Patch

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

>>>> Source/WebCore/storage/IDBDatabase.h:90
>>>> +    virtual bool dispatchEvent(PassRefPtr<Event>);
>>> 
>>> usually we dont' interleave virtual and non-virtual...but this is going away anyway
>> 
>> I kept it around to help make multi-process and single-process chrome do the same things.
> 
> Why was this necessary.  In the future, when you hit stuff that might require a complicated workaround like this, please just ask before writing the code...and explain somewhere why you did what you did.  (Probably in the change log and here.)

Added some explanation to IDBDatabase.h and IDBDatabase::close, will follow up in person.

[Jeremy Orlow]

Comment on attachment 84446 [details]
Patch

r=me

[Jeremy Orlow]

Comment on attachment 84446 [details]
Patch

Clearing flags on attachment: 84446

Committed r80183: <http://trac.webkit.org/changeset/80183>

[Jeremy Orlow]

All reviewed patches have been landed.  Closing bug.