|Summary:||[V8] IndexedDB: Minor GC can collect IDBDatabase wrapper with versionchange handler|
|Product:||WebKit||Reporter:||Joshua Bell <jsbell>|
|Component:||WebCore Misc.||Assignee:||Joshua Bell <jsbell>|
|Severity:||Normal||CC:||alecflett, dgrogan, haraken, kbr, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:||102716|
Description Joshua Bell 2013-02-01 11:45:09 PST
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.
Comment 1 Joshua Bell 2013-02-01 11:46:42 PST
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.
Comment 2 Joshua Bell 2013-02-01 12:23:57 PST
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.
Comment 3 Joshua Bell 2013-02-01 13:58:18 PST
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
Comment 4 Joshua Bell 2013-02-01 14:00:00 PST
Possibly related to wkbug.com/102716 though.
Comment 5 Joshua Bell 2013-02-07 16:18:31 PST
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
Comment 7 Kentaro Hara 2013-02-07 16:58:06 PST
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() ?
Comment 8 Joshua Bell 2013-02-07 17:08:19 PST
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.
Comment 10 Joshua Bell 2013-02-08 12:48:07 PST
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 12 WebKit Review Bot 2013-02-11 09:30:01 PST
Comment on attachment 187350 [details] Patch Clearing flags on attachment: 187350 Committed r142483: <http://trac.webkit.org/changeset/142483>
Comment 13 WebKit Review Bot 2013-02-11 09:30:05 PST
All reviewed patches have been landed. Closing bug.