Bug 37154 – V8CustomIDBCallbacks<> should not hold a reference to the frame

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

V8CustomIDBCallbacks<> should not hold a reference to the frame

[Jeremy Orlow]

Created attachment 52644 [details]
Patch

[Jeremy Orlow]

This addresses the issues discussed in our email thread.

[Jeremy Orlow]

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.)

[Adam Barth]

+     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.

[Jeremy Orlow]

(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.

[Jeremy Orlow]

(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.

[Jeremy Orlow]

Created attachment 52656 [details]
Patch

[Adam Barth]

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?

[Eric Seidel (no email)]

Attachment 52656 [details] was posted by a committer and has review+, assigning to Jeremy Orlow for commit.

[Jeremy Orlow]

(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.

[Jeremy Orlow]

Committed r57203: <http://trac.webkit.org/changeset/57203>