Reland "Add onPolicyUpdate notification in Java."

This reverts commit 4d2dee85.

Reason for revert: Create the reland to debug


Original change's description:
> Revert "Resolve the issue by moving PolicyMapAndroid instances in the
test so that they can live until the end of tests."
>
> This reverts commit 6b3c4bdb.
>
> Reason for revert: Broke PolicyServiceAndroidTest.PolicyUpdateEvent on Android ASAN
> https://ci.chromium.org/p/chromium/builders/ci/android-asan/8133
>
> Original change's description:
> > Add onPolicyUpdate notification in Java.
> >
> > Support PolicyService::onPolicyUpdate event in Java. Same as other
> > functions, only Chrome policies are supported.
> >
> > Other changes:
> > 1) Implement PolicyMap.equals()
> > 2) Use ObserverList instead of Set to manage PolicyService's observers.
> >
> > Bug: 1127469
> > Change-Id: I40c6958e87c746aa40afec306ee632bba52183f6
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2446639
> > Commit-Queue: Owen Min <zmin@chromium.org>
> > Reviewed-by: Sky Malice <skym@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#814310}
>
> TBR=zmin@chromium.org,skym@chromium.org
>
> Change-Id: Ibde93e22e01f4d3eb8d9eb2a8d4d9c42166ed191
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 1127469
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2454653
> Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
> Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#814628}

TBR=vasilii@chromium.org,zmin@chromium.org,skym@chromium.org

# Not skipping CQ checks because this is a reland.

Bug: 1127469
Change-Id: I778a68262809758d9dd5e43bc3ad5accbc1d823c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2458767Reviewed-by: Owen Min <zmin@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Commit-Queue: Owen Min <zmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815228}

chrome/android/java/src/org/chromium/chrome/browser/firstrun/TosAndUmaFirstRunFragmentWithEnterpriseSupport.java

@@ -236,10 +236,13 @@ public class TosAndUmaFirstRunFragmentWithEnterpriseSupport
         if (policyService.isInitializationComplete()) {
             updateCctTosPolicy();
         } else {
-            mPolicyServiceObserver = () -> {
-                policyService.removeObserver(mPolicyServiceObserver);
-                mPolicyServiceObserver = null;
-                updateCctTosPolicy();
+            mPolicyServiceObserver = new PolicyService.Observer() {
+                @Override
+                public void onPolicyServiceInitialized() {
+                    policyService.removeObserver(mPolicyServiceObserver);
+                    mPolicyServiceObserver = null;
+                    updateCctTosPolicy();
+                }
             };
             policyService.addObserver(mPolicyServiceObserver);
         }

components/policy/android/java/src/org/chromium/components/policy/PolicyMap.java

@@ -66,6 +66,11 @@ public class PolicyMap {
         return PolicyMapJni.get().getDictValue(mNativePolicyMap, PolicyMap.this, policy);
     }
 
+    public boolean isEqual(PolicyMap other) {
+        if (this == other) return true;
+        return PolicyMapJni.get().equals(mNativePolicyMap, PolicyMap.this, other.mNativePolicyMap);
+    }
+
     @CalledByNative
     private PolicyMap(long nativePolicyMap) {
         mNativePolicyMap = nativePolicyMap;
@@ -85,5 +90,7 @@ public class PolicyMap {
         String getListValue(long nativePolicyMap, PolicyMap caller, String policy);
         @NativeClassQualifiedName("PolicyMapAndroid")
         String getDictValue(long nativePolicyMap, PolicyMap caller, String policy);
+        @NativeClassQualifiedName("PolicyMapAndroid")
+        boolean equals(long nativePolicyMap, PolicyMap caller, long nativeOtherPolicyMap);
     }
 }

components/policy/android/java/src/org/chromium/components/policy/PolicyService.java

@@ -4,15 +4,12 @@
 
 package org.chromium.components.policy;
 
-import org.chromium.base.CollectionUtil;
+import org.chromium.base.ObserverList;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.base.annotations.NativeClassQualifiedName;
 import org.chromium.base.annotations.NativeMethods;
 
-import java.util.HashSet;
-import java.util.Set;
-
 /**
  * Wrapper of the native PolicyService class in the Java layer.
  *
@@ -28,20 +25,32 @@ import java.util.Set;
 @JNINamespace("policy::android")
 public class PolicyService {
     private long mNativePolicyService;
-    private final Set<Observer> mObservers = new HashSet<Observer>();
+    private final ObserverList<Observer> mObservers = new ObserverList<Observer>();
 
     /**
      * Observer interface for observing PolicyService change for Chrome policy
      * domain.
+     *
+     * The default method below may increase method count with Desugar. If there
+     * are more than 10+ observer implementations, please consider use
+     * EmptyObserver instead for default behavior.
      */
     public interface Observer {
+        /**
+         * Invoked when Chrome policy is modified. The native class of both
+         * |previous| and |current| become invalid once the method
+         * returns. Do not use their references outside the method.
+         * @param previous PolicyMap contains values before the update.
+         * @param current PolicyMap contains values after the update.
+         */
+        default void onPolicyUpdated(PolicyMap previous, PolicyMap current) {}
         /**
          * Invoked when Chome policy domain is initialized. Observer must be
          * added before the naitve PolicyService initialization being finished.
          * Use {@link #isInitializationComplete} to check the initialization
          * state before listening to this event.
          */
-        void onPolicyServiceInitialized();
+        default void onPolicyServiceInitialized() {}
     }
 
     /**
@@ -52,7 +61,7 @@ public class PolicyService {
         if (mObservers.isEmpty()) {
             PolicyServiceJni.get().addObserver(mNativePolicyService, PolicyService.this);
         }
-        mObservers.add(observer);
+        mObservers.addObserver(observer);
     }
 
     /**
@@ -60,7 +69,7 @@ public class PolicyService {
      * policy update.
      */
     public void removeObserver(Observer observer) {
-        mObservers.remove(observer);
+        mObservers.removeObserver(observer);
         if (mObservers.isEmpty()) {
             PolicyServiceJni.get().removeObserver(mNativePolicyService, PolicyService.this);
         }
@@ -86,7 +95,19 @@ public class PolicyService {
      */
     @CalledByNative
     private void onPolicyServiceInitialized() {
-        CollectionUtil.forEach(mObservers, observer -> observer.onPolicyServiceInitialized());
+        for (Observer observer : mObservers) {
+            observer.onPolicyServiceInitialized();
+        }
+    }
+
+    /**
+     * Pass the onPolicyServiceInitialized event to the |mObservers|.
+     */
+    @CalledByNative
+    private void onPolicyUpdated(PolicyMap previous, PolicyMap current) {
+        for (Observer observer : mObservers) {
+            observer.onPolicyUpdated(previous, current);
+        }
     }
 
     @CalledByNative

components/policy/android/javatests/src/org/chromium/components/policy/test/PolicyServiceTestSupporter.java

@@ -4,6 +4,8 @@
 
 package org.chromium.components.policy.test;
 
+import static org.mockito.ArgumentMatchers.argThat;
+import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.times;
 
 import org.junit.Assert;
@@ -11,6 +13,7 @@ import org.mockito.Mockito;
 
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
+import org.chromium.components.policy.PolicyMap;
 import org.chromium.components.policy.PolicyService;
 
 import java.util.ArrayList;
@@ -25,6 +28,7 @@ import java.util.List;
 @JNINamespace("policy::android")
 public class PolicyServiceTestSupporter {
     private List<PolicyService.Observer> mObservers = new ArrayList<>();
+    private List<Boolean> mPolicyUpdated = new ArrayList<>();
 
     PolicyService mPolicyService;
 
@@ -42,6 +46,7 @@ public class PolicyServiceTestSupporter {
     private int addObserver() {
         mObservers.add(Mockito.mock(PolicyService.Observer.class));
         mPolicyService.addObserver(mObservers.get(mObservers.size() - 1));
+        mPolicyUpdated.add(false);
         return mObservers.size() - 1;
     }
 
@@ -51,10 +56,35 @@ public class PolicyServiceTestSupporter {
     }
 
     @CalledByNative
-    private void verifyObserverCalled(int index, int cnt) {
+    private void verifyInitializationEvent(int index, int cnt) {
         Mockito.verify(mObservers.get(index), times(cnt)).onPolicyServiceInitialized();
     }
 
+    @CalledByNative
+    private void verifyPolicyUpdatedEvent(int index, int cnt) {
+        Mockito.verify(mObservers.get(index), times(cnt))
+                .onPolicyUpdated(any(PolicyMap.class), any(PolicyMap.class));
+    }
+
+    @CalledByNative
+    private void setupPolicyUpdatedEventWithValues(
+            int index, PolicyMap expectPrevious, PolicyMap expectCurrent) {
+        // The native PolicyMapAndroid instance is only available inside the
+        // PolicyService.Observer.onPolicUpdated() function. Hence we have to setup the argument
+        // expectation before the event being triggered.
+        mPolicyUpdated.set(index, false);
+        Mockito.doAnswer(invocation -> mPolicyUpdated.set(index, true))
+                .when(mObservers.get(index))
+                .onPolicyUpdated(argThat(actualPrevious -> expectPrevious.isEqual(actualPrevious)),
+                        argThat(actualCurrent -> expectCurrent.isEqual(actualCurrent)));
+    }
+
+    @CalledByNative
+    private void verifyPolicyUpdatedEventWithValues(int index, int cnt) {
+        Assert.assertTrue(mPolicyUpdated.get(index));
+        verifyPolicyUpdatedEvent(index, cnt);
+    }
+
     @CalledByNative
     private void verifyNoMoreInteractions() {
         for (PolicyService.Observer observer : mObservers) {

components/policy/core/common/android/policy_map_android.cc

@@ -70,6 +70,14 @@ base::android::ScopedJavaLocalRef<jstring> PolicyMapAndroid::GetDictValue(
   return GetListOrDictValue(env, policy, /* is_dict */ true);
 }
 
+jboolean PolicyMapAndroid::Equals(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jobject>& caller,
+    jlong other) const {
+  return policy_map_.Equals(
+      reinterpret_cast<PolicyMapAndroid*>(other)->policy_map_);
+}
+
 base::android::ScopedJavaLocalRef<jobject> PolicyMapAndroid::GetJavaObject() {
   JNIEnv* env = base::android::AttachCurrentThread();
   if (!java_ref_) {

components/policy/core/common/android/policy_map_android.h

@@ -52,6 +52,10 @@ class POLICY_EXPORT PolicyMapAndroid {
       const base::android::JavaParamRef<jobject>& caller,
       const base::android::JavaRef<jstring>& policy) const;
 
+  jboolean Equals(JNIEnv* env,
+                  const base::android::JavaParamRef<jobject>& caller,
+                  jlong other) const;
+
   base::android::ScopedJavaLocalRef<jobject> GetJavaObject();
 
  private:

components/policy/core/common/android/policy_service_android.cc

@@ -36,6 +36,19 @@ void PolicyServiceAndroid::OnPolicyServiceInitialized(PolicyDomain domain) {
       base::android::ScopedJavaLocalRef<jobject>(java_ref_));
 }
 
+void PolicyServiceAndroid::OnPolicyUpdated(const PolicyNamespace& ns,
+                                           const PolicyMap& previous,
+                                           const PolicyMap& current) {
+  DCHECK_EQ(POLICY_DOMAIN_CHROME, ns.domain);
+  DCHECK(java_ref_);
+  PolicyMapAndroid previous_android(previous);
+  PolicyMapAndroid current_android(current);
+  Java_PolicyService_onPolicyUpdated(
+      base::android::AttachCurrentThread(),
+      base::android::ScopedJavaLocalRef<jobject>(java_ref_),
+      previous_android.GetJavaObject(), current_android.GetJavaObject());
+}
+
 bool PolicyServiceAndroid::IsInitializationComplete(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& caller) const {

components/policy/core/common/android/policy_service_android.h

@@ -47,6 +47,9 @@ class POLICY_EXPORT PolicyServiceAndroid : public PolicyService::Observer {
   // PolicyService::Observer implementation.
   // Pass the event to the Java observers.
   void OnPolicyServiceInitialized(PolicyDomain domain) override;
+  void OnPolicyUpdated(const PolicyNamespace& ns,
+                       const PolicyMap& previous,
+                       const PolicyMap& current) override;
 
   base::android::ScopedJavaLocalRef<jobject> GetJavaObject();
 

components/policy/core/common/android/policy_service_android_unittest.cc

@@ -6,12 +6,12 @@
 
 #include <jni.h>
 
+#include "base/android/java_exception_reporter.h"
 #include "base/android/jni_android.h"
 #include "base/android/scoped_java_ref.h"
 #include "components/policy/android/test_jni_headers/PolicyServiceTestSupporter_jni.h"
 #include "components/policy/core/common/mock_policy_service.h"
 #include "testing/gtest/include/gtest/gtest.h"
-
 using ::testing::Return;
 using ::testing::ReturnRef;
 
@@ -69,7 +69,13 @@ TEST_F(PolicyServiceAndroidTest, OneObserver) {
       Java_PolicyServiceTestSupporter_addObserver(env_, j_support_);
 
   policy_service_android_->OnPolicyServiceInitialized(POLICY_DOMAIN_CHROME);
-  Java_PolicyServiceTestSupporter_verifyObserverCalled(
+  Java_PolicyServiceTestSupporter_verifyInitializationEvent(
+      env_, j_support_, /*index*/ 0, /*times*/ 1);
+
+  policy_service_android_->OnPolicyUpdated(
+      PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()), PolicyMap(),
+      PolicyMap());
+  Java_PolicyServiceTestSupporter_verifyPolicyUpdatedEvent(
       env_, j_support_, /*index*/ 0, /*times*/ 1);
 
   EXPECT_CALL(policy_service_, RemoveObserver(POLICY_DOMAIN_CHROME,
@@ -96,10 +102,10 @@ TEST_F(PolicyServiceAndroidTest, MultipleObservers) {
 
   // Trigger the event and only the activated Java observer get notified.
   policy_service_android_->OnPolicyServiceInitialized(POLICY_DOMAIN_CHROME);
-  Java_PolicyServiceTestSupporter_verifyObserverCalled(env_, j_support_,
-                                                       observer1, /*times*/ 1);
-  Java_PolicyServiceTestSupporter_verifyObserverCalled(env_, j_support_,
-                                                       observer2, /*times*/ 0);
+  Java_PolicyServiceTestSupporter_verifyInitializationEvent(
+      env_, j_support_, observer1, /*times*/ 1);
+  Java_PolicyServiceTestSupporter_verifyInitializationEvent(
+      env_, j_support_, observer2, /*times*/ 0);
 
   // Remove the last Java observers and triggers the C++ observer cleanup too.
   EXPECT_CALL(policy_service_, RemoveObserver(POLICY_DOMAIN_CHROME,
@@ -109,5 +115,38 @@ TEST_F(PolicyServiceAndroidTest, MultipleObservers) {
   ::testing::Mock::VerifyAndClearExpectations(&policy_service_);
 }
 
+TEST_F(PolicyServiceAndroidTest, PolicyUpdateEvent) {
+  EXPECT_CALL(policy_service_,
+              AddObserver(POLICY_DOMAIN_CHROME, policy_service_android_.get()))
+      .Times(1);
+  int observer_id =
+      Java_PolicyServiceTestSupporter_addObserver(env_, j_support_);
+
+  PolicyMap previous;
+  PolicyMap current;
+  previous.Set("policy", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_MACHINE,
+               POLICY_SOURCE_PLATFORM, base::Value(1), nullptr);
+  current.Set("policy", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_MACHINE,
+              POLICY_SOURCE_PLATFORM, base::Value(2), nullptr);
+
+  // PolicyMapAndroid needs to be valid until the verification is over.
+  PolicyMapAndroid previous_android(previous);
+  PolicyMapAndroid current_android(current);
+
+  Java_PolicyServiceTestSupporter_setupPolicyUpdatedEventWithValues(
+      env_, j_support_, /*index*/ 0, previous_android.GetJavaObject(),
+      current_android.GetJavaObject());
+  policy_service_android_->OnPolicyUpdated(
+      PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()), previous, current);
+  Java_PolicyServiceTestSupporter_verifyPolicyUpdatedEventWithValues(
+      env_, j_support_, /*index*/ 0, /*times*/ 1);
+
+  EXPECT_CALL(policy_service_, RemoveObserver(POLICY_DOMAIN_CHROME,
+                                              policy_service_android_.get()))
+      .Times(1);
+  Java_PolicyServiceTestSupporter_removeObserver(env_, j_support_, observer_id);
+  ::testing::Mock::VerifyAndClearExpectations(&policy_service_);
+}
+
 }  // namespace android
 }  // namespace policy