[Webkit-unassigned] [Bug 55095] IndexedDB: fire versionchange events when calling setVersion

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=55095





--- Comment #18 from David Grogan <dgrogan at chromium.org>  2011-03-02 12:16:58 PST ---
(From update of attachment 84351)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.