Bug 108670

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>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, dgrogan, haraken, kbr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 102716    
Bug Blocks:    
Attachments:
Description Flags
Attempted repro
none
Patch
none
Patch none

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 6 Joshua Bell 2013-02-07 16:49:46 PST
Created attachment 187201 [details]
Patch
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 9 Joshua Bell 2013-02-08 12:42:22 PST
Created attachment 187350 [details]
Patch
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 11 Kentaro Hara 2013-02-10 17:43:05 PST
Comment on attachment 187350 [details]
Patch

LGTM
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.