Make native AwContents hold a reference to the WindowAndroidWrapper

Ensure that native WindowAndroid outlives native AwContents.

ContentViewCore holds a window pointer that is used during
destruction of native AwContents to remove observers. However the
current CleanupReference based finalization scheme does not enforce
an ordering on the destruction of native WindowAndroid and AwContents
instances. Satisfaction of the constraint that AwContents is destroyed
before WindowAndroid is therefore dependent on the CleanupReference
implementation, and possibly the implementation of the JVM as well.

Making the AwContents DestroyRunnable strongly reference the
associated WindowAndroidWrapper enforces the correct ordering.

BUG=595336
Committed: https://crrev.com/81a813fcee09c1ed0f299aba3b589e3705238875
Cr-Commit-Position: refs/heads/master@{#381675}

[Tobias Sargeant, 2016-03-16 15:33:10]

The CQ bit was checked by tobiasjs@chromium.org to run a CQ dry run

[Tobias Sargeant, 2016-03-16 15:33:29]

tobiasjs@chromium.org changed reviewers:
+ boliu@chromium.org, torne@chromium.org

[commit-bot: I haz the power, 2016-03-16 15:33:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1809643002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1809643002/1

[boliu, 2016-03-16 15:45:37]

What if you kept the reference in DestroyRunnable instead?

[Tobias Sargeant, 2016-03-16 15:49:16]

On 2016/03/16 15:45:37, boliu wrote:
> What if you kept the reference in DestroyRunnable instead?

Torne and I discussed that possibility, but we concluded that it doesn't help
because the problem is that the entire graph of objects (including the
RestroyRunnables) becomes collectible at once, and once that happens, there's no
sequencing guarantee between the callbacks of any CleanupReference instances
that reach into that graph.

[boliu, 2016-03-16 15:54:03]

On 2016/03/16 15:49:16, Tobias Sargeant wrote:
> On 2016/03/16 15:45:37, boliu wrote:
> > What if you kept the reference in DestroyRunnable instead?
> 
> Torne and I discussed that possibility, but we concluded that it doesn't help
> because the problem is that the entire graph of objects (including the
> RestroyRunnables) becomes collectible at once,

Not true?

AwContents becomes eligible for gc, at which point the CleanupReference to
AwContents is enqueued into the reference queue. Then we go through the queue
and give DestroyRunnable a chance to run. Then CleanupReference is eligible for
gc.

I mean DestroyRunnable is responsible for nativeDestroy, how it just go away
together with java AwContents?

> and once that happens, there's no
> sequencing guarantee between the callbacks of any CleanupReference instances
> that reach into that graph.

[commit-bot: I haz the power, 2016-03-16 16:08:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-16 16:08:25]

Dry run: This issue passed the CQ dry run.

[Torne, 2016-03-16 16:46:03]

On 2016/03/16 15:54:03, boliu wrote:
> On 2016/03/16 15:49:16, Tobias Sargeant wrote:
> > On 2016/03/16 15:45:37, boliu wrote:
> > > What if you kept the reference in DestroyRunnable instead?
> > 
> > Torne and I discussed that possibility, but we concluded that it doesn't
help
> > because the problem is that the entire graph of objects (including the
> > RestroyRunnables) becomes collectible at once,
> 
> Not true?
> 
> AwContents becomes eligible for gc, at which point the CleanupReference to
> AwContents is enqueued into the reference queue. Then we go through the queue
> and give DestroyRunnable a chance to run. Then CleanupReference is eligible
for
> gc.
> 
> I mean DestroyRunnable is responsible for nativeDestroy, how it just go away
> together with java AwContents?

We discussed a lot of things and I think we got mixed up somewhere here. Part of
the problem is that there's two DestroyRunnables here ;)

If the CleanupReference for AwContents had a strong reference path to the
WindowAndroidWrapper (by adding it to AwContents.DestroyRunnable) then yes, that
would also fix it; all the CleanupReference objects are strongly referenced
until after their runnable has run, so this would keep WindowAndroidWrapper
alive until after AwContents.destroy() has been called and the native object
disposed of.

I don't think I like it, though. The main AwContents cleanup reference currently
is pretty sane and reasonable: it hangs onto a copy of the native object pointer
so that if we dispose of AwContents without calling destroy, it can dispose of
the native object to match. This is pretty much exactly what phantom references
are for (to do that without relying on a finalizer). Just stuffing an
WindowAndroidWrapper reference in there to keep the object alive, which the
Runnable itself won't do anything with at all, looks really weird; anything you
have to put a comment on to explain why it even exists is a bit suspect ;)

Actually adding the JNI reference here makes explicit something that was
previously only implied: that the native AwContents depends on the continued
existence of the Java WindowAndroid. We've explicitly tied the native object
lifetimes to the Java object lifetimes instead of having the native objects own
each other directly (e.g. by refcounting), and so the lifetime relationship here
does seem right to me. You can then regard the native AwContents's bare pointer
to the native WindowAndroid to be a cache - it's just hanging onto a copy to
avoid having to actually call back into Java to retrieve it.

We could probably go either way with this, though; it's not like either way is
actually pleasant code. :/

---

Orthogonally to this change, the WindowAndroidWrapper concept is also really
horrible, and means that the lifetimes are harder to explain than they really
should be, because it's whether there are references to the *wrapper* that
matters, and whether there are references to the WindowAndroid is irrelevant. I
assume it's like this because WindowAndroid is shared with chrome in general,
who don't care about cleaning up in cases where destroy() isn't called, but it
would still be much clearer if WindowAndroid just took care of its own cleanup
internally and we didn't have to introduce the extra wrapper. That wouldn't
solve the problem in this CL (we'd still have to do one of the same fixes, just
by adding a reference to the WindowAndroid directly instead of the wrapper), but
it would also help improve comprehensibility here..

> > and once that happens, there's no
> > sequencing guarantee between the callbacks of any CleanupReference instances
> > that reach into that graph.

[Tobias Sargeant, 2016-03-16 17:24:01]

I think given that neither approach can guarantee that holding on to a
WindowAndroid instance is enough to ensure it lives, and PS2 is smaller and
doesn't involve JNI, we should probably prefer it.

If strongly referencing WindowAndroid was enough to keep it alive, then I
believe it would be better to do that in native code, so I'll make this a TODO.

[Tobias Sargeant, 2016-03-16 17:25:35]

Description was changed from

==========
Make native AwContents hold a reference to the WindowAndroidWrapper

ContentViewCore holds a window pointer that is used during
destruction of native AwContents to remove observers. However the
current CleanupReference based finalization scheme does not enforce
an ordering on the destruction of native WindowAndroid and AwContents
instances. Satisfaction of the constraint that AwContents is destroyed
before WindowAndroid is therefore dependent on the CleanupReference
implementation, and possibly the implementation of the JVM as well.

Holding on to a java global reference to the WindowAndroidWrapper from
native AwContents ensures that the WindowAndroid is only destroyed after
all referencing AwContents instances have been destroyed.

BUG=595336
==========

to

==========
Ensure that native WindowAndroid outlives native AwContents.

ContentViewCore holds a window pointer that is used during
destruction of native AwContents to remove observers. However the
current CleanupReference based finalization scheme does not enforce
an ordering on the destruction of native WindowAndroid and AwContents
instances. Satisfaction of the constraint that AwContents is destroyed
before WindowAndroid is therefore dependent on the CleanupReference
implementation, and possibly the implementation of the JVM as well.

Making the AwContents DestroyRunnable strongly reference the
WindowAndroidWrapper enforces the correct ordering.

BUG=595336
==========

[Tobias Sargeant, 2016-03-16 17:25:59]

Description was changed from

==========
Ensure that native WindowAndroid outlives native AwContents.

ContentViewCore holds a window pointer that is used during
destruction of native AwContents to remove observers. However the
current CleanupReference based finalization scheme does not enforce
an ordering on the destruction of native WindowAndroid and AwContents
instances. Satisfaction of the constraint that AwContents is destroyed
before WindowAndroid is therefore dependent on the CleanupReference
implementation, and possibly the implementation of the JVM as well.

Making the AwContents DestroyRunnable strongly reference the
WindowAndroidWrapper enforces the correct ordering.

BUG=595336
==========

to

==========
Ensure that native WindowAndroid outlives native AwContents.

ContentViewCore holds a window pointer that is used during
destruction of native AwContents to remove observers. However the
current CleanupReference based finalization scheme does not enforce
an ordering on the destruction of native WindowAndroid and AwContents
instances. Satisfaction of the constraint that AwContents is destroyed
before WindowAndroid is therefore dependent on the CleanupReference
implementation, and possibly the implementation of the JVM as well.

Making the AwContents DestroyRunnable strongly reference the
associated WindowAndroidWrapper enforces the correct ordering.

BUG=595336
==========

[boliu, 2016-03-16 17:26:21]

On 2016/03/16 16:46:03, Torne wrote:
> On 2016/03/16 15:54:03, boliu wrote:
> > On 2016/03/16 15:49:16, Tobias Sargeant wrote:
> > > On 2016/03/16 15:45:37, boliu wrote:
> > > > What if you kept the reference in DestroyRunnable instead?
> > > 
> > > Torne and I discussed that possibility, but we concluded that it doesn't
> help
> > > because the problem is that the entire graph of objects (including the
> > > RestroyRunnables) becomes collectible at once,
> > 
> > Not true?
> > 
> > AwContents becomes eligible for gc, at which point the CleanupReference to
> > AwContents is enqueued into the reference queue. Then we go through the
queue
> > and give DestroyRunnable a chance to run. Then CleanupReference is eligible
> for
> > gc.
> > 
> > I mean DestroyRunnable is responsible for nativeDestroy, how it just go away
> > together with java AwContents?
> 
> We discussed a lot of things and I think we got mixed up somewhere here. Part
of
> the problem is that there's two DestroyRunnables here ;)
> 
> If the CleanupReference for AwContents had a strong reference path to the
> WindowAndroidWrapper (by adding it to AwContents.DestroyRunnable) then yes,
that
> would also fix it; all the CleanupReference objects are strongly referenced
> until after their runnable has run, so this would keep WindowAndroidWrapper
> alive until after AwContents.destroy() has been called and the native object
> disposed of.
> 
> I don't think I like it, though. The main AwContents cleanup reference
currently
> is pretty sane and reasonable: it hangs onto a copy of the native object
pointer
> so that if we dispose of AwContents without calling destroy, it can dispose of
> the native object to match. This is pretty much exactly what phantom
references
> are for (to do that without relying on a finalizer). Just stuffing an
> WindowAndroidWrapper reference in there to keep the object alive, which the
> Runnable itself won't do anything with at all, looks really weird; anything
you
> have to put a comment on to explain why it even exists is a bit suspect ;)
> 
> Actually adding the JNI reference here makes explicit something that was
> previously only implied: that the native AwContents depends on the continued
> existence of the Java WindowAndroid.

I think holding onto Java resources in AwContents.DestroyRunnable expresses the
same idea, but that's just me. I just generally like to avoid jni if possible.

> We've explicitly tied the native object
> lifetimes to the Java object lifetimes instead of having the native objects
own
> each other directly (e.g. by refcounting), and so the lifetime relationship
here
> does seem right to me. You can then regard the native AwContents's bare
pointer
> to the native WindowAndroid to be a cache - it's just hanging onto a copy to
> avoid having to actually call back into Java to retrieve it.

This is just aesthetics.. and you really can argue it both ways:
You can think of java AwContents owning native AwContents, and part of the
promise is WindowAndroid has to be alive throughout. Then putting it into
DestroyRunnable makes perfect sense. Incidentally this is also roughly what
chrome does: native WindowAndroid is passed around as a bare pointer, with
something at a higher level guaranteeing WindowAndroid outlives everything else.

> 
> We could probably go either way with this, though; it's not like either way is
> actually pleasant code. :/

Either way, this needs a comment :)

> 
> ---
> 
> Orthogonally to this change, the WindowAndroidWrapper concept is also really
> horrible, and means that the lifetimes are harder to explain than they really
> should be, because it's whether there are references to the *wrapper* that
> matters, and whether there are references to the WindowAndroid is irrelevant.
I
> assume it's like this because WindowAndroid is shared with chrome in general,
> who don't care about cleaning up in cases where destroy() isn't called, but it
> would still be much clearer if WindowAndroid just took care of its own cleanup
> internally and we didn't have to introduce the extra wrapper. That wouldn't
> solve the problem in this CL (we'd still have to do one of the same fixes,
just
> by adding a reference to the WindowAndroid directly instead of the wrapper),
but
> it would also help improve comprehensibility here..

Well, before I added WindowAndroidWrapper, we were leaking native WindowAndroid
:p

I'm against making WindowAndroid smart, because we have a bug to remove usage of
CleanupReference everywhere else, then move CleanupReference into
android_webview: crbug.com/507405 I think that's a worthy goal.

> 
> > > and once that happens, there's no
> > > sequencing guarantee between the callbacks of any CleanupReference
instances
> > > that reach into that graph.

[Torne, 2016-03-16 17:29:51]

On 2016/03/16 17:26:21, boliu wrote:
> On 2016/03/16 16:46:03, Torne wrote:
> > On 2016/03/16 15:54:03, boliu wrote:
> > > On 2016/03/16 15:49:16, Tobias Sargeant wrote:
> > > > On 2016/03/16 15:45:37, boliu wrote:
> > > > > What if you kept the reference in DestroyRunnable instead?
> > > > 
> > > > Torne and I discussed that possibility, but we concluded that it doesn't
> > help
> > > > because the problem is that the entire graph of objects (including the
> > > > RestroyRunnables) becomes collectible at once,
> > > 
> > > Not true?
> > > 
> > > AwContents becomes eligible for gc, at which point the CleanupReference to
> > > AwContents is enqueued into the reference queue. Then we go through the
> queue
> > > and give DestroyRunnable a chance to run. Then CleanupReference is
eligible
> > for
> > > gc.
> > > 
> > > I mean DestroyRunnable is responsible for nativeDestroy, how it just go
away
> > > together with java AwContents?
> > 
> > We discussed a lot of things and I think we got mixed up somewhere here.
Part
> of
> > the problem is that there's two DestroyRunnables here ;)
> > 
> > If the CleanupReference for AwContents had a strong reference path to the
> > WindowAndroidWrapper (by adding it to AwContents.DestroyRunnable) then yes,
> that
> > would also fix it; all the CleanupReference objects are strongly referenced
> > until after their runnable has run, so this would keep WindowAndroidWrapper
> > alive until after AwContents.destroy() has been called and the native object
> > disposed of.
> > 
> > I don't think I like it, though. The main AwContents cleanup reference
> currently
> > is pretty sane and reasonable: it hangs onto a copy of the native object
> pointer
> > so that if we dispose of AwContents without calling destroy, it can dispose
of
> > the native object to match. This is pretty much exactly what phantom
> references
> > are for (to do that without relying on a finalizer). Just stuffing an
> > WindowAndroidWrapper reference in there to keep the object alive, which the
> > Runnable itself won't do anything with at all, looks really weird; anything
> you
> > have to put a comment on to explain why it even exists is a bit suspect ;)
> > 
> > Actually adding the JNI reference here makes explicit something that was
> > previously only implied: that the native AwContents depends on the continued
> > existence of the Java WindowAndroid.
> 
> I think holding onto Java resources in AwContents.DestroyRunnable expresses
the
> same idea, but that's just me. I just generally like to avoid jni if possible.
> 
> > We've explicitly tied the native object
> > lifetimes to the Java object lifetimes instead of having the native objects
> own
> > each other directly (e.g. by refcounting), and so the lifetime relationship
> here
> > does seem right to me. You can then regard the native AwContents's bare
> pointer
> > to the native WindowAndroid to be a cache - it's just hanging onto a copy to
> > avoid having to actually call back into Java to retrieve it.
> 
> This is just aesthetics.. and you really can argue it both ways:
> You can think of java AwContents owning native AwContents, and part of the
> promise is WindowAndroid has to be alive throughout. Then putting it into
> DestroyRunnable makes perfect sense. Incidentally this is also roughly what
> chrome does: native WindowAndroid is passed around as a bare pointer, with
> something at a higher level guaranteeing WindowAndroid outlives everything
else.
> 
> > 
> > We could probably go either way with this, though; it's not like either way
is
> > actually pleasant code. :/
> 
> Either way, this needs a comment :)
> 
> > 
> > ---
> > 
> > Orthogonally to this change, the WindowAndroidWrapper concept is also really
> > horrible, and means that the lifetimes are harder to explain than they
really
> > should be, because it's whether there are references to the *wrapper* that
> > matters, and whether there are references to the WindowAndroid is
irrelevant.
> I
> > assume it's like this because WindowAndroid is shared with chrome in
general,
> > who don't care about cleaning up in cases where destroy() isn't called, but
it
> > would still be much clearer if WindowAndroid just took care of its own
cleanup
> > internally and we didn't have to introduce the extra wrapper. That wouldn't
> > solve the problem in this CL (we'd still have to do one of the same fixes,
> just
> > by adding a reference to the WindowAndroid directly instead of the wrapper),
> but
> > it would also help improve comprehensibility here..
> 
> Well, before I added WindowAndroidWrapper, we were leaking native
WindowAndroid
> :p

Right, but I think the right fix would have been to add it to WindowAndroid.

> I'm against making WindowAndroid smart, because we have a bug to remove usage
of
> CleanupReference everywhere else, then move CleanupReference into
> android_webview: crbug.com/507405 I think that's a worthy goal.

I'm not sure we should be doing this. Wrapping objects in this way makes the
lifetime *really* weird and the guarantees *really* bad - you have to make sure
you are hanging on to the wrapper everywhere and not the real object. I think we
should continue to use CleanupReference for any object that WebView needs to
have finalizer-equivalent semantics.

> > 
> > > > and once that happens, there's no
> > > > sequencing guarantee between the callbacks of any CleanupReference
> instances
> > > > that reach into that graph.

[boliu, 2016-03-16 17:38:28]

On 2016/03/16 17:29:51, Torne wrote:
> On 2016/03/16 17:26:21, boliu wrote:
> > On 2016/03/16 16:46:03, Torne wrote:
> > > On 2016/03/16 15:54:03, boliu wrote:
> > > > On 2016/03/16 15:49:16, Tobias Sargeant wrote:
> > > > > On 2016/03/16 15:45:37, boliu wrote:
> > > > > > What if you kept the reference in DestroyRunnable instead?
> > > > > 
> > > > > Torne and I discussed that possibility, but we concluded that it
doesn't
> > > help
> > > > > because the problem is that the entire graph of objects (including the
> > > > > RestroyRunnables) becomes collectible at once,
> > > > 
> > > > Not true?
> > > > 
> > > > AwContents becomes eligible for gc, at which point the CleanupReference
to
> > > > AwContents is enqueued into the reference queue. Then we go through the
> > queue
> > > > and give DestroyRunnable a chance to run. Then CleanupReference is
> eligible
> > > for
> > > > gc.
> > > > 
> > > > I mean DestroyRunnable is responsible for nativeDestroy, how it just go
> away
> > > > together with java AwContents?
> > > 
> > > We discussed a lot of things and I think we got mixed up somewhere here.
> Part
> > of
> > > the problem is that there's two DestroyRunnables here ;)
> > > 
> > > If the CleanupReference for AwContents had a strong reference path to the
> > > WindowAndroidWrapper (by adding it to AwContents.DestroyRunnable) then
yes,
> > that
> > > would also fix it; all the CleanupReference objects are strongly
referenced
> > > until after their runnable has run, so this would keep
WindowAndroidWrapper
> > > alive until after AwContents.destroy() has been called and the native
object
> > > disposed of.
> > > 
> > > I don't think I like it, though. The main AwContents cleanup reference
> > currently
> > > is pretty sane and reasonable: it hangs onto a copy of the native object
> > pointer
> > > so that if we dispose of AwContents without calling destroy, it can
dispose
> of
> > > the native object to match. This is pretty much exactly what phantom
> > references
> > > are for (to do that without relying on a finalizer). Just stuffing an
> > > WindowAndroidWrapper reference in there to keep the object alive, which
the
> > > Runnable itself won't do anything with at all, looks really weird;
anything
> > you
> > > have to put a comment on to explain why it even exists is a bit suspect ;)
> > > 
> > > Actually adding the JNI reference here makes explicit something that was
> > > previously only implied: that the native AwContents depends on the
continued
> > > existence of the Java WindowAndroid.
> > 
> > I think holding onto Java resources in AwContents.DestroyRunnable expresses
> the
> > same idea, but that's just me. I just generally like to avoid jni if
possible.
> > 
> > > We've explicitly tied the native object
> > > lifetimes to the Java object lifetimes instead of having the native
objects
> > own
> > > each other directly (e.g. by refcounting), and so the lifetime
relationship
> > here
> > > does seem right to me. You can then regard the native AwContents's bare
> > pointer
> > > to the native WindowAndroid to be a cache - it's just hanging onto a copy
to
> > > avoid having to actually call back into Java to retrieve it.
> > 
> > This is just aesthetics.. and you really can argue it both ways:
> > You can think of java AwContents owning native AwContents, and part of the
> > promise is WindowAndroid has to be alive throughout. Then putting it into
> > DestroyRunnable makes perfect sense. Incidentally this is also roughly what
> > chrome does: native WindowAndroid is passed around as a bare pointer, with
> > something at a higher level guaranteeing WindowAndroid outlives everything
> else.
> > 
> > > 
> > > We could probably go either way with this, though; it's not like either
way
> is
> > > actually pleasant code. :/
> > 
> > Either way, this needs a comment :)
> > 
> > > 
> > > ---
> > > 
> > > Orthogonally to this change, the WindowAndroidWrapper concept is also
really
> > > horrible, and means that the lifetimes are harder to explain than they
> really
> > > should be, because it's whether there are references to the *wrapper* that
> > > matters, and whether there are references to the WindowAndroid is
> irrelevant.
> > I
> > > assume it's like this because WindowAndroid is shared with chrome in
> general,
> > > who don't care about cleaning up in cases where destroy() isn't called,
but
> it
> > > would still be much clearer if WindowAndroid just took care of its own
> cleanup
> > > internally and we didn't have to introduce the extra wrapper. That
wouldn't
> > > solve the problem in this CL (we'd still have to do one of the same fixes,
> > just
> > > by adding a reference to the WindowAndroid directly instead of the
wrapper),
> > but
> > > it would also help improve comprehensibility here..
> > 
> > Well, before I added WindowAndroidWrapper, we were leaking native
> WindowAndroid
> > :p
> 
> Right, but I think the right fix would have been to add it to WindowAndroid.
> 
> > I'm against making WindowAndroid smart, because we have a bug to remove
usage
> of
> > CleanupReference everywhere else, then move CleanupReference into
> > android_webview: crbug.com/507405 I think that's a worthy goal.
> 
> I'm not sure we should be doing this. Wrapping objects in this way makes the
> lifetime *really* weird and the guarantees *really* bad - you have to make
sure
> you are hanging on to the wrapper everywhere and not the real object. I think
we
> should continue to use CleanupReference for any object that WebView needs to
> have finalizer-equivalent semantics.

Immediate problem is CleanupReference lives in content and is tied to the UI
thread, but WindowAndroid is in a lower layer where there is no UI thread
concept.

Not sure how flexible CleanupReference could be made in general..

> 
> > > 
> > > > > and once that happens, there's no
> > > > > sequencing guarantee between the callbacks of any CleanupReference
> > instances
> > > > > that reach into that graph.

[boliu, 2016-03-16 19:01:15]

On 2016/03/16 17:38:28, boliu wrote:
> On 2016/03/16 17:29:51, Torne wrote:
> > On 2016/03/16 17:26:21, boliu wrote:
> > > On 2016/03/16 16:46:03, Torne wrote:
> > > > On 2016/03/16 15:54:03, boliu wrote:
> > > > > On 2016/03/16 15:49:16, Tobias Sargeant wrote:
> > > > > > On 2016/03/16 15:45:37, boliu wrote:
> > > > > > > What if you kept the reference in DestroyRunnable instead?
> > > > > > 
> > > > > > Torne and I discussed that possibility, but we concluded that it
> doesn't
> > > > help
> > > > > > because the problem is that the entire graph of objects (including
the
> > > > > > RestroyRunnables) becomes collectible at once,
> > > > > 
> > > > > Not true?
> > > > > 
> > > > > AwContents becomes eligible for gc, at which point the
CleanupReference
> to
> > > > > AwContents is enqueued into the reference queue. Then we go through
the
> > > queue
> > > > > and give DestroyRunnable a chance to run. Then CleanupReference is
> > eligible
> > > > for
> > > > > gc.
> > > > > 
> > > > > I mean DestroyRunnable is responsible for nativeDestroy, how it just
go
> > away
> > > > > together with java AwContents?
> > > > 
> > > > We discussed a lot of things and I think we got mixed up somewhere here.
> > Part
> > > of
> > > > the problem is that there's two DestroyRunnables here ;)
> > > > 
> > > > If the CleanupReference for AwContents had a strong reference path to
the
> > > > WindowAndroidWrapper (by adding it to AwContents.DestroyRunnable) then
> yes,
> > > that
> > > > would also fix it; all the CleanupReference objects are strongly
> referenced
> > > > until after their runnable has run, so this would keep
> WindowAndroidWrapper
> > > > alive until after AwContents.destroy() has been called and the native
> object
> > > > disposed of.
> > > > 
> > > > I don't think I like it, though. The main AwContents cleanup reference
> > > currently
> > > > is pretty sane and reasonable: it hangs onto a copy of the native object
> > > pointer
> > > > so that if we dispose of AwContents without calling destroy, it can
> dispose
> > of
> > > > the native object to match. This is pretty much exactly what phantom
> > > references
> > > > are for (to do that without relying on a finalizer). Just stuffing an
> > > > WindowAndroidWrapper reference in there to keep the object alive, which
> the
> > > > Runnable itself won't do anything with at all, looks really weird;
> anything
> > > you
> > > > have to put a comment on to explain why it even exists is a bit suspect
;)
> > > > 
> > > > Actually adding the JNI reference here makes explicit something that was
> > > > previously only implied: that the native AwContents depends on the
> continued
> > > > existence of the Java WindowAndroid.
> > > 
> > > I think holding onto Java resources in AwContents.DestroyRunnable
expresses
> > the
> > > same idea, but that's just me. I just generally like to avoid jni if
> possible.
> > > 
> > > > We've explicitly tied the native object
> > > > lifetimes to the Java object lifetimes instead of having the native
> objects
> > > own
> > > > each other directly (e.g. by refcounting), and so the lifetime
> relationship
> > > here
> > > > does seem right to me. You can then regard the native AwContents's bare
> > > pointer
> > > > to the native WindowAndroid to be a cache - it's just hanging onto a
copy
> to
> > > > avoid having to actually call back into Java to retrieve it.
> > > 
> > > This is just aesthetics.. and you really can argue it both ways:
> > > You can think of java AwContents owning native AwContents, and part of the
> > > promise is WindowAndroid has to be alive throughout. Then putting it into
> > > DestroyRunnable makes perfect sense. Incidentally this is also roughly
what
> > > chrome does: native WindowAndroid is passed around as a bare pointer, with
> > > something at a higher level guaranteeing WindowAndroid outlives everything
> > else.
> > > 
> > > > 
> > > > We could probably go either way with this, though; it's not like either
> way
> > is
> > > > actually pleasant code. :/
> > > 
> > > Either way, this needs a comment :)
> > > 
> > > > 
> > > > ---
> > > > 
> > > > Orthogonally to this change, the WindowAndroidWrapper concept is also
> really
> > > > horrible, and means that the lifetimes are harder to explain than they
> > really
> > > > should be, because it's whether there are references to the *wrapper*
that
> > > > matters, and whether there are references to the WindowAndroid is
> > irrelevant.
> > > I
> > > > assume it's like this because WindowAndroid is shared with chrome in
> > general,
> > > > who don't care about cleaning up in cases where destroy() isn't called,
> but
> > it
> > > > would still be much clearer if WindowAndroid just took care of its own
> > cleanup
> > > > internally and we didn't have to introduce the extra wrapper. That
> wouldn't
> > > > solve the problem in this CL (we'd still have to do one of the same
fixes,
> > > just
> > > > by adding a reference to the WindowAndroid directly instead of the
> wrapper),
> > > but
> > > > it would also help improve comprehensibility here..
> > > 
> > > Well, before I added WindowAndroidWrapper, we were leaking native
> > WindowAndroid
> > > :p
> > 
> > Right, but I think the right fix would have been to add it to WindowAndroid.
> > 
> > > I'm against making WindowAndroid smart, because we have a bug to remove
> usage
> > of
> > > CleanupReference everywhere else, then move CleanupReference into
> > > android_webview: crbug.com/507405 I think that's a worthy goal.
> > 
> > I'm not sure we should be doing this. Wrapping objects in this way makes the
> > lifetime *really* weird and the guarantees *really* bad - you have to make
> sure
> > you are hanging on to the wrapper everywhere and not the real object. I
think
> we
> > should continue to use CleanupReference for any object that WebView needs to
> > have finalizer-equivalent semantics.
> 
> Immediate problem is CleanupReference lives in content and is tied to the UI
> thread, but WindowAndroid is in a lower layer where there is no UI thread
> concept.
> 
> Not sure how flexible CleanupReference could be made in general..

Hmm, in theory, CleanupReference could be made thread agnostic. Need to make the
Handler and the HashSet thread local. Still only need one reaper thread. Then
CleanupReference can be moved to base. And constructing a CleanupReference, have
to specify a Looper as well to run the callback. All kind of complex though.

So then the debate becomes kinda philosophical again, whether an explicit
destroy or using CleanupReference is better. Chrome has overwhelmingly chose
explicit destroy, because android app APIs support that mode (although I have
found places where finalizer is not used correctly in chrome, usually not
realizing finalizer runs on some arbitrary thread)

I lean towards the second: only use CleanupReference in webview because webview
api needs support for both.


fwiw, would we have had this debate if I called it WindowAndroid*Reference*
instead..?

> 
> > 
> > > > 
> > > > > > and once that happens, there's no
> > > > > > sequencing guarantee between the callbacks of any CleanupReference
> > > instances
> > > > > > that reach into that graph.

[boliu, 2016-03-16 20:53:08]

Oh check it out. This affects M too:
https://build.chromium.org/p/chromium.android/builders/Marshmallow%2064%20bit...
first failure of testCreateAndGcManyTimes contains my PhantomRef change.

lgtm

https://codereview.chromium.org/1809643002/diff/40001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/1809643002/diff/40001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwContents.java:321: //
TODO(tobiasjs): It would be better if holding on to a WindowAndroid reference
was
I'd remove this TODO since it's not really settled..

[boliu, 2016-03-16 20:57:16]

On 2016/03/16 20:53:08, boliu wrote:
> Oh check it out. This affects M too:
>
https://build.chromium.org/p/chromium.android/builders/Marshmallow%2064%20bit...
> first failure of testCreateAndGcManyTimes contains my PhantomRef change.

And L
https://build.chromium.org/p/chromium.android/builders/Lollipop%20Low-end%20T...

> 
> lgtm
> 
>
https://codereview.chromium.org/1809643002/diff/40001/android_webview/java/sr...
> File android_webview/java/src/org/chromium/android_webview/AwContents.java
> (right):
> 
>
https://codereview.chromium.org/1809643002/diff/40001/android_webview/java/sr...
> android_webview/java/src/org/chromium/android_webview/AwContents.java:321: //
> TODO(tobiasjs): It would be better if holding on to a WindowAndroid reference
> was
> I'd remove this TODO since it's not really settled..

[Tobias Sargeant, 2016-03-17 09:13:23]

Patchset #3 (id:40001) has been deleted

[Tobias Sargeant, 2016-03-17 09:13:44]

The CQ bit was checked by tobiasjs@chromium.org

[commit-bot: I haz the power, 2016-03-17 09:13:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1809643002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1809643002/20001

[commit-bot: I haz the power, 2016-03-17 09:43:48]

Description was changed from

==========
Ensure that native WindowAndroid outlives native AwContents.

ContentViewCore holds a window pointer that is used during
destruction of native AwContents to remove observers. However the
current CleanupReference based finalization scheme does not enforce
an ordering on the destruction of native WindowAndroid and AwContents
instances. Satisfaction of the constraint that AwContents is destroyed
before WindowAndroid is therefore dependent on the CleanupReference
implementation, and possibly the implementation of the JVM as well.

Making the AwContents DestroyRunnable strongly reference the
associated WindowAndroidWrapper enforces the correct ordering.

BUG=595336
==========

to

==========
Ensure that native WindowAndroid outlives native AwContents.

ContentViewCore holds a window pointer that is used during
destruction of native AwContents to remove observers. However the
current CleanupReference based finalization scheme does not enforce
an ordering on the destruction of native WindowAndroid and AwContents
instances. Satisfaction of the constraint that AwContents is destroyed
before WindowAndroid is therefore dependent on the CleanupReference
implementation, and possibly the implementation of the JVM as well.

Making the AwContents DestroyRunnable strongly reference the
associated WindowAndroidWrapper enforces the correct ordering.

BUG=595336
==========

[commit-bot: I haz the power, 2016-03-17 09:43:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-17 09:45:04]

Description was changed from

==========
Ensure that native WindowAndroid outlives native AwContents.

ContentViewCore holds a window pointer that is used during
destruction of native AwContents to remove observers. However the
current CleanupReference based finalization scheme does not enforce
an ordering on the destruction of native WindowAndroid and AwContents
instances. Satisfaction of the constraint that AwContents is destroyed
before WindowAndroid is therefore dependent on the CleanupReference
implementation, and possibly the implementation of the JVM as well.

Making the AwContents DestroyRunnable strongly reference the
associated WindowAndroidWrapper enforces the correct ordering.

BUG=595336
==========

to

==========
Ensure that native WindowAndroid outlives native AwContents.

ContentViewCore holds a window pointer that is used during
destruction of native AwContents to remove observers. However the
current CleanupReference based finalization scheme does not enforce
an ordering on the destruction of native WindowAndroid and AwContents
instances. Satisfaction of the constraint that AwContents is destroyed
before WindowAndroid is therefore dependent on the CleanupReference
implementation, and possibly the implementation of the JVM as well.

Making the AwContents DestroyRunnable strongly reference the
associated WindowAndroidWrapper enforces the correct ordering.

BUG=595336

Committed: https://crrev.com/81a813fcee09c1ed0f299aba3b589e3705238875
Cr-Commit-Position: refs/heads/master@{#381675}
==========

[commit-bot: I haz the power, 2016-03-17 09:45:05]

Patchset 2 (id:??) landed as
https://crrev.com/81a813fcee09c1ed0f299aba3b589e3705238875
Cr-Commit-Position: refs/heads/master@{#381675}

[Torne, 2016-03-17 10:21:41]

Change itself LGTM, for the record; both ways suck, and this is smaller, so hey.

On 2016/03/16 19:01:15, boliu wrote:
> On 2016/03/16 17:38:28, boliu wrote:
> > Immediate problem is CleanupReference lives in content and is tied to the UI
> > thread, but WindowAndroid is in a lower layer where there is no UI thread
> > concept.
> > 
> > Not sure how flexible CleanupReference could be made in general..
> 
> Hmm, in theory, CleanupReference could be made thread agnostic. Need to make
the
> Handler and the HashSet thread local. Still only need one reaper thread. Then
> CleanupReference can be moved to base. And constructing a CleanupReference,
have
> to specify a Looper as well to run the callback. All kind of complex though.
> 
> So then the debate becomes kinda philosophical again, whether an explicit
> destroy or using CleanupReference is better. Chrome has overwhelmingly chose
> explicit destroy, because android app APIs support that mode (although I have
> found places where finalizer is not used correctly in chrome, usually not
> realizing finalizer runs on some arbitrary thread)

No, I realise explicit destroy is better, but since we're expecting to support
apps not calling destroy then we have to just deal with mixing both, and it
seems like the simplest way to solve this is to have all the objects we care
about able to clean themselves up.

Also, we should really just ban finalizers completely with some static analyser
(findbugs?). Using them at all is a bug :)

> I lean towards the second: only use CleanupReference in webview because
webview
> api needs support for both.
> 
> 
> fwiw, would we have had this debate if I called it WindowAndroid*Reference*
> instead..?

I don't think that would help. The fundamental problem here is that you have an
object (WindowAndroid) which becomes broken (native counterpart destroyed) when
a different object that WindowAndroid knows nothing about becomes unreachable.
That's not how garbage collection is expected to work, and if we're using
explicit destroy() methods then it should generally be the case that destroy()
is only called when the caller is the only thing that can possibly still be
referencing the object and is about to drop its reference to allow it to be
collected (i.e. in the same circumstance you would use delete in c++).

[boliu, 2016-03-17 14:16:55]

On 2016/03/17 10:21:41, Torne wrote:
> Change itself LGTM, for the record; both ways suck, and this is smaller, so
hey.
> 
> On 2016/03/16 19:01:15, boliu wrote:
> > On 2016/03/16 17:38:28, boliu wrote:
> > > Immediate problem is CleanupReference lives in content and is tied to the
UI
> > > thread, but WindowAndroid is in a lower layer where there is no UI thread
> > > concept.
> > > 
> > > Not sure how flexible CleanupReference could be made in general..
> > 
> > Hmm, in theory, CleanupReference could be made thread agnostic. Need to make
> the
> > Handler and the HashSet thread local. Still only need one reaper thread.
Then
> > CleanupReference can be moved to base. And constructing a CleanupReference,
> have
> > to specify a Looper as well to run the callback. All kind of complex though.
> > 
> > So then the debate becomes kinda philosophical again, whether an explicit
> > destroy or using CleanupReference is better. Chrome has overwhelmingly chose
> > explicit destroy, because android app APIs support that mode (although I
have
> > found places where finalizer is not used correctly in chrome, usually not
> > realizing finalizer runs on some arbitrary thread)
> 
> No, I realise explicit destroy is better, but since we're expecting to support
> apps not calling destroy then we have to just deal with mixing both, and it
> seems like the simplest way to solve this is to have all the objects we care
> about able to clean themselves up.
> 
> Also, we should really just ban finalizers completely with some static
analyser
> (findbugs?). Using them at all is a bug :)
> 
> > I lean towards the second: only use CleanupReference in webview because
> webview
> > api needs support for both.
> > 
> > 
> > fwiw, would we have had this debate if I called it WindowAndroid*Reference*
> > instead..?
> 
> I don't think that would help. The fundamental problem here is that you have
an
> object (WindowAndroid) which becomes broken (native counterpart destroyed)
when
> a different object that WindowAndroid knows nothing about becomes unreachable.
> That's not how garbage collection is expected to work, and if we're using
> explicit destroy() methods then it should generally be the case that destroy()
> is only called when the caller is the only thing that can possibly still be
> referencing the object and is about to drop its reference to allow it to be
> collected (i.e. in the same circumstance you would use delete in c++).

Oh I see. A dirty hack would be have WindowAndroid hold the CleanupReference
after both are created. Maybe not as dirty as all the issues as I said
previously..

[Torne, 2016-03-17 14:36:25]

On 2016/03/17 14:16:55, boliu wrote:
> Oh I see. A dirty hack would be have WindowAndroid hold the CleanupReference
> after both are created. Maybe not as dirty as all the issues as I said
> previously..

Yeah.. really not sure what the ideal way to structure this is, all the options
seem pretty hacky in slightly different ways :(