aw: Cancel pending callbacks after native destroy

aw: Cancel pending callbacks after native destroy

Normal destroy already calls removeCallbacksAndMessages. This Cl covers
when the CleanupReference runs before the java side is garbage
collected.

Add a test for this, and stop this test suite repeating since they are
only unit tests.

Committed: https://crrev.com/d9836842ef4f93f2d9fd63f2080e4175f3601af9
Cr-Commit-Position: refs/heads/master@{#415629}

[boliu, 2016-08-30 22:15:57]

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

[boliu, 2016-08-30 22:15:57]

ptal

[boliu, 2016-08-30 22:17:48]

fwiw, I didn't figure out a better api than setCancelCallbackPoller. this setter
sucks because it's easy to forget calling it

seems it's not going to be easy to fix unless we do like a massive refactor
though :/

[Tobias Sargeant, 2016-08-31 09:44:14]

On 2016/08/30 22:17:48, boliu wrote:
> fwiw, I didn't figure out a better api than setCancelCallbackPoller. this
setter
> sucks because it's easy to forget calling it
> 
> seems it's not going to be easy to fix unless we do like a massive refactor
> though :/

LGTM. I agree that the solution is a bit weird, but it doesn't seem possible to
do better to me, either. Does referencing the AwContentsClientCallbackHelper
from the AwContents DestroyRunnable creates a root that stops GC? Or did you
discount doing that for another reason?

[boliu, 2016-08-31 13:22:47]

On 2016/08/31 09:44:14, Tobias Sargeant wrote:
> On 2016/08/30 22:17:48, boliu wrote:
> > fwiw, I didn't figure out a better api than setCancelCallbackPoller. this
> setter
> > sucks because it's easy to forget calling it
> > 
> > seems it's not going to be easy to fix unless we do like a massive refactor
> > though :/
> 
> LGTM. I agree that the solution is a bit weird, but it doesn't seem possible
to
> do better to me, either. Does referencing the AwContentsClientCallbackHelper
> from the AwContents DestroyRunnable creates a root that stops GC? Or did you
> discount doing that for another reason?

Why does that help..? Explicitly call clear?

[boliu, 2016-08-31 13:23:44]

On 2016/08/31 13:22:47, boliu wrote:
> On 2016/08/31 09:44:14, Tobias Sargeant wrote:
> > On 2016/08/30 22:17:48, boliu wrote:
> > > fwiw, I didn't figure out a better api than setCancelCallbackPoller. this
> > setter
> > > sucks because it's easy to forget calling it
> > > 
> > > seems it's not going to be easy to fix unless we do like a massive
refactor
> > > though :/
> > 
> > LGTM. I agree that the solution is a bit weird, but it doesn't seem possible
> to
> > do better to me, either. Does referencing the AwContentsClientCallbackHelper
> > from the AwContents DestroyRunnable creates a root that stops GC? Or did you
> > discount doing that for another reason?
> 
> Why does that help..? Explicitly call clear?

And yeah. That would be very dangerous because it's likely the client refs back
to AwContents.

[Tobias Sargeant, 2016-08-31 13:26:01]

On 2016/08/31 13:22:47, boliu wrote:
> On 2016/08/31 09:44:14, Tobias Sargeant wrote:
> > On 2016/08/30 22:17:48, boliu wrote:
> > > fwiw, I didn't figure out a better api than setCancelCallbackPoller. this
> > setter
> > > sucks because it's easy to forget calling it
> > > 
> > > seems it's not going to be easy to fix unless we do like a massive
refactor
> > > though :/
> > 
> > LGTM. I agree that the solution is a bit weird, but it doesn't seem possible
> to
> > do better to me, either. Does referencing the AwContentsClientCallbackHelper
> > from the AwContents DestroyRunnable creates a root that stops GC? Or did you
> > discount doing that for another reason?
> 
> Why does that help..? Explicitly call clear?

AwContentsClientCallbackHelper could provide a method like dropCallbacks, that
the DestroyRunnable called, which would make it easier to manage if we ever had
to do that from a second site.

[boliu, 2016-08-31 13:28:32]

On 2016/08/31 13:26:01, Tobias Sargeant wrote:
> On 2016/08/31 13:22:47, boliu wrote:
> > On 2016/08/31 09:44:14, Tobias Sargeant wrote:
> > > On 2016/08/30 22:17:48, boliu wrote:
> > > > fwiw, I didn't figure out a better api than setCancelCallbackPoller.
this
> > > setter
> > > > sucks because it's easy to forget calling it
> > > > 
> > > > seems it's not going to be easy to fix unless we do like a massive
> refactor
> > > > though :/
> > > 
> > > LGTM. I agree that the solution is a bit weird, but it doesn't seem
possible
> > to
> > > do better to me, either. Does referencing the
AwContentsClientCallbackHelper
> > > from the AwContents DestroyRunnable creates a root that stops GC? Or did
you
> > > discount doing that for another reason?
> > 
> > Why does that help..? Explicitly call clear?
> 
> AwContentsClientCallbackHelper could provide a method like dropCallbacks, that
> the DestroyRunnable called, which would make it easier to manage if we ever
had
> to do that from a second site.

Yeah that will break exactly as you outlined.

[boliu, 2016-08-31 13:28:37]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2016-08-31 13:28:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 14:49:24]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-31 14:51:59]

Description was changed from

==========
aw: Cancel pending callbacks after native destroy

Normal destroy already calls removeCallbacksAndMessages. This Cl covers
when the CleanupReference runs before the java side is garbage
collected.

Add a test for this, and stop this test suite repeating since they are
only unit tests.
==========

to

==========
aw: Cancel pending callbacks after native destroy

Normal destroy already calls removeCallbacksAndMessages. This Cl covers
when the CleanupReference runs before the java side is garbage
collected.

Add a test for this, and stop this test suite repeating since they are
only unit tests.

Committed: https://crrev.com/d9836842ef4f93f2d9fd63f2080e4175f3601af9
Cr-Commit-Position: refs/heads/master@{#415629}
==========

[commit-bot: I haz the power, 2016-08-31 14:52:00]

Patchset 1 (id:??) landed as
https://crrev.com/d9836842ef4f93f2d9fd63f2080e4175f3601af9
Cr-Commit-Position: refs/heads/master@{#415629}