[aw] Allow to set Network in AwPacProcessor.

Bug: 1085115
Change-Id: I8846261a0f21ecb48692b70ab778f68996843760
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2414173Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Anna Malova <amalova@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812494}

android_webview/browser/aw_pac_processor.cc

@@ -108,9 +108,6 @@ proxy_resolver::ProxyResolverV8TracingFactory* GetProxyResolverFactory() {
 // blocking DNS queries, to get better performance.
 class HostResolver : public proxy_resolver::ProxyHostResolver {
  public:
-  HostResolver(net_handle_t net_handle) : net_handle_(net_handle) {}
-  ~HostResolver() override {}
-
   std::unique_ptr<proxy_resolver::ProxyHostResolver::Request> CreateRequest(
       const std::string& hostname,
       net::ProxyResolveDnsOperation operation,
@@ -119,9 +116,11 @@ class HostResolver : public proxy_resolver::ProxyHostResolver {
                                          link_addresses_);
   }
 
-  void SetNetworkLinkAddresses(
+  void SetNetworkAndLinkAddresses(
+      const net_handle_t net_handle,
       const std::vector<net::IPAddress>& link_addresses) {
     link_addresses_ = link_addresses;
+    net_handle_ = net_handle;
   }
 
  private:
@@ -169,7 +168,7 @@ class HostResolver : public proxy_resolver::ProxyHostResolver {
     bool MyIpAddressImpl() {
       // For network-aware queries the results are set from Java on
       // NetworkCallback#onLinkPropertiesChanged.
-      // See SetNetworkLinkAddresses.
+      // See SetNetworkAndLinkAddresses.
       if (IsNetworkSpecified()) {
         results_.push_back(link_addresses_.front());
         return true;
@@ -380,9 +379,8 @@ class MakeProxyRequestJob : public Job {
   std::unique_ptr<net::ProxyResolver::Request> request_;
 };
 
-AwPacProcessor::AwPacProcessor(net_handle_t net_handle)
-    : net_handle_(net_handle) {
-  host_resolver_ = std::make_unique<HostResolver>(net_handle_);
+AwPacProcessor::AwPacProcessor() {
+  host_resolver_ = std::make_unique<HostResolver>();
 }
 
 AwPacProcessor::~AwPacProcessor() {
@@ -467,30 +465,26 @@ ScopedJavaLocalRef<jstring> AwPacProcessor::MakeProxyRequest(
   return ConvertUTF8ToJavaString(env, MakeProxyRequest(url));
 }
 
-// ProxyResolverV8Tracing posts DNS resolution queries back to the thread
-// it is called from. Post update of link addresses to the same thread to
-// prevent concurrent access and modification of the same vector.
-void AwPacProcessor::SetNetworkLinkAddresses(
+void AwPacProcessor::SetNetworkAndLinkAddresses(
     JNIEnv* env,
+    net_handle_t net_handle,
     const base::android::JavaParamRef<jobjectArray>& jlink_addresses) {
   std::vector<std::string> string_link_addresses;
   base::android::AppendJavaStringArrayToStringVector(env, jlink_addresses,
                                                      &string_link_addresses);
-
   std::vector<net::IPAddress> link_addresses;
   for (std::string const& address : string_link_addresses) {
     link_addresses.push_back(StringToIPAddress(address));
   }
 
   GetTaskRunner()->PostTask(
-      FROM_HERE, base::BindOnce(&HostResolver::SetNetworkLinkAddresses,
+      FROM_HERE, base::BindOnce(&HostResolver::SetNetworkAndLinkAddresses,
                                 base::Unretained(host_resolver_.get()),
-                                std::move(link_addresses)));
+                                net_handle, std::move(link_addresses)));
 }
 
-static jlong JNI_AwPacProcessor_CreateNativePacProcessor(JNIEnv* env,
-                                                         jlong net_handle) {
-  AwPacProcessor* processor = new AwPacProcessor(net_handle);
+static jlong JNI_AwPacProcessor_CreateNativePacProcessor(JNIEnv* env) {
+  AwPacProcessor* processor = new AwPacProcessor();
   return reinterpret_cast<intptr_t>(processor);
 }
 

android_webview/browser/aw_pac_processor.h

@@ -22,7 +22,7 @@ class HostResolver;
 
 class AwPacProcessor {
  public:
-  AwPacProcessor(net_handle_t net_handle);
+  AwPacProcessor();
   AwPacProcessor(const AwPacProcessor&) = delete;
   AwPacProcessor& operator=(const AwPacProcessor&) = delete;
 
@@ -39,8 +39,9 @@ class AwPacProcessor {
       const base::android::JavaParamRef<jobject>& obj,
       const base::android::JavaParamRef<jstring>& jurl);
   std::string MakeProxyRequest(std::string url);
-  void SetNetworkLinkAddresses(
+  void SetNetworkAndLinkAddresses(
       JNIEnv* env,
+      net_handle_t net_handle,
       const base::android::JavaParamRef<jobjectArray>& addresses);
 
  private:
@@ -63,7 +64,6 @@ class AwPacProcessor {
   std::unique_ptr<HostResolver> host_resolver_;
 
   std::set<Job*> jobs_;
-  net_handle_t net_handle_;
 };
 }  // namespace android_webview
 

android_webview/browser/aw_pac_processor_unittest.cc

@@ -33,7 +33,7 @@ class AwPacProcessorTest : public testing::Test {
  protected:
   base::test::TaskEnvironment task_environment_{
            base::test::TaskEnvironment::TimeSource::MOCK_TIME};
-  AwPacProcessor* pac_processor_ = new AwPacProcessor(NETWORK_UNSPECIFIED);
+  AwPacProcessor* pac_processor_ = new AwPacProcessor();
 };
 
 TEST_F(AwPacProcessorTest, MakeProxyRequest) {
@@ -49,8 +49,7 @@ TEST_F(AwPacProcessorTest, MakeProxyRequestDnsResolve) {
 }
 
 TEST_F(AwPacProcessorTest, MultipleProxyRequest) {
-  AwPacProcessor* other_pac_processor_ =
-      new AwPacProcessor(NETWORK_UNSPECIFIED);
+  AwPacProcessor* other_pac_processor_ = new AwPacProcessor();
   pac_processor_->SetProxyScript(kScript);
   other_pac_processor_->SetProxyScript(kScriptDnsResolve);
 

android_webview/java/src/org/chromium/android_webview/AwPacProcessor.java

@@ -24,57 +24,62 @@ import org.chromium.base.annotations.NativeMethods;
 public class AwPacProcessor {
     private long mNativePacProcessor;
     private Network mNetwork;
+    private ConnectivityManager.NetworkCallback mNetworkCallback;
 
     public static final long NETWORK_UNSPECIFIED = 0;
 
     private static class LazyHolder {
-        static final AwPacProcessor sInstance = new AwPacProcessor(null);
+        static final AwPacProcessor sInstance = new AwPacProcessor();
     }
 
     public static AwPacProcessor getInstance() {
         return LazyHolder.sInstance;
     }
 
-    public AwPacProcessor(Network network) {
-        if (network == null) {
-            mNativePacProcessor =
-                    AwPacProcessorJni.get().createNativePacProcessor(NETWORK_UNSPECIFIED);
-            return;
-        }
-
-        mNetwork = network;
-        mNativePacProcessor =
-                AwPacProcessorJni.get().createNativePacProcessor(mNetwork.getNetworkHandle());
+    public AwPacProcessor() {
+        mNativePacProcessor = AwPacProcessorJni.get().createNativePacProcessor();
+        registerNetworkCallback();
+    }
 
+    private static ConnectivityManager getConnectivityManager() {
         Context context = ContextUtils.getApplicationContext();
-        ConnectivityManager connectivityManager =
-                (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE);
-        NetworkRequest.Builder builder = new NetworkRequest.Builder();
+        return (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE);
+    }
+
+    private void updateNetworkLinkAddress(Network network, LinkProperties linkProperties) {
+        if (network == null || linkProperties == null) {
+            setNetworkAndLinkAddresses(NETWORK_UNSPECIFIED, new String[0]);
+        } else {
+            String[] addresses = linkProperties.getLinkAddresses()
+                                         .stream()
+                                         .map(LinkAddress::toString)
+                                         .toArray(String[] ::new);
+            setNetworkAndLinkAddresses(network.getNetworkHandle(), addresses);
+        }
+    }
 
-        connectivityManager.registerNetworkCallback(
-                builder.build(), new ConnectivityManager.NetworkCallback() {
-                    @Override
-                    public void onLinkPropertiesChanged(
-                            Network network, LinkProperties linkProperties) {
-                        if (network.equals(mNetwork)) {
-                            updateNetworkLinkAddress(linkProperties);
-                        }
-                    }
-                });
-
-        updateNetworkLinkAddress(connectivityManager.getLinkProperties(mNetwork));
+    public void setNetworkAndLinkAddresses(long networkHandle, String[] addresses) {
+        AwPacProcessorJni.get().setNetworkAndLinkAddresses(
+                mNativePacProcessor, networkHandle, addresses);
     }
 
-    private void updateNetworkLinkAddress(LinkProperties linkProperties) {
-        String[] addresses = linkProperties.getLinkAddresses()
-                                     .stream()
-                                     .map(LinkAddress::toString)
-                                     .toArray(String[] ::new);
-        AwPacProcessorJni.get().setNetworkLinkAddresses(mNativePacProcessor, addresses);
+    private void registerNetworkCallback() {
+        mNetworkCallback = new ConnectivityManager.NetworkCallback() {
+            @Override
+            public void onLinkPropertiesChanged(Network network, LinkProperties linkProperties) {
+                if (network.equals(mNetwork)) {
+                    updateNetworkLinkAddress(network, linkProperties);
+                }
+            }
+        };
+        NetworkRequest.Builder builder = new NetworkRequest.Builder();
+
+        getConnectivityManager().registerNetworkCallback(builder.build(), mNetworkCallback);
     }
 
     // The calling code must not call any methods after it called destroy().
     public void destroy() {
+        getConnectivityManager().unregisterNetworkCallback(mNetworkCallback);
         AwPacProcessorJni.get().destroyNative(mNativePacProcessor, this);
     }
 
@@ -86,6 +91,11 @@ public class AwPacProcessor {
         return AwPacProcessorJni.get().makeProxyRequest(mNativePacProcessor, this, url);
     }
 
+    public void setNetwork(Network network) {
+        updateNetworkLinkAddress(network, getConnectivityManager().getLinkProperties(network));
+        mNetwork = network;
+    }
+
     public Network getNetwork() {
         return mNetwork;
     }
@@ -97,10 +107,11 @@ public class AwPacProcessor {
     @NativeMethods
     interface Natives {
         void initializeEnvironment();
-        long createNativePacProcessor(long netHandle);
+        long createNativePacProcessor();
         boolean setProxyScript(long nativeAwPacProcessor, AwPacProcessor caller, String script);
         String makeProxyRequest(long nativeAwPacProcessor, AwPacProcessor caller, String url);
         void destroyNative(long nativeAwPacProcessor, AwPacProcessor caller);
-        void setNetworkLinkAddresses(long nativeAwPacProcessor, String[] adresses);
+        void setNetworkAndLinkAddresses(
+                long nativeAwPacProcessor, long networkHandle, String[] adresses);
     }
 }

android_webview/javatests/src/org/chromium/android_webview/test/AwPacProcessorTest.java

+// Copyright 2020 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.android_webview.test;
+import androidx.test.filters.SmallTest;
+
+import com.android.webview.chromium.WebViewChromiumFactoryProvider;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.chromium.android_webview.AwPacProcessor;
+import org.chromium.base.JNIUtils;
+import org.chromium.base.library_loader.LibraryLoader;
+
+/**
+ * Tests for AwPacProcessor class.
+ */
+@RunWith(AwJUnit4ClassRunner.class)
+public class AwPacProcessorTest {
+    private AwPacProcessor mProcessor;
+
+    private final String mPacScript = "function FindProxyForURL(url, host) {\n"
+            + "var x = myIpAddress();"
+            + "\treturn \"PROXY \" + x + \":80\";\n"
+            + "}";
+    private final String mTestUrl = "http://testurl.test";
+
+    @Rule
+    public AwActivityTestRule mRule = new AwActivityTestRule();
+
+    @Before
+    public void setUp() {
+        JNIUtils.setClassLoader(WebViewChromiumFactoryProvider.class.getClassLoader());
+        LibraryLoader.getInstance().ensureInitialized();
+
+        mProcessor = AwPacProcessor.getInstance();
+    }
+
+    @Test
+    @SmallTest
+    public void testUpdateNetworkAndLinkAddresses() throws Throwable {
+        // PAC script returns result of myIpAddress call
+        mProcessor.setProxyScript(mPacScript);
+
+        // Save the proxy request result when network is not set
+        String proxyResultNetworkIsNotSet = mProcessor.makeProxyRequest(mTestUrl);
+
+        // Set network and IP addresses, check they are correctly propagated.
+        mProcessor.setNetworkAndLinkAddresses(42, new String[] {"1.2.3.4"});
+        String proxyResultNetworkIsSet = mProcessor.makeProxyRequest(mTestUrl);
+        Assert.assertEquals("PROXY 1.2.3.4:80", proxyResultNetworkIsSet);
+
+        // Unset network, the returned proxy string must be equal previously saved value
+        mProcessor.setNetwork(null);
+        Assert.assertEquals(proxyResultNetworkIsNotSet, mProcessor.makeProxyRequest(mTestUrl));
+    }
+}
\ No newline at end of file

android_webview/test/BUILD.gn

@@ -255,6 +255,7 @@ instrumentation_test_apk("webview_instrumentation_test_apk") {
     "../javatests/src/org/chromium/android_webview/test/AwLegacyQuirksTest.java",
     "../javatests/src/org/chromium/android_webview/test/AwMetricsIntegrationTest.java",
     "../javatests/src/org/chromium/android_webview/test/AwNetworkConfigurationTest.java",
+    "../javatests/src/org/chromium/android_webview/test/AwPacProcessorTest.java",
     "../javatests/src/org/chromium/android_webview/test/AwPageLoadMetricsTest.java",
     "../javatests/src/org/chromium/android_webview/test/AwPermissionManagerTest.java",
     "../javatests/src/org/chromium/android_webview/test/AwProxyControllerTest.java",