Remove AndroidTelephonyBridgeManager.

This class required location permissions but was only used by the
AvailabilityProber, which can simply rely on getNetworkOperator, which
doesn't require location permissions.

Bug: 1266018
Change-Id: Id2bde5bac607557a777789aac2e8b362b09628ed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3257115
Reviewed-by: Sophie Chang <sophiechang@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Robert Ogden <robertogden@chromium.org>
Reviewed-by: Michael Bai <michaelbai@chromium.org>
Commit-Queue: Simon Pelchat <spelchat@chromium.org>
Cr-Commit-Position: refs/heads/main@{#938537}

chrome/browser/availability/availability_prober.cc

@@ -54,6 +54,8 @@ const char kAttemptsBeforeSuccessHistogram[] =
 const char kHttpRespCodeHistogram[] = "Availability.Prober.ResponseCode";
 const char kNetErrorHistogram[] = "Availability.Prober.NetError";
 const char kCacheEntryAgeHistogram[] = "Availability.Prober.CacheEntryAge";
+const char kGenerateCacheKeyHistogram[] =
+    "Availability.Prober.GenerateCacheKey";
 
 // Please keep this up to date with logged histogram suffix
 // |Availability.Prober.Clients| in tools/metrics/histograms/histograms.xml.
@@ -707,9 +709,13 @@ void AvailabilityProber::RunCallback(bool success) {
 
 std::string AvailabilityProber::GetCacheKeyForCurrentNetwork() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return base::StringPrintf(
+  base::Time start(clock_->Now());
+  std::string key = base::StringPrintf(
       "%s;%s:%d", GenerateNetworkID(network_connection_tracker_).c_str(),
       url_.host().c_str(), url_.EffectiveIntPort());
+  UmaHistogramTimes(AppendNameToHistogram(kGenerateCacheKeyHistogram),
+                    clock_->Now() - start);
+  return key;
 }
 
 std::string AvailabilityProber::AppendNameToHistogram(

chrome/browser/availability/availability_prober_unittest.cc

@@ -990,3 +990,12 @@ TEST_F(AvailabilityProberTest, ReportExternalFailure_WhileActive) {
       "IsolatedPrerenderOriginCheck",
       true, 1);
 }
+
+TEST_F(AvailabilityProberTest, GenerateCacheEntryHistogram) {
+  base::HistogramTester histogram_tester;
+  std::unique_ptr<AvailabilityProber> prober = NewProber();
+  EXPECT_EQ(prober->LastProbeWasSuccessful(), absl::nullopt);
+
+  histogram_tester.ExpectTotalCount(
+      "Availability.Prober.GenerateCacheKey.IsolatedPrerenderOriginCheck", 1);
+}

net/android/BUILD.gn

@@ -10,7 +10,6 @@ android_library("net_java") {
     "java/src/org/chromium/net/AndroidCertVerifyResult.java",
     "java/src/org/chromium/net/AndroidKeyStore.java",
     "java/src/org/chromium/net/AndroidNetworkLibrary.java",
-    "java/src/org/chromium/net/AndroidTelephonyManagerBridge.java",
     "java/src/org/chromium/net/AndroidTrafficStats.java",
     "java/src/org/chromium/net/DnsStatus.java",
     "java/src/org/chromium/net/GURLUtils.java",

net/android/java/src/org/chromium/net/AndroidNetworkLibrary.java

@@ -23,6 +23,7 @@ import android.os.Build;
 import android.os.Build.VERSION_CODES;
 import android.os.ParcelFileDescriptor;
 import android.os.Process;
+import android.telephony.TelephonyManager;
 import android.util.Log;
 
 import androidx.annotation.VisibleForTesting;
@@ -150,20 +151,16 @@ class AndroidNetworkLibrary {
 
     /**
      * Returns the MCC+MNC (mobile country code + mobile network code) as
-     * the numeric name of the current registered operator.
+     * the numeric name of the current registered operator. This function
+     * potentially blocks the thread, so use with care.
      */
     @CalledByNative
     private static String getNetworkOperator() {
-        return AndroidTelephonyManagerBridge.getInstance().getNetworkOperator();
-    }
-
-    /**
-     * Returns the MCC+MNC (mobile country code + mobile network code) as
-     * the numeric name of the current SIM operator.
-     */
-    @CalledByNative
-    private static String getSimOperator() {
-        return AndroidTelephonyManagerBridge.getInstance().getSimOperator();
+        TelephonyManager telephonyManager =
+                (TelephonyManager) ContextUtils.getApplicationContext().getSystemService(
+                        Context.TELEPHONY_SERVICE);
+        if (telephonyManager == null) return "";
+        return telephonyManager.getNetworkOperator();
     }
 
     /**

net/android/java/src/org/chromium/net/AndroidTelephonyManagerBridge.java

@@ -1,148 +0,0 @@
-// Copyright 2019 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.
-
-package org.chromium.net;
-
-import android.content.Context;
-import android.telephony.PhoneStateListener;
-import android.telephony.ServiceState;
-import android.telephony.TelephonyManager;
-
-import androidx.annotation.AnyThread;
-import androidx.annotation.MainThread;
-
-import org.chromium.base.ContextUtils;
-import org.chromium.base.ThreadUtils;
-import org.chromium.base.annotations.MainDex;
-
-import javax.annotation.CheckForNull;
-
-/**
- * This class subscribes to {@link TelephonyManager} events and stores
- * some useful values.
- */
-@AnyThread
-@MainDex
-public class AndroidTelephonyManagerBridge {
-    private static volatile AndroidTelephonyManagerBridge sInstance;
-
-    @CheckForNull
-    private volatile String mNetworkCountryIso;
-    @CheckForNull
-    private volatile String mNetworkOperator;
-    @CheckForNull
-    private volatile String mSimOperator;
-
-    private Listener mListener;
-
-    private AndroidTelephonyManagerBridge() {}
-
-    /**
-     * Returns the ISO country code equivalent of the current MCC.
-     */
-    public String getNetworkCountryIso() {
-        if (mNetworkCountryIso == null) {
-            TelephonyManager telephonyManager = getTelephonyManager();
-            if (telephonyManager == null) {
-                return "";
-            }
-            mNetworkCountryIso = telephonyManager.getNetworkCountryIso();
-        }
-        return mNetworkCountryIso;
-    }
-
-    /**
-     * Returns the MCC+MNC (mobile country code + mobile network code) as
-     * the numeric name of the current registered operator.
-     */
-    public String getNetworkOperator() {
-        if (mNetworkOperator == null) {
-            TelephonyManager telephonyManager = getTelephonyManager();
-            if (telephonyManager == null) {
-                return "";
-            }
-            mNetworkOperator = telephonyManager.getNetworkOperator();
-        }
-        return mNetworkOperator;
-    }
-
-    /**
-     * Returns the MCC+MNC (mobile country code + mobile network code) as
-     * the numeric name of the current SIM operator.
-     */
-    public String getSimOperator() {
-        if (mSimOperator == null) {
-            TelephonyManager telephonyManager = getTelephonyManager();
-            if (telephonyManager == null) {
-                return "";
-            }
-            mSimOperator = telephonyManager.getSimOperator();
-        }
-        return mSimOperator;
-    }
-
-    private void update(@CheckForNull TelephonyManager telephonyManager) {
-        if (telephonyManager == null) {
-            return;
-        }
-        mNetworkCountryIso = telephonyManager.getNetworkCountryIso();
-        mNetworkOperator = telephonyManager.getNetworkOperator();
-        mSimOperator = telephonyManager.getSimOperator();
-    }
-
-    @MainThread
-    private void listenTelephonyServiceState(TelephonyManager telephonyManager) {
-        ThreadUtils.assertOnUiThread();
-
-        mListener = new Listener();
-        telephonyManager.listen(mListener, PhoneStateListener.LISTEN_SERVICE_STATE);
-    }
-
-    @CheckForNull
-    private static TelephonyManager getTelephonyManager() {
-        return (TelephonyManager) ContextUtils.getApplicationContext().getSystemService(
-                Context.TELEPHONY_SERVICE);
-    }
-
-    private static AndroidTelephonyManagerBridge create() {
-        AndroidTelephonyManagerBridge instance = new AndroidTelephonyManagerBridge();
-        ThreadUtils.runOnUiThread(() -> {
-            TelephonyManager telephonyManager = getTelephonyManager();
-            if (telephonyManager != null) {
-                instance.listenTelephonyServiceState(telephonyManager);
-            }
-        });
-        return instance;
-    }
-
-    /**
-     * Returns {@link AndroidTelephonyManagerBridge} instance.
-     */
-    public static AndroidTelephonyManagerBridge getInstance() {
-        AndroidTelephonyManagerBridge instance = sInstance;
-        if (instance == null) {
-            synchronized (AndroidTelephonyManagerBridge.class) {
-                instance = sInstance;
-                if (instance == null) {
-                    instance = create();
-                    sInstance = instance;
-                }
-            }
-        }
-        return instance;
-    }
-
-    private class Listener extends PhoneStateListener {
-        @CheckForNull
-        private ServiceState mServiceState;
-
-        @Override
-        public void onServiceStateChanged(ServiceState serviceState) {
-            if (mServiceState == null || !mServiceState.equals(serviceState)) {
-                mServiceState = serviceState;
-                update(getTelephonyManager());
-            }
-        }
-    }
-}

net/android/network_library.cc

@@ -101,12 +101,6 @@ std::string GetTelephonyNetworkOperator() {
           base::android::AttachCurrentThread()));
 }
 
-std::string GetTelephonySimOperator() {
-  return base::android::ConvertJavaStringToUTF8(
-      Java_AndroidNetworkLibrary_getSimOperator(
-          base::android::AttachCurrentThread()));
-}
-
 bool GetIsRoaming() {
   return Java_AndroidNetworkLibrary_getIsRoaming(
       base::android::AttachCurrentThread());

net/android/network_library.h

@@ -57,13 +57,10 @@ bool GetMimeTypeFromExtension(const std::string& extension,
                               std::string* result);
 
 // Returns MCC+MNC (mobile country code + mobile network code) as
-// the numeric name of the current registered operator.
+// the numeric name of the current registered operator. This function
+// potentially blocks the thread, so use with care.
 NET_EXPORT std::string GetTelephonyNetworkOperator();
 
-// Returns MCC+MNC (mobile country code + mobile network code) as
-// the numeric name of the current SIM operator.
-NET_EXPORT std::string GetTelephonySimOperator();
-
 // Returns true if the device is roaming on the currently active network. When
 // true, it suggests that use of data may incur extra costs.
 NET_EXPORT bool GetIsRoaming();

tools/metrics/histograms/metadata/histogram_suffixes_list.xml

@@ -1264,38 +1264,6 @@ chromium-metrics-reviews@google.com.
   <affected-histogram name="Autofill.WalletCards2"/>
 </histogram_suffixes>
 
-<histogram_suffixes name="Availability_Prober_Clients" separator=".">
-  <suffix name="IsolatedPrerenderCanaryCheck"
-      label="(M85 only) Canary check for Isolated Prerenders probing"/>
-  <suffix name="IsolatedPrerenderDNSCanaryCheck"
-      label="DNS canary check for Isolated Prerenders probing"/>
-  <suffix name="IsolatedPrerenderOriginCheck"
-      label="Origin check for Isolated Prerenders"/>
-  <suffix name="IsolatedPrerenderTLSCanaryCheck"
-      label="TLS canary check for Isolated Prerenders probing"/>
-  <suffix name="Litepages" label="Lite page HTTPS Server Previews">
-    <obsolete>
-      Removed in M84.
-    </obsolete>
-  </suffix>
-  <suffix name="LitepagesOriginCheck"
-      label="Origin check for Litepage previews">
-    <obsolete>
-      Removed in M84.
-    </obsolete>
-  </suffix>
-  <affected-histogram name="Availability.Prober.CacheEntryAge"/>
-  <affected-histogram name="Availability.Prober.DidSucceed"/>
-  <affected-histogram
-      name="Availability.Prober.DidSucceed.AfterReportedFailure"/>
-  <affected-histogram name="Availability.Prober.FinalState"/>
-  <affected-histogram name="Availability.Prober.NetError"/>
-  <affected-histogram name="Availability.Prober.NumAttemptsBeforeSuccess"/>
-  <affected-histogram name="Availability.Prober.ResponseCode"/>
-  <affected-histogram name="Availability.Prober.TimeUntilFailure2"/>
-  <affected-histogram name="Availability.Prober.TimeUntilSuccess2"/>
-</histogram_suffixes>
-
 <histogram_suffixes name="BackgroundDownload" separator=".">
   <suffix name="BackgroundDownload"
       label="Download that started in background."/>

tools/metrics/histograms/metadata/others/histograms.xml

@@ -22,6 +22,28 @@ chromium-metrics-reviews@google.com.
 
 <histograms>
 
+<variants name="AvailabilityProberClient">
+  <variant name="IsolatedPrerenderCanaryCheck"
+      summary="(M85 only) Canary check for Isolated Prerenders probing"/>
+  <variant name="IsolatedPrerenderDNSCanaryCheck"
+      summary="DNS canary check for Isolated Prerenders probing"/>
+  <variant name="IsolatedPrerenderOriginCheck"
+      summary="Origin check for Isolated Prerenders"/>
+  <variant name="IsolatedPrerenderTLSCanaryCheck"
+      summary="TLS canary check for Isolated Prerenders probing"/>
+  <variant name="Litepages" summary="Lite page HTTPS Server Previews">
+    <obsolete>
+      Removed in M84.
+    </obsolete>
+  </variant>
+  <variant name="LitepagesOriginCheck"
+      summary="Origin check for Litepage previews">
+    <obsolete>
+      Removed in M84.
+    </obsolete>
+  </variant>
+</variants>
+
 <variants name="ProcessName">
   <variant name="BrowserProcess" summary=""/>
   <variant name="GPUProcess" summary=""/>
@@ -1391,26 +1413,19 @@ chromium-metrics-reviews@google.com.
   </summary>
 </histogram>
 
-<histogram base="true" name="Availability.Prober.CacheEntryAge" units="hours"
-    expires_after="M95">
+<histogram base="true"
+    name="Availability.Prober.CacheEntryAge.{AvailabilityProberClient}"
+    units="hours" expires_after="M95">
   <owner>robertogden@chromium.org</owner>
   <owner>tbansal@chromium.org</owner>
   <summary>
     How old a cached probe result was when it was used, in hours.
   </summary>
-</histogram>
-
-<histogram base="true" name="Availability.Prober.DidSucceed"
-    enum="BooleanSuccess" expires_after="M95">
-  <owner>robertogden@chromium.org</owner>
-  <owner>tbansal@chromium.org</owner>
-  <summary>
-    Records the completion status of a probe when it completes each attempt.
-  </summary>
+  <token key="AvailabilityProberClient" variants="AvailabilityProberClient"/>
 </histogram>
 
 <histogram base="true"
-    name="Availability.Prober.DidSucceed.AfterReportedFailure"
+    name="Availability.Prober.DidSucceed.AfterReportedFailure.{AvailabilityProberClient}"
     enum="BooleanSuccess" expires_after="M95">
   <owner>robertogden@chromium.org</owner>
   <owner>tbansal@chromium.org</owner>
@@ -1418,9 +1433,22 @@ chromium-metrics-reviews@google.com.
     Records the completion status of a probe retry after an externally reported
     failure.
   </summary>
+  <token key="AvailabilityProberClient" variants="AvailabilityProberClient"/>
 </histogram>
 
-<histogram base="true" name="Availability.Prober.FinalState"
+<histogram base="true"
+    name="Availability.Prober.DidSucceed.{AvailabilityProberClient}"
+    enum="BooleanSuccess" expires_after="M95">
+  <owner>robertogden@chromium.org</owner>
+  <owner>tbansal@chromium.org</owner>
+  <summary>
+    Records the completion status of a probe when it completes each attempt.
+  </summary>
+  <token key="AvailabilityProberClient" variants="AvailabilityProberClient"/>
+</histogram>
+
+<histogram base="true"
+    name="Availability.Prober.FinalState.{AvailabilityProberClient}"
     enum="BooleanSuccess" expires_after="M95">
   <owner>robertogden@chromium.org</owner>
   <owner>tbansal@chromium.org</owner>
@@ -1429,10 +1457,25 @@ chromium-metrics-reviews@google.com.
     when the prober succeeds, fails and has no more retries, or the delegate
     stops probing.
   </summary>
+  <token key="AvailabilityProberClient" variants="AvailabilityProberClient"/>
 </histogram>
 
-<histogram base="true" name="Availability.Prober.NetError" enum="NetErrorCodes"
-    expires_after="M95">
+<histogram base="true"
+    name="Availability.Prober.GenerateCacheKey.{AvailabilityProberClient}"
+    units="ms" expires_after="M99">
+  <owner>spelchat@chromium.org</owner>
+  <owner>curranmax@chromium.org</owner>
+  <summary>
+    Records the amount of time spent generating a cache key for the availability
+    prober. In particular, this tracks how much getNetworkOperator calls might
+    be slowing down this method. See crbug.com/1266018 for more details.
+  </summary>
+  <token key="AvailabilityProberClient" variants="AvailabilityProberClient"/>
+</histogram>
+
+<histogram base="true"
+    name="Availability.Prober.NetError.{AvailabilityProberClient}"
+    enum="NetErrorCodes" expires_after="M95">
   <owner>robertogden@chromium.org</owner>
   <owner>tbansal@chromium.org</owner>
   <summary>
@@ -1440,9 +1483,11 @@ chromium-metrics-reviews@google.com.
     response does not occur within the probe's TTL, when a sample will also be
     added to the ERR_TIMED_OUT bucket.
   </summary>
+  <token key="AvailabilityProberClient" variants="AvailabilityProberClient"/>
 </histogram>
 
-<histogram base="true" name="Availability.Prober.NumAttemptsBeforeSuccess"
+<histogram base="true"
+    name="Availability.Prober.NumAttemptsBeforeSuccess.{AvailabilityProberClient}"
     units="count" expires_after="M95">
   <owner>robertogden@chromium.org</owner>
   <owner>tbansal@chromium.org</owner>
@@ -1451,9 +1496,11 @@ chromium-metrics-reviews@google.com.
     result. Only recorded on success. This metric is 1-based so if a probe
     succeeds the first time, a sample of 1 will be recorded.
   </summary>
+  <token key="AvailabilityProberClient" variants="AvailabilityProberClient"/>
 </histogram>
 
-<histogram base="true" name="Availability.Prober.ResponseCode"
+<histogram base="true"
+    name="Availability.Prober.ResponseCode.{AvailabilityProberClient}"
     enum="HttpResponseCode" expires_after="M95">
   <owner>robertogden@chromium.org</owner>
   <owner>tbansal@chromium.org</owner>
@@ -1461,26 +1508,31 @@ chromium-metrics-reviews@google.com.
     Records the HTTP response code of a completed probe, when a HTTP response is
     received.
   </summary>
+  <token key="AvailabilityProberClient" variants="AvailabilityProberClient"/>
 </histogram>
 
-<histogram base="true" name="Availability.Prober.TimeUntilFailure2" units="ms"
-    expires_after="M95">
+<histogram base="true"
+    name="Availability.Prober.TimeUntilFailure2.{AvailabilityProberClient}"
+    units="ms" expires_after="M95">
   <owner>robertogden@chromium.org</owner>
   <owner>tbansal@chromium.org</owner>
   <summary>
     Records the amount of time spent working on a single probe attempt to get to
     a failed state. Recorded every time a probe fails.
   </summary>
+  <token key="AvailabilityProberClient" variants="AvailabilityProberClient"/>
 </histogram>
 
-<histogram base="true" name="Availability.Prober.TimeUntilSuccess2" units="ms"
-    expires_after="M95">
+<histogram base="true"
+    name="Availability.Prober.TimeUntilSuccess2.{AvailabilityProberClient}"
+    units="ms" expires_after="M95">
   <owner>robertogden@chromium.org</owner>
   <owner>tbansal@chromium.org</owner>
   <summary>
     Records the amount of time spent working on a single probe attempt to get to
     a successful state. Recorded every time a probe succeeds.
   </summary>
+  <token key="AvailabilityProberClient" variants="AvailabilityProberClient"/>
 </histogram>
 
 <histogram name="Badging.AppBadgeUpdate.Mac.Result"