Bug 108670 – [V8] IndexedDB: Minor GC can collect IDBDatabase wrapper with versionchange handler

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joshua Bell]

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.

[Joshua Bell]

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.

[Joshua Bell]

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.

[Joshua Bell]

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

[Joshua Bell]

Possibly related to wkbug.com/102716 though.

[Joshua Bell]

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

[Joshua Bell]

Created attachment 187201 [details]
Patch

[Kentaro Hara]

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() ?

[Joshua Bell]

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.

[Joshua Bell]

Created attachment 187350 [details]
Patch

[Joshua Bell]

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.

[Kentaro Hara]

Comment on attachment 187350 [details]
Patch

LGTM

[WebKit Review Bot]

Comment on attachment 187350 [details]
Patch

Clearing flags on attachment: 187350

Committed r142483: <http://trac.webkit.org/changeset/142483>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.