Bug 78167 – [Chromium] IndexedDB: IDBVersionChangeRequest V8 wrapper not generated as ActiveDOMObject

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

[Joshua Bell]

Created attachment 126167 [details]
Repro case

The IndexedDB implementation relies on the ActiveDOMObject concept to keep certain objects alive when they have pending activity. IDBRequest and its subclass IDBVersionChangeRequest need to be kept alive if they are expecting events to be raised, even if all of the references from script have been dropped.

However, this isn't working for IDBVersionChangeRequest, meaning that sometimes the events from IDBFactory.deleteDatabase() and IDBDatabase.setVersion() never fire. See attached repro; if run in DRT or Chrome with --js-flags='--expose-gc' to expose window.gc() the onsuccess event will never fire.

It appears that the IDBVersionChangeRequest.idl's derivation from IDBRequest is insufficient; the generated V8IDBVersionChangeRequest is missing the logic to ensure that the V8 wrapper is treated as an ActiveDOMObject. With this change:

--- a/Source/WebCore/storage/IDBVersionChangeRequest.idl
+++ b/Source/WebCore/storage/IDBVersionChangeRequest.idl
@@ -27,6 +27,7 @@ module storage {
 
     interface [
         Conditional=INDEXED_DATABASE,
+        ActiveDOMObject,
         EventTarget
     ] IDBVersionChangeRequest : IDBRequest {
         attribute EventListener onblocked;

The following is diff is produced in the generated V8IDBVersionChangeRequest.h/.cpp:

diff -uw 1/V8IDBVersionChangeRequest.h 2/V8IDBVersionChangeRequest.h
--- 1/V8IDBVersionChangeRequest.h	2012-02-08 15:25:48.927992208 -0800
+++ 2/V8IDBVersionChangeRequest.h	2012-02-08 15:26:26.368725887 -0800
@@ -25,7 +25,6 @@
 
 #include "IDBVersionChangeRequest.h"
 #include "V8DOMWrapper.h"
-#include "V8IDBRequest.h"
 #include "WrapperTypeInfo.h"
 #include <v8.h>
 #include <wtf/HashMap.h>
@@ -35,7 +34,7 @@
 
 class V8IDBVersionChangeRequest {
 public:
-    static const bool hasDependentLifetime = V8IDBRequest::hasDependentLifetime;
+    static const bool hasDependentLifetime = true;
     static bool HasInstance(v8::Handle<v8::Value>);
     static v8::Persistent<v8::FunctionTemplate> GetRawTemplate();
     static v8::Persistent<v8::FunctionTemplate> GetTemplate();
@@ -46,6 +45,7 @@
     inline static v8::Handle<v8::Object> wrap(IDBVersionChangeRequest*);
     static void derefObject(void*);
     static WrapperTypeInfo info;
+    static ActiveDOMObject* toActiveDOMObject(v8::Handle<v8::Object>);
     static const int eventListenerCacheIndex = v8DefaultWrapperInternalFieldCount + 0;
     static const int internalFieldCount = v8DefaultWrapperInternalFieldCount + 1;
     static v8::Handle<v8::Object> existingWrapper(IDBVersionChangeRequest*);
@@ -56,7 +56,7 @@
 
 ALWAYS_INLINE v8::Handle<v8::Object> V8IDBVersionChangeRequest::existingWrapper(IDBVersionChangeRequest* impl)
 {
-    return getDOMObjectMap().get(impl);
+    return getActiveDOMObjectMap().get(impl);
 }
 
 v8::Handle<v8::Object> V8IDBVersionChangeRequest::wrap(IDBVersionChangeRequest* impl)

diff -uw 1/V8IDBVersionChangeRequest.cpp 2/V8IDBVersionChangeRequest.cpp
--- 1/V8IDBVersionChangeRequest.cpp	2012-02-08 15:25:48.927992208 -0800
+++ 2/V8IDBVersionChangeRequest.cpp	2012-02-08 15:26:26.368725887 -0800
@@ -37,7 +37,7 @@
 
 namespace WebCore {
 
-WrapperTypeInfo V8IDBVersionChangeRequest::info = { V8IDBVersionChangeRequest::GetTemplate, V8IDBVersionChangeRequest::derefObject, 0, &V8IDBRequest::info };
+WrapperTypeInfo V8IDBVersionChangeRequest::info = { V8IDBVersionChangeRequest::GetTemplate, V8IDBVersionChangeRequest::derefObject, V8IDBVersionChangeRequest::toActiveDOMObject, &V8IDBRequest::info };
 
 namespace IDBVersionChangeRequestInternal {
 
@@ -114,6 +114,10 @@
     return GetRawTemplate()->HasInstance(value);
 }
 
+ActiveDOMObject* V8IDBVersionChangeRequest::toActiveDOMObject(v8::Handle<v8::Object> object)
+{
+    return toNative(object);
+}      
 
 v8::Handle<v8::Object> V8IDBVersionChangeRequest::wrapSlow(IDBVersionChangeRequest* impl)
 {
@@ -128,7 +132,7 @@
 
     if (!hasDependentLifetime)
         wrapperHandle.MarkIndependent();
-    getDOMObjectMap().set(impl, wrapperHandle);
+    getActiveDOMObjectMap().set(impl, wrapperHandle);
     return wrapper;
 }
 
It seems like that should happen "automagically" in CodeGeneratorV8.pm, but perhaps adding the ActiveDOMObject annotation to the derived class is the right fix?

[Joshua Bell]

Created attachment 126199 [details]
Patch

[Joshua Bell]

Comment on attachment 126199 [details]
Patch

Note that the layout test is not terribly exciting, but it will timeout without the IDL change.

[David Grogan]

LGTM idb-wise

[WebKit Review Bot]

Comment on attachment 126199 [details]
Patch

Attachment 126199 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11480044

New failing tests:
platform/chromium/compositing/layout-width-change.html

[Joshua Bell]

Created attachment 126321 [details]
Patch

[Joshua Bell]

Comment on attachment 126321 [details]
Patch

Re-upping to remove some whitespace and try and get a clean cr-linux run.

abarth@ can you review?

[Adam Barth]

Comment on attachment 126321 [details]
Patch

Yeah, it seems like this should be inherited.

[Adam Barth]

@haraken: This looks like a subtle gotcha in the code generator.

[WebKit Review Bot]

Comment on attachment 126321 [details]
Patch

Clearing flags on attachment: 126321

Committed r107278: <http://trac.webkit.org/changeset/107278>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

(In reply to comment #7)
> (From update of attachment 126321 [details])
> Yeah, it seems like this should be inherited.

The inheritance will introduce [ActiveDOMObject] to classes which have not had [ActiveDOMObject] so far, but would it be OK? For example, DedicatedWorkerContext and SharedWorkerContext do not have [ActiveDOMObject] but inherit Worker, which already has [ActiveDOMObject].

IMHO, I am neutral about inheriting IDL attributes. Explicitly writing IDL attributes might be less confusing for people.

[Adam Barth]

Isn't it a bug whenever we don't inherit this attribute?  The wrappers will be collected too early, right?

[Kentaro Hara]

(In reply to comment #12)
> Isn't it a bug whenever we don't inherit this attribute?  The wrappers will be collected too early, right?

Let's discuss the topic in bug 78302.