aw: Destroy ContentViewCore and WebContents together

aw: Destroy ContentViewCore and WebContents together

Even though it's having WebContents without a ContentViewCore
is a support scenario in //content, it's not a well tested
path, especially the webview only code. This makes sure
ContentViewCore and WebContents are destroyed together.

Instead of posting on native side to destroy WebContents,
post on Java side and have one call destroy both Java and
native side.

More specific details:
* All caller except destroy to AwContents.destroyNatives
  either have already detached webview so ok to move detach
  to destroy.
* Need to remove some asserts that objects are null when
  mIsDestroyed is true.
* Reordered some native AwContents members so they are
  naturally destroyed in the correct order.

BUG=

Committed: https://crrev.com/4947ca3ee453380304d0bbb6a382b3eea93a6fca
Cr-Commit-Position: refs/heads/master@{#302513}

[boliu, 2014-10-29 01:49:21]

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

[boliu, 2014-10-29 01:50:52]

PTAL.

Having to create a new handler just for this kinda sucks, but apparently using
mContainerView.post causes a test polling for destroy to fail.

[benm (inactive), 2014-10-29 14:56:15]

Hmm, need to think carefully about this. We now have a window where after
destroy() is called the functions that check for a valid native pointer may
pass, so there's a potential for racing?

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
File android_webview/native/aw_contents.cc (left):

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
android_webview/native/aw_contents.cc:295: // See b/15074651.
Can you check the repro case from this internal bug is still fixed?

[boliu, 2014-10-29 15:14:33]

On 2014/10/29 14:56:15, benm wrote:
> Hmm, need to think carefully about this. We now have a window where after
> destroy() is called the functions that check for a valid native pointer may
> pass, so there's a potential for racing?

Two points:

1) Why is it a bad thing? We haven't destroyed anything yet, so we actually can
call any method we want.
2) Everything should be checking isDestroyed() instead of mNativeAwContents ==
0. So as long as we set mIsDestroyed to true, everything should stop calling
into native, which we do.

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
File android_webview/native/aw_contents.cc (left):

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
android_webview/native/aw_contents.cc:295: // See b/15074651.
On 2014/10/29 14:56:15, benm wrote:
> Can you check the repro case from this internal bug is still fixed?

Will do. I'll let you the result afterwards.

[boliu, 2014-10-29 18:51:59]

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
File android_webview/native/aw_contents.cc (left):

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
android_webview/native/aw_contents.cc:295: // See b/15074651.
On 2014/10/29 15:14:33, boliu wrote:
> On 2014/10/29 14:56:15, benm wrote:
> > Can you check the repro case from this internal bug is still fixed?
> 
> Will do. I'll let you the result afterwards.

Not conclusive :(

Nothing appeared wrong when I followed the steps in b/15074651. But then I
reverted this change, and then commented out these two lines and still no
issues, which means I was never able to reproduce the original problem..

[boliu, 2014-10-29 18:56:39]

On 2014/10/29 18:51:59, boliu wrote:
>
https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
> File android_webview/native/aw_contents.cc (left):
> 
>
https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
> android_webview/native/aw_contents.cc:295: // See b/15074651.
> On 2014/10/29 15:14:33, boliu wrote:
> > On 2014/10/29 14:56:15, benm wrote:
> > > Can you check the repro case from this internal bug is still fixed?
> > 
> > Will do. I'll let you the result afterwards.
> 
> Not conclusive :(
> 
> Nothing appeared wrong when I followed the steps in b/15074651. But then I
> reverted this change, and then commented out these two lines and still no
> issues, which means I was never able to reproduce the original problem..

Oh I think this was fixed because setNewAwContents calls destroyNatives (which
is synchronous), not destroy.

[boliu, 2014-10-30 15:44:09]

ping?

[benm (inactive), 2014-10-30 18:37:27]

On 2014/10/30 15:44:09, boliu wrote:
> ping?

Hey dude really sorry for the delay, been swamped the past couple of days. Will
be sure to get to this tomorrow.

[benm (inactive), 2014-10-31 13:53:29]

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

https://codereview.chromium.org/690553002/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContents.java:572:
mHandler = new Handler();
does this need to be a member? destroy should come in on the UI thread right? In
which case we should be abel to just create a local I think?

[sgurun-gerrit only, 2014-10-31 19:01:43]

sgurun@chromium.org changed reviewers:
+ sgurun@chromium.org

[sgurun-gerrit only, 2014-10-31 19:01:43]

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
File android_webview/native/aw_contents.cc (left):

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
android_webview/native/aw_contents.cc:306:
BrowserThread::DeleteSoon(BrowserThread::UI, FROM_HERE, this);
Have you tried the condition here? Does it work fine with your changes? (note
that I just saw this and only skimmed it for a few minutes)

[boliu, 2014-10-31 19:49:01]

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
File android_webview/native/aw_contents.cc (left):

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
android_webview/native/aw_contents.cc:306:
BrowserThread::DeleteSoon(BrowserThread::UI, FROM_HERE, this);
On 2014/10/31 19:01:43, sgurun wrote:
> Have you tried the condition here? Does it work fine with your changes? (note
> that I just saw this and only skimmed it for a few minutes)

Not sure what you are asking here. What condition?

[hush (inactive), 2014-10-31 20:58:15]

hush@chromium.org changed reviewers:
+ hush@chromium.org

[hush (inactive), 2014-10-31 20:58:16]

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
File android_webview/native/aw_contents.cc (left):

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
android_webview/native/aw_contents.cc:306:
BrowserThread::DeleteSoon(BrowserThread::UI, FROM_HERE, this);
On 2014/10/31 19:49:01, boliu wrote:
> On 2014/10/31 19:01:43, sgurun wrote:
> > Have you tried the condition here? Does it work fine with your changes?
(note
> > that I just saw this and only skimmed it for a few minutes)
> 
> Not sure what you are asking here. What condition?

Will an app that calls WebView#destroy() in ShouldOverrideUrlLoading work after
your patch?

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
android_webview/native/aw_contents.h:237: scoped_ptr<content::WebContents>
web_contents_;
what's the purpose of this move?

[boliu, 2014-10-31 21:47:05]

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
File android_webview/native/aw_contents.cc (left):

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
android_webview/native/aw_contents.cc:306:
BrowserThread::DeleteSoon(BrowserThread::UI, FROM_HERE, this);
On 2014/10/31 20:58:16, hush wrote:
> On 2014/10/31 19:49:01, boliu wrote:
> > On 2014/10/31 19:01:43, sgurun wrote:
> > > Have you tried the condition here? Does it work fine with your changes?
> (note
> > > that I just saw this and only skimmed it for a few minutes)
> > 
> > Not sure what you are asking here. What condition?
> 
> Will an app that calls WebView#destroy() in ShouldOverrideUrlLoading work
after
> your patch?

I think it's pretty obvious it will work. Will add a test to make certain, as I
can't find the app that does this from code archeology.

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
File android_webview/native/aw_contents.h (right):

https://codereview.chromium.org/690553002/diff/40001/android_webview/native/a...
android_webview/native/aw_contents.h:237: scoped_ptr<content::WebContents>
web_contents_;
On 2014/10/31 20:58:16, hush wrote:
> what's the purpose of this move?

This is the "Reordered some native AwContents members so they are naturally
destroyed in the correct order." part of the description.

Member variables are destroyed in the reverse order they are listed in the class
definition.

WebContents expects the delegate to outlive itself. And destroying WebContents
before AwContentsClientBridge does the same thing as
AwContentsClientBridgeBase::Associate before this patch.

[boliu, 2014-10-31 22:56:54]

On 2014/10/31 21:47:05, boliu wrote:
> I think it's pretty obvious it will work. Will add a test to make certain, as
I
> can't find the app that does this from code archeology.

Bahh, can't write this test. shouldInterceptRequest happens on IO thread, so it
can't call destroy directly, and blocking on UI thread in that is just asking
for trouble imo. Sad the public doc doesn't say this.

I'll try finding some other callback that is on UI.

[boliu, 2014-10-31 23:15:25]

Made the change Ben suggested. Added a test using onPageFinished

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

https://codereview.chromium.org/690553002/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwContents.java:572:
mHandler = new Handler();
On 2014/10/31 13:53:29, benm wrote:
> does this need to be a member? destroy should come in on the UI thread right?
In
> which case we should be abel to just create a local I think?

I wasn't concerned about threading issues as much as what happens when the last
ref to the Handler goes away.

But then this is java and this is probably ok: if handler is gc-ed, it doesn't
matter if the callback fired or not, we'll be destroying native anyway.

[benm (inactive), 2014-11-03 14:26:18]

lgtm

[boliu, 2014-11-03 16:00:51]

sgurun/hush: Any more comments?

[hush (inactive), 2014-11-03 18:25:17]

On 2014/11/03 16:00:51, boliu wrote:
> sgurun/hush: Any more comments?

no more from me. Thanks!

[sgurun-gerrit only, 2014-11-03 22:14:52]

On 2014/11/03 18:25:17, hush wrote:
> On 2014/11/03 16:00:51, boliu wrote:
> > sgurun/hush: Any more comments?
> 
> no more from me. Thanks!

lgtm once we verify calling destroy in *shouldoverrideurlloading* works

[sgurun-gerrit only, 2014-11-03 22:34:28]

still lgtm

[boliu, 2014-11-03 22:36:28]

On 2014/11/03 22:34:28, sgurun wrote:
> still lgtm

Selim pointed out I confused shouldOverrideUrlLoading (called on UI) with
shouldInterceptRequest (called on IO) in my previous attempt at writing that
test. Glitch fixed :)

[boliu, 2014-11-03 22:36:40]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-11-03 22:37:29]

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

[commit-bot: I haz the power, 2014-11-03 23:24:44]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-03 23:25:26]

Patchset 6 (id:??) landed as
https://crrev.com/4947ca3ee453380304d0bbb6a382b3eea93a6fca
Cr-Commit-Position: refs/heads/master@{#302513}