Improve CleanupReference & add test

- switch CleanupReference to have a reaper thread rather than sacrificial object, as the
old design was causing many spurious messages to be sent to the UI thread.
- CleanupReference now triggers when GC sees the object has become weakly rather than
phantom reachable, as this can occur earlier but if sufficient for knowing we must
perform native cleanup.
- added some basic GC cleanup tests in CleanupReferenceTest.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190110

[David Trainor- moved to gerrit, 2013-03-18 17:44:39]

lgtm

https://chromiumcodereview.appspot.com/12658010/diff/13001/content/public/and...
File
content/public/android/java/src/org/chromium/content/common/CleanupReference.java
(right):

https://chromiumcodereview.appspot.com/12658010/diff/13001/content/public/and...
content/public/android/java/src/org/chromium/content/common/CleanupReference.java:31:
static private final String TAG = "CleanupReference";
Can we just use CleanupReference.class.getName()?

https://chromiumcodereview.appspot.com/12658010/diff/13001/content/public/and...
content/public/android/java/src/org/chromium/content/common/CleanupReference.java:33:
static private final boolean DEBUG =true;// false;  // Always check in as false!
should be false?

[joth, 2013-03-18 20:40:56]

thanks. I realized I could make this less brittle by counting native AwContents
instances rather than CleanupReference.sRefs size, so switched to that approach
now.

https://chromiumcodereview.appspot.com/12658010/diff/13001/content/public/and...
File
content/public/android/java/src/org/chromium/content/common/CleanupReference.java
(right):

https://chromiumcodereview.appspot.com/12658010/diff/13001/content/public/and...
content/public/android/java/src/org/chromium/content/common/CleanupReference.java:31:
static private final String TAG = "CleanupReference";
On 2013/03/18 17:44:39, David Trainor wrote:
> Can we just use CleanupReference.class.getName()?

digit argues against this on
https://code.google.com/p/chromium/issues/detail?id=146559 - probably best to
make a decision on that bug.

https://chromiumcodereview.appspot.com/12658010/diff/13001/content/public/and...
content/public/android/java/src/org/chromium/content/common/CleanupReference.java:33:
static private final boolean DEBUG =true;// false;  // Always check in as false!
On 2013/03/18 17:44:39, David Trainor wrote:
> should be false?
Done
ah, thanks! I was trying to debug why this was failing on the bots, but worked
out now it's due to LKGR had not yet rolled past my leak fix
(https://codereview.chromium.org/12666007/) -- it now has now, so should all
pass now.

[joth, 2013-03-20 02:12:51]

testing via AwContents is unreliable at the moment for other reasons, so I added
dedicated unit test for CleanupReference.

PTAL?

[Kristian Monsen, 2013-03-20 03:59:02]

https://codereview.chromium.org/12658010/diff/25003/content/public/android/ja...
File
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java
(right):

https://codereview.chromium.org/12658010/diff/25003/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java:54:
System.gc();
Is this sure to work in the future? I think System.gc() is just a hint, and even
if a gc runs it might not want to collect this object. I wonder if it would work
with a non stop-the-world gc?

[joth, 2013-03-20 21:00:39]

https://codereview.chromium.org/12658010/diff/25003/content/public/android/ja...
File
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java
(right):

https://codereview.chromium.org/12658010/diff/25003/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java:54:
System.gc();
On 2013/03/20 03:59:02, Kristian Monsen wrote:
> Is this sure to work in the future? I think System.gc() is just a hint, and
even
> if a gc runs it might not want to collect this object. I wonder if it would
work
> with a non stop-the-world gc?

Broadly, if it stops working in future this is probably something we'd want to
know about. The test failing will help us learn this.

Specifically about System.gc(), agree it's almost always wrong to depend on it,
but testing finalizer/ReferenceQueues this is one of the few times you basically
have to. (Example in
http://google-collections.googlecode.com/svn-history/r77/trunk/test/com/googl...
)

We poll for 3s waiting for the GC to take effect. Hopefully it always will.
If it did ignore this 'hint' in future, we could add a "new byte[10 billion]"
call too, as the VM must attempt GC before throwing OOM exception.

I'll add a comment.

[Kristian Monsen, 2013-03-20 21:10:11]

On 2013/03/20 21:00:39, joth wrote:
>
https://codereview.chromium.org/12658010/diff/25003/content/public/android/ja...
> File
>
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java
> (right):
> 
>
https://codereview.chromium.org/12658010/diff/25003/content/public/android/ja...
>
content/public/android/javatests/src/org/chromium/content/common/CleanupReferenceTest.java:54:
> System.gc();
> On 2013/03/20 03:59:02, Kristian Monsen wrote:
> > Is this sure to work in the future? I think System.gc() is just a hint, and
> even
> > if a gc runs it might not want to collect this object. I wonder if it would
> work
> > with a non stop-the-world gc?
> 
> Broadly, if it stops working in future this is probably something we'd want to
> know about. The test failing will help us learn this.
> 
> Specifically about System.gc(), agree it's almost always wrong to depend on
it,
> but testing finalizer/ReferenceQueues this is one of the few times you
basically
> have to. (Example in
>
http://google-collections.googlecode.com/svn-history/r77/trunk/test/com/googl...
> )
> 
> We poll for 3s waiting for the GC to take effect. Hopefully it always will.
> If it did ignore this 'hint' in future, we could add a "new byte[10 billion]"
> call too, as the VM must attempt GC before throwing OOM exception.
> 
> I'll add a comment.

Reasonable, lgtm.

[commit-bot: I haz the power, 2013-03-22 04:15:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/12658010/39001

[commit-bot: I haz the power, 2013-03-22 18:28:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/12658010/39001

[commit-bot: I haz the power, 2013-03-23 15:13:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/12658010/39001

[commit-bot: I haz the power, 2013-03-23 23:19:20]

Change committed as 190110