Android - Introduce "keyboard accessory" for Autofill suggestions.

chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[David Trainor- moved to gerrit, 2015/04/20 23:29:43]

2015?

[Evan Stade, 2015/04/21 00:48:51]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h"
+
+#include "base/android/jni_android.h"
+#include "base/android/jni_string.h"
+#include "chrome/browser/android/resource_mapper.h"
+#include "chrome/browser/ui/android/window_android_helper.h"
+#include "chrome/browser/ui/autofill/autofill_popup_controller.h"
+#include "components/autofill/core/browser/suggestion.h"
+#include "jni/AutofillKeyboardAccessoryBridge_jni.h"
+#include "ui/android/view_android.h"
+#include "ui/android/window_android.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/geometry/rect.h"
+
+namespace autofill {
+
+AutofillKeyboardAccessoryView::AutofillKeyboardAccessoryView(
+    AutofillPopupController* controller)
+    : controller_(controller) {
+}
+
+AutofillKeyboardAccessoryView::~AutofillKeyboardAccessoryView() {

[David Trainor- moved to gerrit, 2015/04/20 23:29:43]

Should we be clearing Java's native pointer of this object in Hide() (or here)? 
We don't want something trying to call back into this object when it's gone.  It
could happen if the handler's runnable runs after we destroy this object.

[Evan Stade, 2015/04/21 00:48:51]

I copied this pattern from AutofillPopupBridge/AutofillPopupViewAndroid. I
changed to create/init to avoid this problem.

[David Trainor- moved to gerrit, 2015/04/21 06:55:47]

I might be misunderstanding the lifecycle of this class.  Does this class get
killed/recreated during runtime or is it a singleton that's created only once? 
If it gets killed/recreated during runtime then I think my concern still
applies.  Just because this comes from an existing pattern doesn't make it safe.
 If the Java component lives around long enough to try to poke this native
object after it's destroyed it'll be a bug.  The easiest way to fix that is to
clear the native pointer in Java when this class goes away.

[Evan Stade, 2015/04/21 18:10:30]

No, but if it comes from a pattern that's existed for years without any crash
reports, that's an indication of something. Regardless, I have changed the way
it works so there's no handler.

I don't see the pattern you're talking about used in any of the other C++/java
jni class pairs, but maybe you can point me to one. AFAIU, java_object_
maintains a java reference to the bridge class. Nothing else holds a reference
to that class, so when this C++ object goes down the java side should also go
down.

[David Trainor- moved to gerrit, 2015/04/21 21:02:31]

I don't think that's a valid argument for proving that a piece of code is safe.
Sorry I wasn't very clear earlier :(.  We're actually talking about the same
pattern!  The issue is the complete pattern destroys the java -> native
connection when the native object dies.  The code you copied just didn't do this
(and should probably be fixed too!).  Here are some examples of classes that
follow this pattern and do this (I did a quick code search for upstream code and
grabbed a few):
ContentViewCore.java
WebContents.java
NavigationController.java
MediaDrmBridge.java
MediaPlayerBridge.java
AwContentsClientBridge.java

The issue is, if you don't destroy the Java -> native reference you're 100%
relying on nobody holding onto a Java reference when native dies.  This is
fragile - someone touching your AutofillKeyboardAccessory View and adding some
animation/handler might never think to check the delegate/bridge.  It's also
unsafe if you interact with the Android framework, like this code does.

As for this code in particular:
AutofillKeyboardAccessory holds a reference to AutofillKeyboardAccessoryBridge. 
The Android framework holds a reference to the AutofillKeyboardAccessory View. 
The Android framework can do whatever it wants.  In fact a lot of the click
handling gets posted on their own internal handlers... View.java is actually
unsafe in that regard (posts a task to notify the click listener).  AbsListView
looks somewhat safe for the current version of L (still can post a task, but
looks like it might check if the View was detached from the window), but might
be unsafe for older Android versions or custom OEM versions.

These are incredibly unlikely timing bugs, but just because it's unlikely
doesn't mean we should ignore it.  Especially when the fix is as simple as one
of:

1. Inside AutofillKeyboardAccessoryBridge#dismiss() we just clear our native
pointer and do != 0 checks everywhere we use it.
2. Add a AutofillKeyboardAccessoryBridge#clearNativePtr() that gets called from
the destructor of native AutofillKeyboardAccessoryView and do != 0 checks
everywhere we use the native pointer.

Sorry for the mix up on language around the "pattern."  Let me know if this make
sense!

[Evan Stade, 2015/04/23 01:21:09]

Done. Thanks for explanation.

+}
+
+void AutofillKeyboardAccessoryView::Show() {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  ui::ViewAndroid* view_android = controller_->container_view();
+  DCHECK(view_android);
+
+  java_object_.Reset(Java_AutofillKeyboardAccessoryBridge_create(
+      env, reinterpret_cast<intptr_t>(this),
+      view_android->GetWindowAndroid()->GetJavaObject().obj()));
+
+  UpdateBoundsAndRedrawPopup();
+}
+
+void AutofillKeyboardAccessoryView::Hide() {
+  controller_ = NULL;

[David Trainor- moved to gerrit, 2015/04/20 23:29:43]

nullptr

[Evan Stade, 2015/04/21 00:48:51]

Done.

+  JNIEnv* env = base::android::AttachCurrentThread();
+  Java_AutofillKeyboardAccessoryBridge_dismiss(env, java_object_.obj());
+}
+
+void AutofillKeyboardAccessoryView::UpdateBoundsAndRedrawPopup() {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  size_t count = controller_->GetLineCount();
+  ScopedJavaLocalRef<jobjectArray> data_array =
+      Java_AutofillKeyboardAccessoryBridge_createAutofillSuggestionArray(env,
+                                                                         count);
+
+  for (size_t i = 0; i < count; ++i) {
+    ScopedJavaLocalRef<jstring> value = base::android::ConvertUTF16ToJavaString(
+        env, controller_->GetElidedValueAt(i));
+    ScopedJavaLocalRef<jstring> label = base::android::ConvertUTF16ToJavaString(
+        env, controller_->GetElidedLabelAt(i));
+    int android_icon_id = 0;
+
+    const autofill::Suggestion& suggestion = controller_->GetSuggestionAt(i);
+    if (!suggestion.icon.empty()) {
+      android_icon_id = ResourceMapper::MapFromChromiumId(
+          controller_->GetIconResourceID(suggestion.icon));
+    }
+
+    Java_AutofillKeyboardAccessoryBridge_addToAutofillSuggestionArray(
+        env, data_array.obj(), i, value.obj(), label.obj(), android_icon_id,
+        suggestion.frontend_id);
+  }
+
+  Java_AutofillKeyboardAccessoryBridge_show(
+      env, java_object_.obj(), data_array.obj(), controller_->IsRTL());
+}
+
+void AutofillKeyboardAccessoryView::SuggestionSelected(JNIEnv* env,
+                                                       jobject obj,
+                                                       jint list_index) {
+  // Race: Hide() may have already run.
+  if (controller_)
+    controller_->AcceptSuggestion(list_index);
+}
+
+void AutofillKeyboardAccessoryView::ViewDismissed(JNIEnv* env, jobject obj) {
+  if (controller_)
+    controller_->ViewDestroyed();
+
+  delete this;
+}
+
+void AutofillKeyboardAccessoryView::InvalidateRow(size_t) {
+}
+
+// static
+bool AutofillKeyboardAccessoryView::RegisterAutofillKeyboardAccessoryView(
+    JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
+}  // namespace autofill