Skip to content

Commit

Permalink
Fixing GetSelectedSegment crash for start surface
Browse files Browse the repository at this point in the history
This CL attempts to fix the crash in GetSelectedSegment by using the
exact same cached param for building segmentation platform config as the
Java layer. Earlier there was a crash on FRE because of difference in
values between
(1) Cached Java feature flag - called before caching finch flags, hence
default value
(2) Cached field trial param - called after caching finch flags, hence
non-default value
After this CL, we will always use the field trial param only, which
should be consistent during chrome session.

(cherry picked from commit 054c8f5)

Bug: 1276124
Change-Id: Iedac74b330749e1dc35f61386a75c05f0435a578
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3330725
Reviewed-by: Henrique Nakashima <hnakashima@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Commit-Queue: Shakti Sahu <shaktisahu@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#950772}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3335098
Commit-Queue: Krishna Govind <govind@chromium.org>
Cr-Commit-Position: refs/branch-heads/4758@{#19}
Cr-Branched-From: 4a2cf4b-refs/heads/main@{#950365}
  • Loading branch information
Shakti Sahu authored and Chromium LUCI CQ committed Dec 13, 2021
1 parent 1d81009 commit 8e1d59b
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import org.chromium.base.Log;
import org.chromium.base.SysUtils;
import org.chromium.base.annotations.CalledByNative;
import org.chromium.base.annotations.NativeMethods;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.chrome.browser.flags.BooleanCachedFieldTrialParameter;
Expand Down Expand Up @@ -289,6 +290,11 @@ public static boolean shouldShowAnimationsForFinale() {
return HOME_BUTTON_ON_GRID_TAB_SWITCHER.getValue() && FINALE_ANIMATION_ENABLED.getValue();
}

@CalledByNative
private static boolean isBehaviouralTargetingEnabled() {
return !TextUtils.isEmpty(BEHAVIOURAL_TARGETING.getValue());
}

@VisibleForTesting
static void setFeedVisibilityForTesting(boolean isVisible) {
SharedPreferencesManager.getInstance().writeBoolean(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#if defined(OS_ANDROID)
#include "chrome/browser/flags/android/cached_feature_flags.h"
#include "chrome/browser/flags/android/chrome_feature_list.h"
#include "chrome/browser/ui/android/start_surface/start_surface_android.h"
#endif

using optimization_guide::proto::OptimizationTarget;
Expand Down Expand Up @@ -120,8 +121,7 @@ std::vector<std::unique_ptr<Config>> GetSegmentationPlatformConfig() {
chrome::android::kAdaptiveButtonInTopToolbarCustomizationV2)) {
configs.emplace_back(GetConfigForAdaptiveToolbar());
}
if (chrome::android::IsJavaDrivenFeatureEnabled(
chrome::android::kStartSurfaceAndroid)) {
if (IsStartSurfaceBehaviouralTargetingEnabled()) {
configs.emplace_back(GetConfigForChromeStartAndroid());
}
if (base::FeatureList::IsEnabled(
Expand Down
1 change: 1 addition & 0 deletions chrome/browser/ui/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -814,6 +814,7 @@ static_library("ui") {
"android/simple_message_box_android.cc",
"android/ssl_client_certificate_selector.cc",
"android/start_surface/start_surface_android.cc",
"android/start_surface/start_surface_android.h",
"android/status_tray_android.cc",
"android/tab_contents/chrome_web_contents_view_delegate_android.cc",
"android/tab_contents/chrome_web_contents_view_delegate_android.h",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "chrome/browser/flags/android/chrome_feature_list.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_android.h"
#include "chrome/browser/ui/android/start_surface/start_surface_android.h"
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/render_process_host.h"
Expand All @@ -29,6 +30,11 @@ void WarmUpRenderProcess(Profile* profile) {

} // namespace

bool IsStartSurfaceBehaviouralTargetingEnabled() {
JNIEnv* env = base::android::AttachCurrentThread();
return Java_StartSurfaceConfiguration_isBehaviouralTargetingEnabled(env);
}

static void JNI_StartSurfaceConfiguration_WarmupRenderer(
JNIEnv* env,
const JavaParamRef<jobject>& jprofile) {
Expand Down
11 changes: 11 additions & 0 deletions chrome/browser/ui/android/start_surface/start_surface_android.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright 2021 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef CHROME_BROWSER_UI_ANDROID_START_SURFACE_START_SURFACE_ANDROID_H_
#define CHROME_BROWSER_UI_ANDROID_START_SURFACE_START_SURFACE_ANDROID_H_

// Returns whether behavioural targeting is enabled in the variation params.
bool IsStartSurfaceBehaviouralTargetingEnabled();

#endif // CHROME_BROWSER_UI_ANDROID_START_SURFACE_START_SURFACE_ANDROID_H_

0 comments on commit 8e1d59b

Please sign in to comment.