V8CustomIDBCallbacks<> should not hold a reference to the frame
Created attachment 52644 [details] Patch
This addresses the issues discussed in our email thread.
Mads, I'm not sure if what I'm doing with the contexts is OK. My main hint that it's not when onError() is called with the code that provided the callback on the call stack, things work fine. But when I trigger the callback without anything WebKit on the call stack (i.e. it's triggered by an IPC) then things act quite odd. (The inspector aw snaps if you open it and changes don't show up on screen.) Any ideas of what I might be doing wrong? (Note that this behavior was true before and after the patch.)
+ v8::Persistent<v8::Object> m_onSuccess; + v8::Persistent<v8::Object> m_onError; Please use OwnHandle instead to avoid manual new/dispose. + class V8CustomIDBCallbacks : public IDBCallbacks<ResultType> { I wish the base class where were more abstract so it could be used by non-DB callbacks. As another approach, you could make a more complex object for m_onSuccess instead of just holding a V8 handle. That object could encapsulate the world handle / activeDOMObject logic. Then V8CustomIDBCallbacks could be a simple composition of the callbacks.
(In reply to comment #4) > + v8::Persistent<v8::Object> m_onSuccess; > + v8::Persistent<v8::Object> m_onError; > > Please use OwnHandle instead to avoid manual new/dispose. Oooo. Nice. > + class V8CustomIDBCallbacks : public IDBCallbacks<ResultType> { > > I wish the base class where were more abstract so it could be used by non-DB > callbacks. > > As another approach, you could make a more complex object for m_onSuccess > instead of just holding a V8 handle. That object could encapsulate the world > handle / activeDOMObject logic. Then V8CustomIDBCallbacks could be a simple > composition of the callbacks. A little while ago I actually tried to do that, but (IIRC) I found that I either had to make the container class a factory that you inject the individual callback classes into or you have to subclass both the container class and the individual sub-classes. I'm not against making the code more complex in the interest of re-use, but I really think we should get at very least |window.indexedDB.open()| always calling the error callback before we worry too much about it.
(In reply to comment #5) > (In reply to comment #4) > > + v8::Persistent<v8::Object> m_onSuccess; > > + v8::Persistent<v8::Object> m_onError; > > > > Please use OwnHandle instead to avoid manual new/dispose. > > Oooo. Nice. Unfortunately, I think this will need to wait because changing it would require me to change VoidCallback. And I want to do that once we have our story fully sorted out for IndexedDB. I'll add a FIXME for now though.
Created attachment 52656 [details] Patch
Comment on attachment 52656 [details] Patch Ok. I'm not opposed to an incremental approach. :) I presume you'll add tests once you've gotten it fully fixed?
Attachment 52656 [details] was posted by a committer and has review+, assigning to Jeremy Orlow for commit.
(In reply to comment #8) > (From update of attachment 52656 [details]) > Ok. I'm not opposed to an incremental approach. :) > > I presume you'll add tests once you've gotten it fully fixed? Will test this code and refractor the rest of the code as soon as this stuff is working.
Committed r57203: <http://trac.webkit.org/changeset/57203>