Fix autofill popup controller key press callback registration

chrome/browser/ui/autofill/popup_controller_common.cc

Index: chrome/browser/ui/autofill/popup_controller_common.cc
diff --git a/chrome/browser/ui/autofill/popup_controller_common.cc b/chrome/browser/ui/autofill/popup_controller_common.cc
index 2c97b8b7c222c6fb58f61eebf43f28da21da1208..5c54f8418673813a1f5377db3f08107fdd5dced3 100644
--- a/chrome/browser/ui/autofill/popup_controller_common.cc
+++ b/chrome/browser/ui/autofill/popup_controller_common.cc
@@ -4,11 +4,30 @@
 
 #include "chrome/browser/ui/autofill/popup_controller_common.h"
 
-#include "content/public/browser/render_view_host.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
 
 namespace autofill {
 
+namespace {
+
+// Returns the RenderWidgetHost which should be currently consuming keyboard
+// events in |web_contents|. Can return null if there is no such
+// RenderWidgetHost.
+content::RenderWidgetHost* KeyPressHandlingTarget(
+    content::WebContents* web_contents) {
+  content::RenderFrameHost* frame = web_contents->GetFocusedFrame();
+  if (!frame)
+    frame = web_contents->GetMainFrame();
+  content::RenderWidgetHostView* view = frame->GetView();
+  if (!view)
+    return nullptr;
+  return view->GetRenderWidgetHost();
+}
+
+}  // namespace
+
 PopupControllerCommon::PopupControllerCommon(
     const gfx::RectF& element_bounds,
     base::i18n::TextDirection text_direction,
@@ -30,18 +49,17 @@ void PopupControllerCommon::SetKeyPressCallback(
 
 void PopupControllerCommon::RegisterKeyPressCallback() {
   if (web_contents_ && !key_press_event_target_) {
-    key_press_event_target_ = web_contents_->GetRenderViewHost();
-    key_press_event_target_->GetWidget()->AddKeyPressEventCallback(
+    key_press_event_target_ = KeyPressHandlingTarget(web_contents_);

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

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?).

+    key_press_event_target_->AddKeyPressEventCallback(
         key_press_event_callback_);
   }
 }
 
 void PopupControllerCommon::RemoveKeyPressCallback() {
   if (web_contents_ && (!web_contents_->IsBeingDestroyed()) &&
-      key_press_event_target_ == web_contents_->GetRenderViewHost()) {
-    web_contents_->GetRenderViewHost()
-        ->GetWidget()
-        ->RemoveKeyPressEventCallback(key_press_event_callback_);
+      key_press_event_target_ == KeyPressHandlingTarget(web_contents_)) {
+    key_press_event_target_->RemoveKeyPressEventCallback(

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

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]

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.

+        key_press_event_callback_);
   }
   key_press_event_target_ = NULL;
 }