IndexedDB: Recycle cursor objects when calling continue().

content/browser/in_process_webkit/indexed_db_callbacks.cc

Index: content/browser/in_process_webkit/indexed_db_callbacks.cc
diff --git a/content/browser/in_process_webkit/indexed_db_callbacks.cc b/content/browser/in_process_webkit/indexed_db_callbacks.cc
index 01b4b637e69280e9e00f242927f7a8b966002e52..3482b29476259a28a6181990202a77b5f42f871e 100644
--- a/content/browser/in_process_webkit/indexed_db_callbacks.cc
+++ b/content/browser/in_process_webkit/indexed_db_callbacks.cc
@@ -41,6 +41,20 @@ void IndexedDBCallbacks<WebKit::WebIDBCursor>::onSuccess(
           response_id(), content::SerializedScriptValue(value)));
 }
 
+void IndexedDBCallbacks<WebKit::WebIDBCursor>::onSuccessCursorContinue() {

[michaeln, 2011/10/30 21:54:06]

Would it make sense to pass the WebIDBCursor as a parameter to this method
instead of doing map look ups?

[hans, 2011/10/31 16:11:52]

Can't change the function's signature since it's an override of
WebIDBCallbacks::onSuccessCursorContinue(), but I can pass in the cursor id when
creating the callbacks object. That lets us get rid of the
pending_cursor_continues_ as you suggest in a later comment.

+  int32 cursor_id = dispatcher_host()->GetAndConsumeCursorIdForPendingContinue(
+      response_id());
+  WebKit::WebIDBCursor* idb_cursor = dispatcher_host()->getCursorFromId(
+      cursor_id);
+

[michaeln, 2011/10/30 21:54:06]

What ensures that the idb_cursor isn't deleted between the continue call and the
completion call? Maybe this is entirely synchronous such that
onSuccessCursorContinue() is invoked prior to return from the continue() method?

[hans, 2011/10/31 16:11:52]

It's not synchronous.

It's actually the idb_cursor object that's calling the onSuccessCursorContinue()
method. So if it's been deleted, we would probably have already crashed.

The deletion happens when the RendererWebIDBCursorImpl is destroyed in the
renderer process, causing an IndexedDBHostMsg_CursorDestroyed to be sent to the
browser process.

There's a reassuring comment in ~RendererWebIDBCursorImpl(), saying "It's not
possible for there to be pending callbacks that address this object since inside
WebKit, they hold a reference to the object wich owns this object." So I think
it's safe.

[michaeln, 2011/10/31 18:58:25]

That comment isn't so reassuring since there are no pending callbacks that
address RendererWebIDBCursorImpl at all. Thusfar, each step gets a new instance.

Here's the case i'm thinking about.

* cursor.continue(); cursor = null;   // so there is no longer a js/v8 ref to
the thing
* return to event loop
* gc happens  // so the cursor js/v8 wrapper gets deleted
* continue completion occurs  // BOOM, id is not in maps at some point in the
chain

What actually prevents RendererWebIDBCursorImpl from being deleted in there (and
then propagating that deletion back thru), and if nothing prevents that, how
should that case be handled. I'm wondering if need to either take measures to
'keep it alive' or take measures to 'recreate as needed'.

+  dispatcher_host()->Send(
+      new IndexedDBMsg_CallbacksSuccessCursorContinue(
+          response_id(),
+          IndexedDBKey(idb_cursor->key()),
+          IndexedDBKey(idb_cursor->primaryKey()),
+          content::SerializedScriptValue(idb_cursor->value())));
+}
+
 void IndexedDBCallbacks<WebKit::WebIDBKey>::onSuccess(
     const WebKit::WebIDBKey& value) {
   dispatcher_host()->Send(