Bug 78167 - [Chromium] IndexedDB: IDBVersionChangeRequest V8 wrapper not generated as ActiveDOMObject
Summary: [Chromium] IndexedDB: IDBVersionChangeRequest V8 wrapper not generated as Act...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks: 78004
  Show dependency treegraph
 
Reported: 2012-02-08 15:37 PST by Joshua Bell
Modified: 2012-02-09 17:54 PST (History)
8 users (show)

See Also:


Attachments
Repro case (850 bytes, text/html)
2012-02-08 15:37 PST, Joshua Bell
no flags Details
Patch (5.94 KB, patch)
2012-02-08 17:19 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (5.93 KB, patch)
2012-02-09 09:57 PST, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-02-08 15:37:31 PST
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?
Comment 1 Joshua Bell 2012-02-08 17:19:11 PST
Created attachment 126199 [details]
Patch
Comment 2 Joshua Bell 2012-02-08 17:20:33 PST
Comment on attachment 126199 [details]
Patch

Note that the layout test is not terribly exciting, but it will timeout without the IDL change.
Comment 3 David Grogan 2012-02-08 17:26:33 PST
LGTM idb-wise
Comment 4 WebKit Review Bot 2012-02-08 20:35:42 PST
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
Comment 5 Joshua Bell 2012-02-09 09:57:14 PST
Created attachment 126321 [details]
Patch
Comment 6 Joshua Bell 2012-02-09 09:58:01 PST
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?
Comment 7 Adam Barth 2012-02-09 11:08:07 PST
Comment on attachment 126321 [details]
Patch

Yeah, it seems like this should be inherited.
Comment 8 Adam Barth 2012-02-09 11:08:32 PST
@haraken: This looks like a subtle gotcha in the code generator.
Comment 9 WebKit Review Bot 2012-02-09 12:49:50 PST
Comment on attachment 126321 [details]
Patch

Clearing flags on attachment: 126321

Committed r107278: <http://trac.webkit.org/changeset/107278>
Comment 10 WebKit Review Bot 2012-02-09 12:49:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Kentaro Hara 2012-02-09 17:42:26 PST
(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.
Comment 12 Adam Barth 2012-02-09 17:50:56 PST
Isn't it a bug whenever we don't inherit this attribute?  The wrappers will be collected too early, right?
Comment 13 Kentaro Hara 2012-02-09 17:54:59 PST
(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.