Fix autofill popup controller key press callback registration

Fix autofill popup controller key press callback registration

Currently, PopupControllerCommon::RegisterKeyPressCallback registers its key
press handler via RenderView. This results in broken key handling with site
isolation. The fix is to use the focused RenderFrame itself.

BUG=688848
Review-Url: https://codereview.chromium.org/2762233004
Cr-Commit-Position: refs/heads/master@{#462381}
Committed: https://chromium.googlesource.com/chromium/src/+/6d61e0d262cc02641e963c1fc4fccd374555fad7

[Charlie Reis, 2017-03-22 16:59:42]

Description was changed from

==========
Fix autofill popup controller key press callback registration

COMMIT=false
This is still a WIP, missing polishing and tests.
Uploaded just for discussion and backup for now.

Currently, PopupControllerCommon::RegisterKeyPressCallback registers its key
press handler via RenderView. This results in broken key handling with site
isolation. The fix is to use the focused RenderFrame itself.

BUG=688848
==========

to

==========
Fix autofill popup controller key press callback registration

COMMIT=false
This is still a WIP, missing polishing and tests.
Uploaded just for discussion and backup for now.

Currently, PopupControllerCommon::RegisterKeyPressCallback registers its key
press handler via RenderView. This results in broken key handling with site
isolation. The fix is to use the focused RenderFrame itself.

BUG=688848
==========

[vabr (Chromium), 2017-03-24 22:47:09]

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

[commit-bot: I haz the power, 2017-03-24 22:47:38]

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

[vabr (Chromium), 2017-03-24 23:11:47]

Description was changed from

==========
Fix autofill popup controller key press callback registration

COMMIT=false
This is still a WIP, missing polishing and tests.
Uploaded just for discussion and backup for now.

Currently, PopupControllerCommon::RegisterKeyPressCallback registers its key
press handler via RenderView. This results in broken key handling with site
isolation. The fix is to use the focused RenderFrame itself.

BUG=688848
==========

to

==========
Fix autofill popup controller key press callback registration

Currently, PopupControllerCommon::RegisterKeyPressCallback registers its key
press handler via RenderView. This results in broken key handling with site
isolation. The fix is to use the focused RenderFrame itself.

BUG=688848
==========

[commit-bot: I haz the power, 2017-03-24 23:33:58]

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

[commit-bot: I haz the power, 2017-03-24 23:33:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vabr (Chromium), 2017-03-24 23:46:32]

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

[commit-bot: I haz the power, 2017-03-24 23:47:10]

Dry run: 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, 2017-03-25 00:47:56]

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

[commit-bot: I haz the power, 2017-03-25 00:47:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vabr (Chromium), 2017-03-25 09:52:30]

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

[commit-bot: I haz the power, 2017-03-25 09:52:38]

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

[vabr (Chromium), 2017-03-25 10:47:00]

The CQ bit was unchecked by vabr@chromium.org

[vabr (Chromium), 2017-03-25 11:14:08]

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

[commit-bot: I haz the power, 2017-03-25 11:14:18]

Dry run: 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, 2017-03-25 13:33:11]

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

[commit-bot: I haz the power, 2017-03-25 13:33:12]

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

[vabr (Chromium), 2017-03-25 17:41:27]

vabr@chromium.org changed reviewers:
+ ekaramad@chromium.org, mathp@chromium.org

[vabr (Chromium), 2017-03-25 17:41:28]

Hi all!

mathp@ -- Could you please review this as the code owner?

ekaramad@ -- Could you please have a look if this makes sense from the point of
view of site isolation? In particular, the way the code handles the focused
frame being null, and the RenderWidgetHostView being null in a non-null frame
(which happened on shutdown in tests).

kenrb@ and lfg@ -- I Cc-ed you based on https://crbug.com/688848#c14 in case you
had further comments from the site isolation side.

Thanks!
Vaclav

[Mathieu, 2017-03-26 02:03:51]

Thanks Vaclav, very cool tests! One question

https://codereview.chromium.org/2762233004/diff/80001/chrome/browser/autofill...
File chrome/browser/autofill/autofill_interactive_uitest.cc (right):

https://codereview.chromium.org/2762233004/diff/80001/chrome/browser/autofill...
chrome/browser/autofill/autofill_interactive_uitest.cc:1799: // Focuss the form
in the iframe and simulate choosing a suggestion via
nit: Focus

https://codereview.chromium.org/2762233004/diff/80001/chrome/browser/autofill...
chrome/browser/autofill/autofill_interactive_uitest.cc:1809: // Check that the
suggestion fas filled.
*was

https://codereview.chromium.org/2762233004/diff/80001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/popup_controller_common.cc (right):

https://codereview.chromium.org/2762233004/diff/80001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/popup_controller_common.cc:15:
content::RenderWidgetHost* KeyPressHandlingTarget(
since it's in the anonymous namespace, a short function comment would be good.

https://codereview.chromium.org/2762233004/diff/80001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/popup_controller_common.cc:22: return nullptr;
this is a sure crash on line 50, no?

[vabr (Chromium), 2017-03-26 03:25:25]

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

[commit-bot: I haz the power, 2017-03-26 03:25:36]

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

[vabr (Chromium), 2017-03-26 03:25:40]

Thanks, Mathieu!

https://codereview.chromium.org/2762233004/diff/80001/chrome/browser/autofill...
File chrome/browser/autofill/autofill_interactive_uitest.cc (right):

https://codereview.chromium.org/2762233004/diff/80001/chrome/browser/autofill...
chrome/browser/autofill/autofill_interactive_uitest.cc:1799: // Focuss the form
in the iframe and simulate choosing a suggestion via
On 2017/03/26 02:03:50, Mathieu wrote:
> nit: Focus

Done.

https://codereview.chromium.org/2762233004/diff/80001/chrome/browser/autofill...
chrome/browser/autofill/autofill_interactive_uitest.cc:1809: // Check that the
suggestion fas filled.
On 2017/03/26 02:03:51, Mathieu wrote:
> *was

Done.

https://codereview.chromium.org/2762233004/diff/80001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/popup_controller_common.cc (right):

https://codereview.chromium.org/2762233004/diff/80001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/popup_controller_common.cc:15:
content::RenderWidgetHost* KeyPressHandlingTarget(
On 2017/03/26 02:03:51, Mathieu wrote:
> since it's in the anonymous namespace, a short function comment would be good.

Done.

https://codereview.chromium.org/2762233004/diff/80001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/popup_controller_common.cc:22: return nullptr;
On 2017/03/26 02:03:51, Mathieu wrote:
> this is a sure crash on line 50, no?

For some reason I only saw the view being null during RemoveKeyPressCallback. My
guess is that during some shutdown sequence this might happen.

During the registering phase, there is no good way to handle this gracefully --
if there is no view and hence RenderWidgetHost, then autofill cannot listen to
keys pressed for the suggestions pop-up. That should not happen, and if it does,
we might as well have a crash to highlight it.

[commit-bot: I haz the power, 2017-03-26 04:50:01]

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

[commit-bot: I haz the power, 2017-03-26 04:50:02]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-03-26 13:01:01]

lgtm

[kenrb, 2017-03-27 19:07:39]

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

[kenrb, 2017-03-27 19:07:40]

I've had a look from an OOPIF perspective, and lgtm. The one thing that I had to
look into was to make sure that your call to getFocusedFrame() makes sense when
there is more than one WebContents in a tab (i.e. because of OOPIF-based
WebView), and it appears that the controller will be attached to the WebContents
that needed the autofill popup, so this should be fine.

[EhsanK, 2017-03-27 19:35:09]

LGTM but there seems to be a minor bug with frame focus + setting and removing
handlers.

https://codereview.chromium.org/2762233004/diff/120001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/popup_controller_common.cc (right):

https://codereview.chromium.org/2762233004/diff/120001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/popup_controller_common.cc:52:
key_press_event_target_ = KeyPressHandlingTarget(web_contents_);
I think both here and below for RemoveKeyPressCallback() there is a potential
race between CotnentAutofillDriver IPC and frame focus IPCs. The scenario I am
thinking of is this:
1- User enters a character in an autocomplete input field which triggers a call
to AutofillAgent (which I think will talk to ContentAutofillDriver?).
2- Another frame takes focus (in a different renderer). Either through
window.focus() or mouse click. Since the FrameHostMsg_SetFocus comes from
another renderer, the set focus might happens sooner. Then we are setting the
handler on a RenderWidgetHost which is not even focused.

I think the solution on the Browser side is to verify the focused frame has the
same local root than the ContentAutofillDriver's RenderFrameHost. But generally,
I think all this logic should have been based on RenderFrameHost rather than
WebContents.

All this being said, I also have a simple question from you or kenrb@. Is it
guaranteed that the IPC for ContentAutofillDriver and other IPCs arrive in the
extact order they were sent. I am worrying FrameHostMsg_RenderProcessGone. If we
trigger autofill and then crash the renderer right away, is it possible that the
RenderWidgetHostView becomes nullptr before we get here (and hence browser
crashes here?).

https://codereview.chromium.org/2762233004/diff/120001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/popup_controller_common.cc:61:
key_press_event_target_->RemoveKeyPressEventCallback(
There seems to be a memory leak, i.e., callbacks which won't get removed for a
RenderWidgetHost.

To repro, Assume you have RWH-A and RWH-B on the same page. You trigger autofill
popup for RWH-A and then click into RWH-B. Then RWH-B gets focused. But then
since the SetFocus IPC is coming from process B, there is no guarantee it will
arive after the call to ContentAutofillDriver::DidEndTextFieldEditing. In that
case what has happened is that we have had a callback for RWH-A which we try to
remove from RWH-B.

This is also another reason why I think the logic here should mostly deal with
the RenderFrameHost of the ContentAutofillDriver rather than WebContents.

Maybe what we should do here is to just use the |key_press_event_target_| to
remove the handler because we know for a fact we have had one on it in call to
RegisterKeyPressCallback().

[vabr (Chromium), 2017-03-27 20:28:53]

Thanks for the detailed review!

I am not sure about the guarantees on the order of IPCs. I would be surprised if
there were any.

I am currently looking at how much work would be to scope all this logic within
a single frame.

In the meantime, one partial reaction to one of the comments below.

Cheers,
Vaclav

https://codereview.chromium.org/2762233004/diff/120001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/popup_controller_common.cc (right):

https://codereview.chromium.org/2762233004/diff/120001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/popup_controller_common.cc:61:
key_press_event_target_->RemoveKeyPressEventCallback(
On 2017/03/27 19:35:09, EhsanK wrote:
> There seems to be a memory leak, i.e., callbacks which won't get removed for a
> RenderWidgetHost.
> 
> To repro, Assume you have RWH-A and RWH-B on the same page. You trigger
autofill
> popup for RWH-A and then click into RWH-B. Then RWH-B gets focused. But then
> since the SetFocus IPC is coming from process B, there is no guarantee it will
> arive after the call to ContentAutofillDriver::DidEndTextFieldEditing. In that
> case what has happened is that we have had a callback for RWH-A which we try
to
> remove from RWH-B.
> 
> This is also another reason why I think the logic here should mostly deal with
> the RenderFrameHost of the ContentAutofillDriver rather than WebContents.
> 
> Maybe what we should do here is to just use the |key_press_event_target_| to
> remove the handler because we know for a fact we have had one on it in call to
> RegisterKeyPressCallback().

The idea to use |key_press_event_target_| seems interesting. But I am worried
that if the object it points to has been destroyed in the meantime, there is no
way to know it here.

[vabr (Chromium), 2017-03-27 20:44:59]

So I do see a way to scope the PopupControllerCommon instance to a frame/driver:
the first call to show the pop-up comes from AutofillExternalDelegate (which is
one-per-driver), and the driver information can be recovered inside
AutofillPopupControllerImpl. So I could change PopupControllerCommon to accept
the driver instead of the WebContents, and add a method on the driver to
register and unregister the key press handler. That should solve both issues
about dealing with deleted frames, and about forgetting to unregister the
handlers.

I do not see any easier way to do this.

This means I will need to rework the CL significantly. I will ping the review
once I'm done.

Thanks again for the thorough review and uncovering these issues.

Cheers,
Vaclav

[EhsanK, 2017-03-27 21:25:21]

On 2017/03/27 20:28:53, vabr (Chromium) wrote:
> Thanks for the detailed review!
> 
> I am not sure about the guarantees on the order of IPCs. I would be surprised
if
> there were any.
> 
> I am currently looking at how much work would be to scope all this logic
within
> a single frame.
> 
> In the meantime, one partial reaction to one of the comments below.
> 
> Cheers,
> Vaclav
> 
>
https://codereview.chromium.org/2762233004/diff/120001/chrome/browser/ui/auto...
> File chrome/browser/ui/autofill/popup_controller_common.cc (right):
> 
>
https://codereview.chromium.org/2762233004/diff/120001/chrome/browser/ui/auto...
> chrome/browser/ui/autofill/popup_controller_common.cc:61:
> key_press_event_target_->RemoveKeyPressEventCallback(
> On 2017/03/27 19:35:09, EhsanK wrote:
> > There seems to be a memory leak, i.e., callbacks which won't get removed for
a
> > RenderWidgetHost.
> > 
> > To repro, Assume you have RWH-A and RWH-B on the same page. You trigger
> autofill
> > popup for RWH-A and then click into RWH-B. Then RWH-B gets focused. But then
> > since the SetFocus IPC is coming from process B, there is no guarantee it
will
> > arive after the call to ContentAutofillDriver::DidEndTextFieldEditing. In
that
> > case what has happened is that we have had a callback for RWH-A which we try
> to
> > remove from RWH-B.
> > 
> > This is also another reason why I think the logic here should mostly deal
with
> > the RenderFrameHost of the ContentAutofillDriver rather than WebContents.
> > 
> > Maybe what we should do here is to just use the |key_press_event_target_| to
> > remove the handler because we know for a fact we have had one on it in call
to
> > RegisterKeyPressCallback().
> 
> The idea to use |key_press_event_target_| seems interesting. But I am worried
> that if the object it points to has been destroyed in the meantime, there is
no
> way to know it here.

In that case how about storing the pair (routing_id, process_id) and use
RenderProcessHost::FromID()?

[vabr (Chromium), 2017-04-05 09:03:40]

Just a clarification -- patch 8 is a new approach (more details on the bug), and
it is not finished yet (uploaded just for backup).

Once I finish the new approach I will explicitly request a re-review.

Cheers,
Vaclav

[vabr (Chromium), 2017-04-05 13:19:01]

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

[commit-bot: I haz the power, 2017-04-05 13:19:17]

Dry run: 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, 2017-04-05 14:11:40]

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

[commit-bot: I haz the power, 2017-04-05 14:11:41]

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

[vabr (Chromium), 2017-04-05 14:50:16]

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

[commit-bot: I haz the power, 2017-04-05 14:51:00]

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

[vabr (Chromium), 2017-04-05 14:53:34]

Hi all,

Thanks for your patience, the CL is now redone. The
autofill_interactive_uitest.cc and cross_origin_iframe.html stayed the same, all
the rest is completely different from the last reviewed patch.

The design is explained in
https://docs.google.com/document/d/14_OAxdRUaJjhnpp7R9D_wZ_hOImFtORSW6E-RgUFm...,
I'm happy to take questions both here and in that doc.

Please review as follows:

mathp@ -- Please review the whole patch

ekaramad@, kenrb@ -- Any input from you, especially about the OOPIF aspect, is
welcome.

I will block landing this on approval from mathp@ and at least one of
{ekaramad@, kenrb@}.

Thanks all for your help!
Vaclav

https://codereview.chromium.org/2762233004/diff/220001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (left):

https://codereview.chromium.org/2762233004/diff/220001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:38: if
(previous.get() && previous->web_contents() == web_contents &&
The same delegate implies the same web_contents (delegate is per frame), so I
dropped the web_contents check.

https://codereview.chromium.org/2762233004/diff/220001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.h (right):

https://codereview.chromium.org/2762233004/diff/220001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_controller_impl.h:25: struct
NativeWebKeyboardEvent;
This is used on line 65. Due to reduction in popup_controller_common.h, it is
now necessary to declare it here.

[commit-bot: I haz the power, 2017-04-05 15:49:20]

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

[commit-bot: I haz the power, 2017-04-05 15:49:21]

Dry run: This issue passed the CQ dry run.

[EhsanK, 2017-04-05 17:21:53]

Thanks Vaclav! LGTM.

https://codereview.chromium.org/2762233004/diff/220001/components/autofill/co...
File components/autofill/content/browser/content_autofill_driver.cc (right):

https://codereview.chromium.org/2762233004/diff/220001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver.cc:272: void
ContentAutofillDriver::RegisterKeyPressHandler(
I believe this is the only part of the CL which relates to OOPIFs.

[Mathieu, 2017-04-05 18:58:21]

Good job, Vaclav! lgtm

https://codereview.chromium.org/2762233004/diff/220001/components/autofill/co...
File components/autofill/content/browser/key_press_handler_manager.cc (right):

https://codereview.chromium.org/2762233004/diff/220001/components/autofill/co...
components/autofill/content/browser/key_press_handler_manager.cc:17: // same
function with the same bound arguments. That's not Equals() does (they
*what Equals() does

[vabr (Chromium), 2017-04-06 05:39:59]

Thanks for the quick reviews!

I am sending to the CQ now.

Cheers,
Vaclav

https://codereview.chromium.org/2762233004/diff/220001/components/autofill/co...
File components/autofill/content/browser/content_autofill_driver.cc (right):

https://codereview.chromium.org/2762233004/diff/220001/components/autofill/co...
components/autofill/content/browser/content_autofill_driver.cc:272: void
ContentAutofillDriver::RegisterKeyPressHandler(
On 2017/04/05 17:21:53, EhsanK wrote:
> I believe this is the only part of the CL which relates to OOPIFs.

The switch from the popup controller using objects tied to RenderFrame Host
instead of WebContents is another one, IMO.

https://codereview.chromium.org/2762233004/diff/220001/components/autofill/co...
File components/autofill/content/browser/key_press_handler_manager.cc (right):

https://codereview.chromium.org/2762233004/diff/220001/components/autofill/co...
components/autofill/content/browser/key_press_handler_manager.cc:17: // same
function with the same bound arguments. That's not Equals() does (they
On 2017/04/05 18:58:21, Mathieu wrote:
> *what Equals() does

Done.

[vabr (Chromium), 2017-04-06 05:40:05]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2017-04-06 05:40:05]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
mathp@chromium.org, ekaramad@chromium.org
Link to the patchset: https://codereview.chromium.org/2762233004/#ps240001
(title: "Fix typo")

[commit-bot: I haz the power, 2017-04-06 05:40:18]

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, 2017-04-06 07:06:45]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1491457205368090,
"parent_rev": "912e5f787ee415f874928b9684ea50dc75f85d40", "commit_rev":
"6d61e0d262cc02641e963c1fc4fccd374555fad7"}

[commit-bot: I haz the power, 2017-04-06 07:07:51]

Description was changed from

==========
Fix autofill popup controller key press callback registration

Currently, PopupControllerCommon::RegisterKeyPressCallback registers its key
press handler via RenderView. This results in broken key handling with site
isolation. The fix is to use the focused RenderFrame itself.

BUG=688848
==========

to

==========
Fix autofill popup controller key press callback registration

Currently, PopupControllerCommon::RegisterKeyPressCallback registers its key
press handler via RenderView. This results in broken key handling with site
isolation. The fix is to use the focused RenderFrame itself.

BUG=688848

Review-Url: https://codereview.chromium.org/2762233004
Cr-Commit-Position: refs/heads/master@{#462381}
Committed:
https://chromium.googlesource.com/chromium/src/+/6d61e0d262cc02641e963c1fc4fc...
==========

[commit-bot: I haz the power, 2017-04-06 07:07:53]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/6d61e0d262cc02641e963c1fc4fc...