IndexedDB: Have IDBCursor and IDBRequest explicitly break ref cycles

IndexedDB: Have IDBCursor and IDBRequest explicitly break ref cycles

Until (1) a transaction ends or (2) a cursor hits the end of its
range, an IDBRequest holds on to an IDBCursor as its result. Per spec,
calling continue() or advance() on the cursor re-uses the same
IDBRequest, requiring a reference cycle.

Previously, the cycle was broken explicitly on either of those two
conditions, but until that time a cursor-request pair would "leak",
holding on to potentially large script value results.

This patch makes both classes RefCountedBase::deref() and check
to see if they have a partner object and both refcounts are 1. If
so, the cycle is broken.

Special case cruft for condition #1 is removed to simplify the
code - just rely on GC to reclaim the objects if necessary.

R=alecflett@chromium.org,dgrogan@chromium.org
BUG=225860
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157382

[jsbell, 2013-09-06 00:25:14]

This is another attempt with the same test as http://crrev.com/22918013

[jsbell, 2013-09-06 00:27:51]

https://codereview.chromium.org/23653024/diff/1/Source/modules/indexeddb/IDBA...
File Source/modules/indexeddb/IDBAny.h (right):

https://codereview.chromium.org/23653024/diff/1/Source/modules/indexeddb/IDBA...
Source/modules/indexeddb/IDBAny.h:94: DOMStringList* domStringList();
This may seem unrelated, but without this the request->getResultCursor() call
churns the cursor's reference count, which ends up with recursing in the
cursor's deref().

Using PassRefPtr<> here is not necessary since these accessors never mint a new
RefPtr<>.

[alecflett, 2013-09-06 17:56:34]

lgtm

This is great! So glad to see the opencursornotifier going away!

https://codereview.chromium.org/23653024/diff/1/LayoutTests/storage/indexeddb...
File LayoutTests/storage/indexeddb/cursor-request-cycle-expected.txt (right):

https://codereview.chromium.org/23653024/diff/1/LayoutTests/storage/indexeddb...
LayoutTests/storage/indexeddb/cursor-request-cycle-expected.txt:6: indexedDB =
self.indexedDB || self.webkitIndexedDB || self.mozIndexedDB || self.msIndexedDB
|| self.OIndexedDB;
Not right now, but we really ought to scrub these from our tests...

https://codereview.chromium.org/23653024/diff/1/LayoutTests/storage/indexeddb...
File LayoutTests/storage/indexeddb/cursor-request-cycle.html (right):

https://codereview.chromium.org/23653024/diff/1/LayoutTests/storage/indexeddb...
LayoutTests/storage/indexeddb/cursor-request-cycle.html:48: gc(); // FIXME: Why
is this second call necessary?
I wonder if there's any way to make this test fail when this is fixed? perhaps
test that the numberOfLiveNodes() != beforeCount?

https://codereview.chromium.org/23653024/diff/1/Source/modules/indexeddb/IDBC...
File Source/modules/indexeddb/IDBCursor.h (right):

https://codereview.chromium.org/23653024/diff/1/Source/modules/indexeddb/IDBC...
Source/modules/indexeddb/IDBCursor.h:85: void deref()
can/should be OVERRIDE?

https://codereview.chromium.org/23653024/diff/1/Source/modules/indexeddb/IDBR...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/23653024/diff/1/Source/modules/indexeddb/IDBR...
Source/modules/indexeddb/IDBRequest.cpp:220: void
IDBRequest::checkForReferenceCycle()
It would be neat if there were some primitive around this, i.e.

WTF::RefCountedCycleOwned<T, N>

[jsbell, 2013-09-06 18:07:43]

https://codereview.chromium.org/23653024/diff/1/LayoutTests/storage/indexeddb...
File LayoutTests/storage/indexeddb/cursor-request-cycle-expected.txt (right):

https://codereview.chromium.org/23653024/diff/1/LayoutTests/storage/indexeddb...
LayoutTests/storage/indexeddb/cursor-request-cycle-expected.txt:6: indexedDB =
self.indexedDB || self.webkitIndexedDB || self.mozIndexedDB || self.msIndexedDB
|| self.OIndexedDB;
On 2013/09/06 17:56:35, alecflett wrote:
> Not right now, but we really ought to scrub these from our tests...

ISTR that we were waiting until the *reference* bots (running older Chrome) had
the prefix-less version. dgrogan@ - does that sound familiar? Any idea if we've
hit that milestone?

https://codereview.chromium.org/23653024/diff/1/LayoutTests/storage/indexeddb...
File LayoutTests/storage/indexeddb/cursor-request-cycle.html (right):

https://codereview.chromium.org/23653024/diff/1/LayoutTests/storage/indexeddb...
LayoutTests/storage/indexeddb/cursor-request-cycle.html:48: gc(); // FIXME: Why
is this second call necessary?
On 2013/09/06 17:56:35, alecflett wrote:
> I wonder if there's any way to make this test fail when this is fixed? perhaps
> test that the numberOfLiveNodes() != beforeCount?

This is only needed on the bots and it's flaky, unfortunately - your suggested
test would fail locally and occasionally fail on the bots. :(

Should probably have a tracking bug for "shouldn't have to call gc() twice" but
it's not IDB specific and there are other instances. I'll file one/do a commit
that just adds the bug ref to the various tests that do this. (i.e. separate
patch.)

https://codereview.chromium.org/23653024/diff/1/Source/modules/indexeddb/IDBC...
File Source/modules/indexeddb/IDBCursor.h (right):

https://codereview.chromium.org/23653024/diff/1/Source/modules/indexeddb/IDBC...
Source/modules/indexeddb/IDBCursor.h:85: void deref()
On 2013/09/06 17:56:35, alecflett wrote:
> can/should be OVERRIDE?

Nope, not here. T::deref() is required by the RefPtr<T> family of templates but
not provided by WTF::RefCountedBase.

It's virtual/OVERRIDE in IDBRequest because it's declared as virtual deref() = 0
in IDBCallbacks since we pass RefPtr<IDBCallbacks> around.

(Sorting that mess out is why I let this approach linger for so long!)

https://codereview.chromium.org/23653024/diff/1/Source/modules/indexeddb/IDBR...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/23653024/diff/1/Source/modules/indexeddb/IDBR...
Source/modules/indexeddb/IDBRequest.cpp:220: void
IDBRequest::checkForReferenceCycle()
On 2013/09/06 17:56:35, alecflett wrote:
> It would be neat if there were some primitive around this, i.e.
> 
> WTF::RefCountedCycleOwned<T, N>
> 

Yeah. But in the short term this is the only instance I've seen (there are
almost no other types that need to derive from WTF::RefCountedBase) and in the
long term we're betting on Oilpan.

[commit-bot: I haz the power, 2013-09-06 18:08:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/23653024/1

[commit-bot: I haz the power, 2013-09-06 19:25:18]

Change committed as 157382