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?
Created attachment 126199 [details] Patch
Comment on attachment 126199 [details] Patch Note that the layout test is not terribly exciting, but it will timeout without the IDL change.
LGTM idb-wise
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
Created attachment 126321 [details] Patch
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 on attachment 126321 [details] Patch Yeah, it seems like this should be inherited.
@haraken: This looks like a subtle gotcha in the code generator.
Comment on attachment 126321 [details] Patch Clearing flags on attachment: 126321 Committed r107278: <http://trac.webkit.org/changeset/107278>
All reviewed patches have been landed. Closing bug.
(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.
Isn't it a bug whenever we don't inherit this attribute? The wrappers will be collected too early, right?
(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.