Thanks for the report!  This likely affects extensions with web iframes on Stable, since we have launched --isolate-extensions.

Can someone from Autofill help us track down why this is happening?  I wonder if some aspect of the code needs to be moved from RenderViews to RenderFrames.

The relevant method seems to be AutofillPopupControllerImpl::HandleKeyPressEvent. My day is over today, but I will try to help tomorrow.

Thanks for offering to look into it, vabr@!  Hopefully we can get the fix into M59 and merged to M58, if it's simple enough.

Hi creis@, while I would love to help, let me set the expectations straight:
I don't have much technical time to spare, and several projects competing in that area. For internal reasons which I should not discuss here, most of my work time is sadly spent outside of coding. Also, this week I only have a laptop to code on and work from a remote location.

So I cannot commit to M59. I can do my best to get more hints here and write them down on this ticket.

Sure-- if you can point us in the right direction (and possibly help with the review), someone on our team might be able to write the fix.  Thanks!

Thanks for being so accommodating!

My laptop still compiles, but meanwhile I'm looking at PopupControllerCommon::RegisterKeyPressCallback [1], which sets the key press event handler on web_contents_->GetRenderViewHost()->GetWidget().

Should that be GetRenderFrameHost()? 


[1] https://cs.chromium.org/chromium/src/chrome/browser/ui/autofill/popup_controller_common.cc?gsn=SetKeyPressCallback&l=33

Just matching the types, it seems to me that I should be able to change 

web_contents_->GetRenderViewHost()->GetWidget()
to
web_contents_->GetFocusedFrame()->GetView()->GetRenderWidgetHost()->GetWidget()

Once the compilation is ready (Goma, I miss you), I will verify that hypothesis. Comments or early-failure warnings always welcome. :)

I could reproduce the issue and verify that changing to RenderFrame fixes it. The very rough patch is https://codereview.chromium.org/2762233004, but awaits tests and some polishing. I would appreciate when site isolation experts could take a look at it already though and sanity check it. (I will trigger a proper code review once the tests are added.)

As always, the biggest challenge is testing. :)

The closest fit to reproduce the manual scenario here seems to be chrome/browser/autofill/autofill_interactive_uitest.cc. I can:
* create the site with a X-origin iframe
* trick the single test server to serve both origins by host_resolver()->AddRule("*", "127.0.0.1");
* hopefully use https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage to focus elements inside that iframe and retrieve the filled results

My open question is: when sending key press events, the existing tests use RenderViewHost to locate the RenderWidgetHost to send those events to (see SendKeyToPopupAndWait inside autofill_interactive_uitest.cc). That sounds like the same issue I am fixing in the code itself. Does this needs to be changed?

Site isolation experts, your advice is explicitly sought for the closing question of #10 above.

Comment #10:
To focus <input> it might be easier to just ExecuteScript and ExecuteScriptAndExtractString on the RenderFrameHost since it will be passed on as a user gesture so most focus relate things should work the same way as user input focus. But post message is also quite nice.

Thanks ekaramad@!  And I agree that the tests will likely need to be updated as well.

ekaramad@, kenrb@, and lfg@ may be good people to help review the CL and answer further questions.

Thanks! I will loop the people specified in #14 in once I have a CL to review (hopefully soon).

Also, sorry for the confusing #12, that CL should have landed in bug 697817. I will delete that comment to make tracking this issue clearer.

Still wrestling with the test. Currently I am working on expanding the autofill_interactive_uitest fixture to also listen to form changes in subframes (currently it only sets the listener-test delegate on the driver corresponding to the main frame), and on figuring out how to generate a valid URL origin different from the one served primarily by the embedded test server (I am using host_resolver()->AddRule("*", "127.0.0.1"), but so far still getting 404s).

I have a vague idea of how to do the former, and I'll check other cross-frame-related tests for the latter. This is just an update explaining why there is still no CL sent for review.

With help on this bug and from mathp@ through chat, I managed to get the test working (=failing before and passing after the patch): https://codereview.chromium.org/2762233004
I am waiting for the trybots to finish, and once all is green, I will ask mathp@ for an owner's review, and ekaramad@ (with kenrb@ and lfg@ Cc-ed) for checking the site-isolation bits of it.

The review caught some significant issues. I will need to rework the code a little more to fix that. See https://codereview.chromium.org/2762233004/#msg31 for more details.

I summarised my plan for addressing the issues (see #18) in https://docs.google.com/document/d/14_OAxdRUaJjhnpp7R9D_wZ_hOImFtORSW6E-RgUFmnM/edit?usp=sharing

I would appreciate comments, in particular from mathp@ and ekaramad@ (but everyone welcome).

Once there is a conclusion on the plan, I will implement it in https://codereview.chromium.org/2762233004/ and request code review again.

Thanks for the comments on the design so far!
I have no time left today, but tomorrow I will implement the design and address the concerns raised on the document.

Thanks for your patience, everyone.
As I started implementing the design, I realised it needs some more simplifying and structural changes. I bypassed PopupControllerCommon, reduced the key press handler registration to just ContentAutofillDriver and merged the past ContentKeyPressHandlerManager with ContentAutofillDriver (keeping KeyPressHandlerManager separate). There are bits I'm still not thrilled about (downcasting *Driver to just Content*Driver in popup controllers), but I'm sufficiently fine with it to continue implementing.

The updated design is still at https://docs.google.com/document/d/14_OAxdRUaJjhnpp7R9D_wZ_hOImFtORSW6E-RgUFmnM/edit?usp=sharing.

Non-coding duties prevent me from continuing today, and tomorrow might be a bit challenging for other reasons, so it might happen that I will only be continuing this on Monday. I'm sorry for this taking so long, please let me know if this needs to be treated more urgently.

Since OOPIFs are enabled on stable, we need to fix this as quickly as possible. Is there a chance of this slipping from M59? What can we help with to ensure it doesn't?

I understand the urgency. Unfortunately I was sick and unable to work since Friday. I am still not feeling ideal, but I slowly start working today. The design linked from #21 seems finalised, I started implementing it on Thursday. That work in progress is on my work computer which I won't have access to until at least tomorrow. I might find time to redo that all today, but it's unlikely, as I have also a bunch of other urgent tasks piled up on me since I was sick. 

As for how can you help: I'm not sure. The only TODO is the implementation of the design. If anyone feels they can do it within a day or so, that might help. Otherwise I'm hopefully to deliver it not much later.

Thanks for the update and I'm sorry to hear you are not feeling well. Take care of yourself first!

Thanks!
So far I plan on coming to the office tomorrow, and I already booked my morning to spend on this fix. I'll see how far I get, will post an update soon.

More updates: My mornings time is up and the code is not finished yet. However, only a small part is still a TODO, so if I'm lucky I'll find time tonight to finish it, or tomorrow in the worst case. I keep the original CL updated with the work in progress, so that I can continue from home in case I'm away from the office again.

I will explicitly ping for review once the change is ready.

https://codereview.chromium.org/2762233004/ is now ready and I already requested a re-review there.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6d61e0d262cc02641e963c1fc4fccd374555fad7

commit 6d61e0d262cc02641e963c1fc4fccd374555fad7
Author: vabr <vabr@chromium.org>
Date: Thu Apr 06 07:06:40 2017

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}

[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/chrome/browser/autofill/autofill_interactive_uitest.cc
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/chrome/browser/ui/autofill/autofill_popup_controller_impl.h
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/chrome/browser/ui/autofill/password_generation_popup_controller_impl.h
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/chrome/browser/ui/autofill/password_generation_popup_view_browsertest.cc
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/chrome/browser/ui/autofill/popup_controller_common.cc
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/chrome/browser/ui/autofill/popup_controller_common.h
[add] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/chrome/test/data/autofill/cross_origin_iframe.html
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/components/autofill/content/browser/BUILD.gn
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/components/autofill/content/browser/content_autofill_driver.cc
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/components/autofill/content/browser/content_autofill_driver.h
[add] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/components/autofill/content/browser/key_press_handler_manager.cc
[add] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/components/autofill/content/browser/key_press_handler_manager.h
[add] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/components/autofill/content/browser/key_press_handler_manager_unittest.cc
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/components/autofill/core/browser/autofill_external_delegate.cc
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/components/autofill/core/browser/autofill_external_delegate.h
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/components/autofill/core/browser/autofill_popup_delegate.h
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/components/password_manager/content/browser/content_password_manager_driver.cc
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/components/password_manager/content/browser/content_password_manager_driver.h
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/components/password_manager/core/browser/password_autofill_manager.cc
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/components/password_manager/core/browser/password_autofill_manager.h
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/components/password_manager/core/browser/password_manager_driver.h
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/components/password_manager/core/browser/stub_password_manager_driver.cc
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/components/password_manager/core/browser/stub_password_manager_driver.h
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/ios/chrome/browser/passwords/ios_chrome_password_manager_driver.h
[modify] https://crrev.com/6d61e0d262cc02641e963c1fc4fccd374555fad7/ios/chrome/browser/passwords/ios_chrome_password_manager_driver.mm

r462381 just landed (see #28) in M59. Locally I verified that it fixes the described issue (and part of the patch is also a browser test verifying the same).

The patch is unfortunately rather big (about 500 lines), but I will attempt a merge to M58 nevertheless, given the milestone of this bug. Please let me know if that's not needed.

And by "attempt a merge to M58" I mean I will request a merge to branch 3029 (M58) in about two days, after this bakes in Canary.

Requesting merge of r462381 to branch 3029 (M58).
The patch is in the current Canary. It is a fairly big change and not completely risk-free. Without it, site-isolation will result in broken UI in filling suggestions in frames.

Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Your change is approved for M58. Please verify the fix, if all looks good merge ASAP so that it will be picked up for next Beta Release.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c23dd84142131a6090f9032ddd0776af68e0dc6

commit 1c23dd84142131a6090f9032ddd0776af68e0dc6
Author: Vaclav Brozek <vabr@chromium.org>
Date: Fri Apr 07 21:23:54 2017

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}
(cherry picked from commit 6d61e0d262cc02641e963c1fc4fccd374555fad7)

Review-Url: https://codereview.chromium.org/2805173003 .
Cr-Commit-Position: refs/branch-heads/3029@{#633}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/chrome/browser/autofill/autofill_interactive_uitest.cc
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/chrome/browser/ui/autofill/autofill_popup_controller_impl.h
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/chrome/browser/ui/autofill/password_generation_popup_controller_impl.h
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/chrome/browser/ui/autofill/password_generation_popup_view_browsertest.cc
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/chrome/browser/ui/autofill/popup_controller_common.cc
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/chrome/browser/ui/autofill/popup_controller_common.h
[add] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/chrome/test/data/autofill/cross_origin_iframe.html
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/components/autofill/content/browser/BUILD.gn
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/components/autofill/content/browser/content_autofill_driver.cc
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/components/autofill/content/browser/content_autofill_driver.h
[add] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/components/autofill/content/browser/key_press_handler_manager.cc
[add] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/components/autofill/content/browser/key_press_handler_manager.h
[add] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/components/autofill/content/browser/key_press_handler_manager_unittest.cc
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/components/autofill/core/browser/autofill_external_delegate.cc
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/components/autofill/core/browser/autofill_external_delegate.h
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/components/autofill/core/browser/autofill_popup_delegate.h
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/components/password_manager/content/browser/content_password_manager_driver.cc
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/components/password_manager/content/browser/content_password_manager_driver.h
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/components/password_manager/core/browser/password_autofill_manager.cc
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/components/password_manager/core/browser/password_autofill_manager.h
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/components/password_manager/core/browser/password_manager_driver.h
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/components/password_manager/core/browser/stub_password_manager_driver.cc
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/components/password_manager/core/browser/stub_password_manager_driver.h
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/ios/chrome/browser/passwords/ios_chrome_password_manager_driver.h
[modify] https://crrev.com/1c23dd84142131a6090f9032ddd0776af68e0dc6/ios/chrome/browser/passwords/ios_chrome_password_manager_driver.mm

Verified the issue on windows 7 , mac os 10.12.2 , ubuntu 14.04 using chrome bet M58 #58.0.3029.68 and issue seems fixed.

Steps tried to verify the issue :

(1) Opened chrome with --site-per-process.
(2) Naviagted to  http://csreis.github.io/tests/cross-site-iframe-simple.html 
(3) Typed in the name (and have filled few details in  auto fill settings) some options appear.
(4) Moved arrow keys (down and up) to highlight different options and observed that the options on the dropdown list are highlighted and the input field automatically filled.

Attached screencast for reference.

Adding TE_Verified lables.

Thanks!

Deleted: 688848.mp4 651 KB

	688848.mp4 651 KB View Download