Reported by a user: > I am getting this behaviour on IDBDatabase when it should be firing a > versionchange event. > > Step 1. IDBDatabase object is assigned an onversionchange event but no > reference is kept. > Step 2. IDBFactory.open is called with a higher version number. > Step 3. Garbage collection comes along and eats the all references to the > first IDBDatabase, but fails to close it. Cause? If it has read or written > to the database? > The versionchange event can't force a call to close() because it is never > called. > Result: The IDBOpenRequest from Step 2 is forever blocked.
Just off the top of my head, it seems like IDBDatabase should implement ActiveDOMObject::hasPendingActivity() and return true until the connection is closed, since versionchange could fire on it at any time.
Alternately, the bug could be the fact that it's not closing when the wrapper is collected. If all script references to the database are dropped I wouldn't necessarily expect the versionchange handler to keep it alive.
Created attachment 186131 [details] Attempted repro So far, no luck trying to repro by: open connection assign versionchange handler (w/o connection reference in closure) drop all references force GC open second connection At this point I see: * the versionchange handler fire (which would seem to indicate that my GCing was unsuccessful) * an arbitrarily long pause (up to 20 seconds?); pause can be shortened (in chrome) by opening the inspector. Presumably this is waiting for an actual GC. * the second connection opens successfully, indicating that the first connection was finally closed
Possibly related to wkbug.com/102716 though.
The reporter confirms that what he was trying matches the description: "ensuring there were no references to the database connection, yet neither seeing the versionchange handler called nor eventually seeing subsequent open calls succeed" (my words, though). Repro cases posted: Open a transaction and make an IDBRequest on an IDBObjectStore but don't assign the onerror/onabort handlers. https://dl.dropbox.com/u/12865946/IndexedDB/108670b.html or Request IDBIndex objects from an IDBObjectStore and keep references of them. https://dl.dropbox.com/u/12865946/IndexedDB/108670.html
Created attachment 187201 [details] Patch
Comment on attachment 187201 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187201&action=review > Source/WebCore/ChangeLog:11 > + No new tests - TODO before landing. Yeah, a test case please. You can write a reliable test case by using gc(), which explicitly triggers full garbage collection. > Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:362 > + // FIXME: Only keep the wrapper alive if there are event listeners. Is it difficult to check whether event listeners are attached or not? > Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:363 > + return !m_closePending || ActiveDOMObject::hasPendingActivity(); Why do you need ActiveDOMObject::hasPendingActivity() ?
Sorry, didn't meant to indicate this was ready for r w/o tests. > > Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:362 > > + // FIXME: Only keep the wrapper alive if there are event listeners. > > Is it difficult to check whether event listeners are attached or not? I just noticed EventTarget::hasEventListeners() - I'll play with it. Unfortunately, it's not marked const but ActiveDOMObject::hasPendingActivity() is const, so I may need to land a const-ifying patch for it first. > > Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:363 > > + return !m_closePending || ActiveDOMObject::hasPendingActivity(); > > Why do you need ActiveDOMObject::hasPendingActivity() ? Copy/paste. It looks like the other IDB implementations of the method call the base class method, even though it's not necessary. I'll remove those.
Created attachment 187350 [details] Patch
Calling hasEventListeners() wasn't going to happen; to make that const would require making eventTargetData() const which didn't appear tractable. Fortunately, the inline version is trivial. Added a test matching the repro provided. In DRT w/o the patch the test shows onVersionChange is not called. However, it also doesn't "hang" waiting on GC to eventually claim the IDBDatabase object (unlike running the repro in Chrome) probably due to GC differences in DRT.
Comment on attachment 187350 [details] Patch LGTM
Comment on attachment 187350 [details] Patch Clearing flags on attachment: 187350 Committed r142483: <http://trac.webkit.org/changeset/142483>
All reviewed patches have been landed. Closing bug.