From b965bede643df7e08405443d92ee8b7e1fdb81ab Mon Sep 17 00:00:00 2001 From: Tomasz Wiszkowski Date: Fri, 2 Jun 2023 20:54:42 +0000 Subject: [PATCH] Strip not created actions from final array of actions passed to Java. This change guarantees actions not created do not take a slot in the final array of actions that gets passed to Java. The change utilizes `std::vector` to track the count of actions that do get passed, and suppresses passing control over to Java when - no actions are applicable to Android, or - actions are not ready to be passed (e.g. the control comes from an intent handler). The change replaces the former mechanism used to cache reference to jclass with an updated mechanism that better aligns with Clank JNI solutions. (cherry picked from commit aefab5d4be3cdf5cb3dd82ae6018435de52014d0) Bug: 1450106 Change-Id: I958ccbc57dd727edc3f7fd5acb8578edafb38fde Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4575458 Reviewed-by: Fred Mello Auto-Submit: Tomasz Wiszkowski Code-Coverage: Findit Commit-Queue: Tomasz Wiszkowski Cr-Original-Commit-Position: refs/heads/main@{#1151320} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4585081 Commit-Queue: Krishna Govind Reviewed-by: Krishna Govind Owners-Override: Krishna Govind Reviewed-by: Tomasz Wiszkowski Cr-Commit-Position: refs/branch-heads/5790@{#284} Cr-Branched-From: 1d71a337b1f6e707a13ae074dca1e2c34905eb9f-refs/heads/main@{#1148114} --- .../actions/omnibox_action_factory_android.cc | 49 +++++++++++-------- .../components/omnibox/AutocompleteMatch.java | 4 +- 2 files changed, 31 insertions(+), 22 deletions(-) diff --git a/components/omnibox/browser/actions/omnibox_action_factory_android.cc b/components/omnibox/browser/actions/omnibox_action_factory_android.cc index eda2a1cd73dd4..721dbb128593e 100644 --- a/components/omnibox/browser/actions/omnibox_action_factory_android.cc +++ b/components/omnibox/browser/actions/omnibox_action_factory_android.cc @@ -19,14 +19,9 @@ namespace { // base class. const char kOmniboxActionClass[] = "org/chromium/components/omnibox/action/OmniboxAction"; -JNI_REGISTRATION_EXPORT std::atomic - g_org_chromium_components_omnibox_action_OmniboxAction_clazz(nullptr); -inline jclass org_chromium_components_omnibox_action_OmniboxAction_clazz( - JNIEnv* env) { - return base::android::LazyGetClass( - env, kOmniboxActionClass, - &g_org_chromium_components_omnibox_action_OmniboxAction_clazz); -} + +base::LazyInstance>::DestructorAtExit + g_java_omnibox_action = LAZY_INSTANCE_INITIALIZER; base::LazyInstance>:: DestructorAtExit g_java_factory = LAZY_INSTANCE_INITIALIZER; @@ -35,7 +30,14 @@ base::LazyInstance>:: /* static */ void JNI_OmniboxActionFactory_SetFactory( JNIEnv* env, const base::android::JavaParamRef& factory) { - g_java_factory.Get().Reset(factory); + if (factory) { + g_java_omnibox_action.Get().Reset( + base::android::GetClass(env, kOmniboxActionClass)); + g_java_factory.Get().Reset(factory); + } else { + g_java_omnibox_action.Get().Reset(nullptr); + g_java_factory.Get().Reset(nullptr); + } } base::android::ScopedJavaGlobalRef BuildOmniboxPedal( @@ -76,20 +78,27 @@ base::android::ScopedJavaGlobalRef BuildOmniboxActionInSuggest( base::android::ScopedJavaLocalRef ToJavaOmniboxActionsList( JNIEnv* env, const std::vector>& actions) { - jclass clazz = - org_chromium_components_omnibox_action_OmniboxAction_clazz(env); - // Fires if OmniboxAction is not part of this build target. - DCHECK(clazz); - base::android::ScopedJavaLocalRef jactions( - env, env->NewObjectArray(actions.size(), clazz, nullptr)); - base::android::CheckException(env); + // Early return for cases where Action creation is not yet possible, e.g. + // if the control is passed from the IntentHandler. + if (!g_java_omnibox_action.IsCreated() || !g_java_factory.IsCreated()) { + return {}; + } - for (size_t index = 0; index < actions.size(); index++) { - auto jobj = actions[index]->GetOrCreateJavaObject(env); + std::vector> jactions_vec; + for (const auto& action : actions) { + auto jobj = action->GetOrCreateJavaObject(env); if (jobj) { - env->SetObjectArrayElement(jactions.obj(), index, jobj.obj()); + jactions_vec.emplace_back(std::move(jobj)); } } - return jactions; + // Return only after all actions are created to capture cases where some + // actions were found, but none was applicable to Android. + if (!jactions_vec.size()) { + return {}; + } + + return base::android::ToTypedJavaArrayOfObjects( + env, jactions_vec, + base::android::ScopedJavaLocalRef(g_java_omnibox_action.Get())); } diff --git a/components/omnibox/browser/android/java/src/org/chromium/components/omnibox/AutocompleteMatch.java b/components/omnibox/browser/android/java/src/org/chromium/components/omnibox/AutocompleteMatch.java index 6209726b3ae8a..83398b0662c7e 100644 --- a/components/omnibox/browser/android/java/src/org/chromium/components/omnibox/AutocompleteMatch.java +++ b/components/omnibox/browser/android/java/src/org/chromium/components/omnibox/AutocompleteMatch.java @@ -158,7 +158,7 @@ private static AutocompleteMatch build(long nativeObject, int nativeType, int[] GURL url, GURL imageUrl, String imageDominantColor, boolean isDeletable, String postContentType, byte[] postData, int groupId, List tiles, byte[] clipboardImageData, boolean hasTabMatch, String[] suggestTileTitles, - GURL[] suggestTileUrls, int[] suggestTileTypes, OmniboxAction[] actions) { + GURL[] suggestTileUrls, int[] suggestTileTypes, @Nullable OmniboxAction[] actions) { assert contentClassificationOffsets.length == contentClassificationStyles.length; List contentClassifications = new ArrayList<>(); for (int i = 0; i < contentClassificationOffsets.length; i++) { @@ -183,7 +183,7 @@ private static AutocompleteMatch build(long nativeObject, int nativeType, int[] relevance, transition, contents, contentClassifications, description, new ArrayList<>(), answer, fillIntoEdit, url, imageUrl, imageDominantColor, isDeletable, postContentType, postData, groupId, tiles, clipboardImageData, - hasTabMatch, suggestTiles, Arrays.asList(actions)); + hasTabMatch, suggestTiles, actions == null ? null : Arrays.asList(actions)); match.updateNativeObjectRef(nativeObject); match.setDescription( description, descriptionClassificationOffsets, descriptionClassificationStyles);