Bug 37154

Summary: V8CustomIDBCallbacks<> should not hold a reference to the frame
Product: WebKit Reporter: Jeremy Orlow <jorlow>
Component: New BugsAssignee: Jeremy Orlow <jorlow>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ager, atwilson, dglazkov, dimich, eric, japhet, levin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch abarth: review+

Description Jeremy Orlow 2010-04-06 09:07:31 PDT
V8CustomIDBCallbacks<> should not hold a reference to the frame
Comment 1 Jeremy Orlow 2010-04-06 09:16:37 PDT
Created attachment 52644 [details]
Patch
Comment 2 Jeremy Orlow 2010-04-06 09:17:51 PDT
This addresses the issues discussed in our email thread.
Comment 3 Jeremy Orlow 2010-04-06 09:45:12 PDT
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.)
Comment 4 Adam Barth 2010-04-06 11:56:06 PDT
+     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.
Comment 5 Jeremy Orlow 2010-04-06 12:11:51 PDT
(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.
Comment 6 Jeremy Orlow 2010-04-06 12:26:47 PDT
(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.
Comment 7 Jeremy Orlow 2010-04-06 12:29:21 PDT
Created attachment 52656 [details]
Patch
Comment 8 Adam Barth 2010-04-06 13:14:00 PDT
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?
Comment 9 Eric Seidel (no email) 2010-04-06 23:45:58 PDT
Attachment 52656 [details] was posted by a committer and has review+, assigning to Jeremy Orlow for commit.
Comment 10 Jeremy Orlow 2010-04-07 04:04:33 PDT
(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.
Comment 11 Jeremy Orlow 2010-04-07 04:39:54 PDT
Committed r57203: <http://trac.webkit.org/changeset/57203>