Ensure WebContentsObserver.destroy is called

Ensure WebContentsObserver.destroy is called

When a Java-based WebContents instance is destroyed, it may yet have
Java-based WebContentsObserver instances. Previously, these instances
would always see a destroy (formerly detach) call, as they each had
a native counterpart.

With the introduction of the proxy WebContentsObserver, destroy was not
always called, as the remaining observers were simply cleared from the
observer list. Explicitly call WebContentsObserver.destroy for these
dangling instances, ensuring proper cleanup.

BUG=464076
Committed: https://crrev.com/28f5f23899a3131bcd63bf73707479d8fa082c20
Cr-Commit-Position: refs/heads/master@{#319162}

[jdduke (slow), 2015-03-04 22:01:27]

jdduke@chromium.org changed reviewers:
+ tedchoc@chromium.org

[jdduke (slow), 2015-03-04 22:01:28]

+tedchoc per earlier review, +dtrainor per offline discussion. This should fix
the issue with testTabsAreDestroyedOnModelDestruction.

[jdduke (slow), 2015-03-04 22:05:42]

https://codereview.chromium.org/981703002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java
(right):

https://codereview.chromium.org/981703002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:230:
mObservers.clear();
I'm tempted to slap a |assert mObservers.empty()| here...

[Ted C, 2015-03-04 22:11:53]

https://codereview.chromium.org/981703002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
(left):

https://codereview.chromium.org/981703002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:334:
if (!mObserverProxy.hasObservers()) {
Why did this get removed?  It was kind of nice that if you ended up removing all
observers for a web contents that it would stop paying the overhead of the JNI
calls.  But maybe that is something that never happens in practice and is
over-optimizing stuff.

I'm confused of the inter-connected-ness of these I guess.

[jdduke (slow), 2015-03-04 22:14:17]

https://codereview.chromium.org/981703002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
(left):

https://codereview.chromium.org/981703002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:334:
if (!mObserverProxy.hasObservers()) {
On 2015/03/04 22:11:53, Ted C wrote:
> Why did this get removed?  It was kind of nice that if you ended up removing
all
> observers for a web contents that it would stop paying the overhead of the JNI
> calls.  But maybe that is something that never happens in practice and is
> over-optimizing stuff.
> 
> I'm confused of the inter-connected-ness of these I guess.

I can add it back, that might result in re-entrant calls to
mObserverProxy.destroy(). I think we should be OK in that case, so up to you.

The more I look at this proxy change the less I like it, to be honest... sigh.

[Ted C, 2015-03-04 22:17:44]

lgtm

https://codereview.chromium.org/981703002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java
(left):

https://codereview.chromium.org/981703002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsImpl.java:334:
if (!mObserverProxy.hasObservers()) {
On 2015/03/04 22:14:17, jdduke wrote:
> On 2015/03/04 22:11:53, Ted C wrote:
> > Why did this get removed?  It was kind of nice that if you ended up removing
> all
> > observers for a web contents that it would stop paying the overhead of the
JNI
> > calls.  But maybe that is something that never happens in practice and is
> > over-optimizing stuff.
> > 
> > I'm confused of the inter-connected-ness of these I guess.
> 
> I can add it back, that might result in re-entrant calls to
> mObserverProxy.destroy(). I think we should be OK in that case, so up to you.
> 
> The more I look at this proxy change the less I like it, to be honest... sigh.

Fight the good fight...I still like it and you're a nice person for adding it. 
<squint>For now</squint>

Let's leave it out until we need to add it back.  We could also have the proxy
actually destroy the native side internally if it sees it has no more observers
to allow it to be in charge of it's own destiny.

https://codereview.chromium.org/981703002/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java
(right):

https://codereview.chromium.org/981703002/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/webcontents/WebContentsObserverProxy.java:230:
mObservers.clear();
On 2015/03/04 22:05:41, jdduke wrote:
> I'm tempted to slap a |assert mObservers.empty()| here...

Works for me.

[jdduke (slow), 2015-03-04 22:32:32]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-03-04 22:32:32]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/981703002/#ps40001 (title:
"Fix build")

[commit-bot: I haz the power, 2015-03-04 22:42:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/981703002/40001

[commit-bot: I haz the power, 2015-03-04 23:41:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-04 23:42:08]

Patchset 3 (id:??) landed as
https://crrev.com/28f5f23899a3131bcd63bf73707479d8fa082c20
Cr-Commit-Position: refs/heads/master@{#319162}