Reset Text Input State for RenderWidgetHostView before RenderWidgetHost Detaches from Delegate

Reset Text Input State for RenderWidgetHostView before RenderWidgetHost Detaches from Delegate

During the destruction of WebContentsImpl, the RWHs are first detached and then destroyed. If some RWHV
has an active input field, then it will never get a chance to notify the WebContentsImpl about
losing being destroyed and the WebContentsImpl will track the wrong text input state. This will
cause DCHECK in WebContentsImpl::GetTextInputState being triggered, specifically, for an outer WebContents
which might outlive the WebContents being destroyed.

BUG=602144, 601570
Committed: https://crrev.com/5292d041d34d49e8c3e851a0e21504a983e5be57
Cr-Commit-Position: refs/heads/master@{#387311}

[EhsanK, 2016-04-11 03:07:35]

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

[commit-bot: I haz the power, 2016-04-11 03:07:44]

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

[commit-bot: I haz the power, 2016-04-11 04:01:58]

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

[commit-bot: I haz the power, 2016-04-11 04:01:59]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2016-04-11 04:06:30]

ekaramad@chromium.org changed reviewers:
+ creis@chromium.org

[EhsanK, 2016-04-11 04:06:31]

PTAL.

For context, please take a look at WebContentsImpl::GetTextInputState(). We
added a DCHECK to make sure RWHV destruction will reset the state. The issue
here is that in WebContentsImpl dtor, WebContents detaches itself from RWH and
RWHV cannot notify the WebContents about losing text input state afterwards.
This is one (might be the only) reason behind the crash in the bug reported by
wfh@ (b/601570). I could repro this on debug using webview.

[Charlie Reis, 2016-04-11 05:45:32]

creis@chromium.org changed reviewers:
+ kenrb@chromium.org

[Charlie Reis, 2016-04-11 05:45:32]

[+kenrb]

Thanks for the fix.  Can you add a test for it, given the nice repro steps in
602144?

Ken, does this look reasonable to you?  I'm happy to stamp it, but I thought
you'd have more context.

[EhsanK, 2016-04-11 06:47:08]

On 2016/04/11 05:45:32, Charlie Reis wrote:
> [+kenrb]
> 
> Thanks for the fix.  Can you add a test for it, given the nice repro steps in
> 602144?
> 
> Ken, does this look reasonable to you?  I'm happy to stamp it, but I thought
> you'd have more context.

Thanks for taking a look at this again. Hmm...I guess I can repro the steps and
see that (we dont get a crash and) the outer WebContentsImpl has updated state.

[Charlie Reis, 2016-04-11 20:24:52]

Thanks.  I think you'll need someone familiar with BrowserPlugin to review the
test.  (I'm not an owner there, and just left a few nits to see if I understand
it.)

Can you run a linux_site_isolation try job as well, to make sure it passes in
--site-per-process mode?

https://codereview.chromium.org/1879453002/diff/20001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (left):

https://codereview.chromium.org/1879453002/diff/20001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1419: //
OOPIF-<webview> is fixed.
Why is this comment removed?

https://codereview.chromium.org/1879453002/diff/20001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right):

https://codereview.chromium.org/1879453002/diff/20001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1445: // This
test creats a <webview> with a text input field inside, gives focus to
nit: creates

https://codereview.chromium.org/1879453002/diff/20001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1453: //
Press tab key twice to end up in the <input> of the <webview>,
nit: End with period, not comma.

https://codereview.chromium.org/1879453002/diff/20001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1468:
content::ExecuteScript(embedder_web_contents(), "detachWebView();"));
How does detachWebView() crash the <webview>?  Or is the comment on 1465
incorrect?

https://codereview.chromium.org/1879453002/diff/20001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/1879453002/diff/20001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:783:
OnTextInputStateChanged(TextInputState());
This wasn't in patch set 1.  Is it also needed to fix the bug?

[EhsanK, 2016-04-11 21:59:18]

PTAL.

Adding lazyboy@ for BrowserPluginGuest and WebView test reviews.

creis@, kenrb@:
 I was wondering if using RenderWidgetHostViewObserver instead of the current
cleanup logic is a better approach. What I am suggesting is having something
like a "TextInputStateTracker" class which belongs to WebContentsImpl and is on
the observer list of all the RWHVs on the tab. That way updating the state will
not rely on RenderWidgetHostImpl or the inner WebContentsImpl losing their
reference to the outermost WebContents.

https://codereview.chromium.org/1879453002/diff/20001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (left):

https://codereview.chromium.org/1879453002/diff/20001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1419: //
OOPIF-<webview> is fixed.
On 2016/04/11 20:24:52, Charlie Reis wrote:
> Why is this comment removed?

The TODO is now at line 551, where the WebViewTextInputStateInteractiveTest is
defined. I did this in the last patch and removed the TODO for the test above
but missed this one.

https://codereview.chromium.org/1879453002/diff/20001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right):

https://codereview.chromium.org/1879453002/diff/20001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1445: // This
test creats a <webview> with a text input field inside, gives focus to
On 2016/04/11 20:24:52, Charlie Reis wrote:
> nit: creates

Done.

https://codereview.chromium.org/1879453002/diff/20001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1453: //
Press tab key twice to end up in the <input> of the <webview>,
On 2016/04/11 20:24:52, Charlie Reis wrote:
> nit: End with period, not comma.

Done.

https://codereview.chromium.org/1879453002/diff/20001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1468:
content::ExecuteScript(embedder_web_contents(), "detachWebView();"));
On 2016/04/11 20:24:52, Charlie Reis wrote:
> How does detachWebView() crash the <webview>?  Or is the comment on 1465
> incorrect?

Comment is incorrect. Thanks. s/crash/detach.

https://codereview.chromium.org/1879453002/diff/20001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/1879453002/diff/20001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:782: // input state which
is tracked by |owner_web_contents_| (crbug.com/602144).
Beyond this point, |attached_ = false| and hence,
|BrowserPluginGuest::owner_web_contents()| return null which breaks the link to
the outer WebContents from the inner WebContents. Therefore, this seems to be
the only place we can call this cleanup.

https://codereview.chromium.org/1879453002/diff/20001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:783:
OnTextInputStateChanged(TextInputState());
On 2016/04/11 20:24:52, Charlie Reis wrote:
> This wasn't in patch set 1.  Is it also needed to fix the bug?

Yes. I have added some comments to crbug.com/602144 regarding this. This is
another case where the destruction of WebContentsImpl does not allow the
RenderWidgetHostView(Guest) to reset the state.

[EhsanK, 2016-04-11 21:59:36]

ekaramad@chromium.org changed reviewers:
+ lazyboy@chromium.org

[EhsanK, 2016-04-11 21:59:36]

lazyboy@chromium.org: Please review changes in

[Charlie Reis, 2016-04-11 22:18:17]

Thanks, LGTM if lazyboy is happy with the browser_plugin_guest.cc change and the
test.

https://codereview.chromium.org/1879453002/diff/20001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (left):

https://codereview.chromium.org/1879453002/diff/20001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1419: //
OOPIF-<webview> is fixed.
On 2016/04/11 21:59:17, ehsaaan wrote:
> On 2016/04/11 20:24:52, Charlie Reis wrote:
> > Why is this comment removed?
> 
> The TODO is now at line 551, where the WebViewTextInputStateInteractiveTest is
> defined. I did this in the last patch and removed the TODO for the test above
> but missed this one.

Acknowledged.

https://codereview.chromium.org/1879453002/diff/20001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/1879453002/diff/20001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:782: // input state which
is tracked by |owner_web_contents_| (crbug.com/602144).
On 2016/04/11 21:59:18, ehsaaan wrote:
> Beyond this point, |attached_ = false| and hence,
> |BrowserPluginGuest::owner_web_contents()| return null which breaks the link
to
> the outer WebContents from the inner WebContents. Therefore, this seems to be
> the only place we can call this cleanup.

Ok, I'll defer to lazyboy@ here.

https://codereview.chromium.org/1879453002/diff/20001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:783:
OnTextInputStateChanged(TextInputState());
On 2016/04/11 21:59:17, ehsaaan wrote:
> On 2016/04/11 20:24:52, Charlie Reis wrote:
> > This wasn't in patch set 1.  Is it also needed to fix the bug?
> 
> Yes. I have added some comments to crbug.com/602144 regarding this. This is
> another case where the destruction of WebContentsImpl does not allow the
> RenderWidgetHostView(Guest) to reset the state.

Acknowledged.

[lazyboy, 2016-04-12 04:11:54]

One TODO suggestion for OnDetach(), let me know if you agree.

https://codereview.chromium.org/1879453002/diff/40001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right):

https://codereview.chromium.org/1879453002/diff/40001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1471: //
State should reset to none.
nit: TextInputState should reset.

https://codereview.chromium.org/1879453002/diff/40001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/1879453002/diff/40001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:782: // input state which
is tracked by |owner_web_contents_| (crbug.com/602144).
Generally you refer to a crbug when there's an issue in the current code and you
want to fix it later, possibly with adding TODO. This code seems like it will
fix 602144, so *probably* no need to refer to the crbug link here?

Ideally, we should be "detaching" the inner web_contents from the outer
web_contents through WebContentsImpl. There isn't a mechanism right now, but I
think eventually we should (that is a long standing TODO of mine).
Can we add a TODO saying we should perform the detach-ment through
WebContentsImpl, so that WebContentsImpl can take care of its lingering link to
incorrect TextInputState?

[EhsanK, 2016-04-12 15:06:50]

Thanks! PTAL.

https://codereview.chromium.org/1879453002/diff/40001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right):

https://codereview.chromium.org/1879453002/diff/40001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:1471: //
State should reset to none.
On 2016/04/12 04:11:54, lazyboy wrote:
> nit: TextInputState should reset.

Done.

https://codereview.chromium.org/1879453002/diff/40001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/1879453002/diff/40001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_guest.cc:782: // input state which
is tracked by |owner_web_contents_| (crbug.com/602144).
On 2016/04/12 04:11:54, lazyboy wrote:
> Generally you refer to a crbug when there's an issue in the current code and
you
> want to fix it later, possibly with adding TODO. This code seems like it will
> fix 602144, so *probably* no need to refer to the crbug link here?
> 
> Ideally, we should be "detaching" the inner web_contents from the outer
> web_contents through WebContentsImpl. There isn't a mechanism right now, but I
> think eventually we should (that is a long standing TODO of mine).
> Can we add a TODO saying we should perform the detach-ment through
> WebContentsImpl, so that WebContentsImpl can take care of its lingering link
to
> incorrect TextInputState?

> Generally you refer to a crbug when there's an issue in the current code and
you
> want to fix it later, possibly with adding TODO. This code seems like it will
> fix 602144, so *probably* no need to refer to the crbug link here?
Yes this is supposed to be part of the fix for crbug.com/602144.I was trying to
put more context into the comment explaining why we are doing this here. But I
see your point.

> Ideally, we should be "detaching" the inner web_contents from the outer
> web_contents through WebContentsImpl. There isn't a mechanism right now, but I
> think eventually we should (that is a long standing TODO of mine).
I agree. I was discussing this with lfg@ yesterday. I also think it is not the
best way to handle a detach. Specifically given that (from what I see), the
outer WebContents outlives the inner one and also BrowserPluginGuest. So we
should have a reference to it (and we do but owner_web_contents() returns
nullptr when |detached_| is false) for as long as we need.

> Can we add a TODO saying we should perform the detach-ment through
> WebContentsImpl, so that WebContentsImpl can take care of its lingering link
to
> incorrect TextInputState?
I will and I will also report a bug to track this.

[lazyboy, 2016-04-12 17:11:44]

https://codereview.chromium.org/1879453002/diff/60001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/text_input_state/window.js
(right):

https://codereview.chromium.org/1879453002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/text_input_state/window.js:35:
chrome.test.failed();
s/failed/fail

but we actually need to send a message instead, since this test is not running
as chrome.test.runTest. So we shouldn't use chrome.test.succeed/fail. e.g.
chrome.test.sendMessage('failed-to-detach')
and in cpp:
detach_listener.set_failure_message("failed-to-detach")

[EhsanK, 2016-04-12 17:35:43]

Thanks! PTAL.

https://codereview.chromium.org/1879453002/diff/60001/chrome/test/data/extens...
File
chrome/test/data/extensions/platform_apps/web_view/text_input_state/window.js
(right):

https://codereview.chromium.org/1879453002/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/web_view/text_input_state/window.js:35:
chrome.test.failed();
On 2016/04/12 17:11:44, lazyboy wrote:
> s/failed/fail
> 
> but we actually need to send a message instead, since this test is not running
> as chrome.test.runTest. So we shouldn't use chrome.test.succeed/fail. e.g.
> chrome.test.sendMessage('failed-to-detach')
> and in cpp:
> detach_listener.set_failure_message("failed-to-detach")

Done.

[lazyboy, 2016-04-12 17:37:42]

BrowserPluginGuest changes and tests LGTM.

[EhsanK, 2016-04-13 01:11:57]

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

[commit-bot: I haz the power, 2016-04-13 01:12:23]

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

[commit-bot: I haz the power, 2016-04-13 03:30:32]

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

[commit-bot: I haz the power, 2016-04-13 03:30:33]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[EhsanK, 2016-04-14 01:19:58]

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

[commit-bot: I haz the power, 2016-04-14 01:20:20]

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

[commit-bot: I haz the power, 2016-04-14 03:36:31]

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

[commit-bot: I haz the power, 2016-04-14 03:36:32]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2016-04-14 14:25:51]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-04-14 14:25:51]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1879453002/#ps80001
(title: "Addressing lazyboy@ comment")

[commit-bot: I haz the power, 2016-04-14 14:26:08]

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

[commit-bot: I haz the power, 2016-04-14 14:31:02]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-14 14:32:37]

Description was changed from

==========
Reset Text Input State for RenderWidgetHostView before RenderWidgetHost Detaches
from Delegate

During the destruction of WebContentsImpl, the RWHs are first detached and then
destroyed. If some RWHV
has an active input field, then it will never get a chance to notify the
WebContentsImpl about
losing being destroyed and the WebContentsImpl will track the wrong text input
state. This will
cause DCHECK in WebContentsImpl::GetTextInputState being triggered,
specifically, for an outer WebContents
which might outlive the WebContents being destroyed.

BUG=602144, 601570
==========

to

==========
Reset Text Input State for RenderWidgetHostView before RenderWidgetHost Detaches
from Delegate

During the destruction of WebContentsImpl, the RWHs are first detached and then
destroyed. If some RWHV
has an active input field, then it will never get a chance to notify the
WebContentsImpl about
losing being destroyed and the WebContentsImpl will track the wrong text input
state. This will
cause DCHECK in WebContentsImpl::GetTextInputState being triggered,
specifically, for an outer WebContents
which might outlive the WebContents being destroyed.

BUG=602144, 601570

Committed: https://crrev.com/5292d041d34d49e8c3e851a0e21504a983e5be57
Cr-Commit-Position: refs/heads/master@{#387311}
==========

[commit-bot: I haz the power, 2016-04-14 14:32:38]

Patchset 5 (id:??) landed as
https://crrev.com/5292d041d34d49e8c3e851a0e21504a983e5be57
Cr-Commit-Position: refs/heads/master@{#387311}

[EhsanK, 2016-04-15 19:18:02]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1887303004/ by ekaramad@chromium.org.

The reason for reverting is: This is needed to revert 
https://codereview.chromium.org/1652483002/. The original CL caused many
regressions..