| Summary: | IndexedDB: fire versionchange events when calling setVersion | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | David Grogan <dgrogan> | ||||||||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | hans, jochen, jorlow | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Other | ||||||||||||||
| OS: | OS X 10.5 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
David Grogan
2011-02-23 15:41:13 PST
Created attachment 83566 [details]
obsoleted Patch 1
Created attachment 83570 [details]
obsolete patch 2
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 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)" (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... Created attachment 84099 [details]
Patch
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 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? 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 Created attachment 84351 [details]
Patch
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. Comment on attachment 84351 [details]
Patch
If this looks ok I'll break out the public webkit interface stuff.
(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. 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 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.) (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 Created attachment 84446 [details]
Patch
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? 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. Comment on attachment 84446 [details]
Patch
r=me
Comment on attachment 84446 [details] Patch Clearing flags on attachment: 84446 Committed r80183: <http://trac.webkit.org/changeset/80183> All reviewed patches have been landed. Closing bug. |