Merge "Fix the behaviour for explicitly switching WebView provider" into nyc-dev
diff --git a/services/core/java/com/android/server/webkit/WebViewUpdateServiceImpl.java b/services/core/java/com/android/server/webkit/WebViewUpdateServiceImpl.java
index 8c01c4c..7bd1f63 100644
--- a/services/core/java/com/android/server/webkit/WebViewUpdateServiceImpl.java
+++ b/services/core/java/com/android/server/webkit/WebViewUpdateServiceImpl.java
@@ -308,38 +308,42 @@
 
         /**
          * Change WebView provider and provider setting and kill packages using the old provider.
-         * Return the new provider (in case we are in the middle of creating relro files this new
-         * provider will not be in use directly, but will when the relros are done).
+         * Return the new provider (in case we are in the middle of creating relro files, or
+         * replacing that provider it will not be in use directly, but will be used when the relros
+         * or the replacement are done).
          */
         public String changeProviderAndSetting(String newProviderName) {
             PackageInfo oldPackage = null;
             PackageInfo newPackage = null;
+            boolean providerChanged = false;
             synchronized(mLock) {
                 oldPackage = mCurrentWebViewPackage;
                 mSystemInterface.updateUserSetting(mContext, newProviderName);
 
                 try {
                     newPackage = findPreferredWebViewPackage();
-                    if (oldPackage != null
-                            && newPackage.packageName.equals(oldPackage.packageName)) {
-                        // If we don't perform the user change, revert the settings change.
-                        mSystemInterface.updateUserSetting(mContext, newPackage.packageName);
-                        return newPackage.packageName;
-                    }
+                    providerChanged = (oldPackage == null)
+                            || !newPackage.packageName.equals(oldPackage.packageName);
                 } catch (WebViewFactory.MissingWebViewPackageException e) {
                     Slog.e(TAG, "Tried to change WebView provider but failed to fetch WebView " +
                             "package " + e);
                     // If we don't perform the user change but don't have an installed WebView
                     // package, we will have changed the setting and it will be used when a package
                     // is available.
-                    return newProviderName;
+                    return "";
                 }
-                onWebViewProviderChanged(newPackage);
+                // Perform the provider change if we chose a new provider
+                if (providerChanged) {
+                    onWebViewProviderChanged(newPackage);
+                }
             }
-            // Kill apps using the old provider
-            if (oldPackage != null) {
+            // Kill apps using the old provider only if we changed provider
+            if (providerChanged && oldPackage != null) {
                 mSystemInterface.killPackageDependents(oldPackage.packageName);
             }
+            // Return the new provider, this is not necessarily the one we were asked to switch to
+            // But the persistent setting will now be pointing to the provider we were asked to
+            // switch to anyway
             return newPackage.packageName;
         }
 
diff --git a/services/tests/servicestests/src/com/android/server/webkit/TestSystemImpl.java b/services/tests/servicestests/src/com/android/server/webkit/TestSystemImpl.java
index 26b87c5..e33be40 100644
--- a/services/tests/servicestests/src/com/android/server/webkit/TestSystemImpl.java
+++ b/services/tests/servicestests/src/com/android/server/webkit/TestSystemImpl.java
@@ -24,7 +24,7 @@
 import java.util.HashMap;
 
 public class TestSystemImpl implements SystemInterface {
-    private String mUserProvider = "";
+    private String mUserProvider = null;
     private final WebViewProviderInfo[] mPackageConfigs;
     HashMap<String, PackageInfo> mPackages = new HashMap();
     private boolean mFallbackLogicEnabled;
@@ -105,6 +105,10 @@
         mPackages.put(pi.packageName, pi);
     }
 
+    public void removePackageInfo(String packageName) {
+        mPackages.remove(packageName);
+    }
+
     @Override
     public int getFactoryPackageVersion(String packageName) {
         return 0;
diff --git a/services/tests/servicestests/src/com/android/server/webkit/WebViewUpdateServiceTest.java b/services/tests/servicestests/src/com/android/server/webkit/WebViewUpdateServiceTest.java
index 7d404c9..c03324a 100644
--- a/services/tests/servicestests/src/com/android/server/webkit/WebViewUpdateServiceTest.java
+++ b/services/tests/servicestests/src/com/android/server/webkit/WebViewUpdateServiceTest.java
@@ -269,14 +269,27 @@
     }
 
     public void testFailListingInvalidWebviewPackage() {
-        WebViewProviderInfo wpi = new WebViewProviderInfo("", "", true, true, null);
+        WebViewProviderInfo wpi = new WebViewProviderInfo("package", "", true, true, null);
         WebViewProviderInfo[] packages = new WebViewProviderInfo[] {wpi};
         setupWithPackages(packages);
-        mTestSystemImpl.setPackageInfo(createPackageInfo(wpi.packageName, true, false));
+        mTestSystemImpl.setPackageInfo(
+                createPackageInfo(wpi.packageName, true /* enabled */, false /* valid */));
 
         mWebViewUpdateServiceImpl.prepareWebViewInSystemServer();
+
+        Mockito.verify(mTestSystemImpl, Mockito.never()).onWebViewProviderChanged(
+                Matchers.anyObject());
+
         WebViewProviderResponse response = mWebViewUpdateServiceImpl.waitForAndGetProvider();
         assertEquals(WebViewFactory.LIBLOAD_FAILED_LISTING_WEBVIEW_PACKAGES, response.status);
+
+        // Verify that we can recover from failing to list webview packages.
+        mTestSystemImpl.setPackageInfo(
+                createPackageInfo(wpi.packageName, true /* enabled */, true /* valid */));
+        mWebViewUpdateServiceImpl.packageStateChanged(wpi.packageName,
+                WebViewUpdateService.PACKAGE_ADDED_REPLACED);
+
+        checkPreparationPhasesForPackage(wpi.packageName, 1);
     }
 
     // Test that switching provider using changeProviderAndSetting works.
@@ -463,7 +476,7 @@
         mTestSystemImpl.setPackageInfo(
                 createPackageInfo(primaryPackage, true /* enabled */ , true /* valid */));
         mWebViewUpdateServiceImpl.packageStateChanged(primaryPackage,
-                WebViewUpdateService.PACKAGE_ADDED);
+                WebViewUpdateService.PACKAGE_ADDED_REPLACED);
 
         // Verify fallback disabled, primary package used as provider, and fallback package killed
         Mockito.verify(mTestSystemImpl).uninstallAndDisablePackageForAllUsers(
@@ -490,16 +503,22 @@
                 Mockito.eq(fallbackPackage), Mockito.eq(false) /* enable */,
                 Matchers.anyInt());
 
+        checkPreparationPhasesForPackage(primaryPackage, 1);
+
+        // Disable primary package and ensure fallback becomes enabled and used
         mTestSystemImpl.setPackageInfo(
                 createPackageInfo(primaryPackage, false /* enabled */, true /* valid */));
         mWebViewUpdateServiceImpl.packageStateChanged(primaryPackage,
                 WebViewUpdateService.PACKAGE_CHANGED);
 
-        // Verify fallback becomes enabled when primary package becomes disabled
         Mockito.verify(mTestSystemImpl).enablePackageForUser(
                 Mockito.eq(fallbackPackage), Mockito.eq(true) /* enable */,
                 Matchers.anyInt());
 
+        checkPreparationPhasesForPackage(fallbackPackage, 1);
+
+
+        // Again enable primary package and verify primary is used and fallback becomes disabled
         mTestSystemImpl.setPackageInfo(
                 createPackageInfo(primaryPackage, true /* enabled */, true /* valid */));
         mWebViewUpdateServiceImpl.packageStateChanged(primaryPackage,
@@ -509,6 +528,8 @@
         Mockito.verify(mTestSystemImpl, Mockito.times(2)).enablePackageForUser(
                 Mockito.eq(fallbackPackage), Mockito.eq(false) /* enable */,
                 Matchers.anyInt());
+
+        checkPreparationPhasesForPackage(primaryPackage, 2);
     }
 
     public void testAddUserWhenFallbackLogicEnabled() {
@@ -565,6 +586,9 @@
         Mockito.verify(mTestSystemImpl).onWebViewProviderChanged(
                 Mockito.argThat(new IsPackageInfoWithName(firstPackage)));
 
+        // Change provider during relro creation to enter a state where we are
+        // waiting for relro creation to complete just to re-run relro creation.
+        // (so that in next notifyRelroCreationCompleted() call we have to list webview packages)
         mWebViewUpdateServiceImpl.changeProviderAndSetting(secondPackage);
 
         // Make packages invalid to cause exception to be thrown
@@ -584,7 +608,7 @@
                     true /* valid */));
 
         mWebViewUpdateServiceImpl.packageStateChanged(firstPackage,
-                WebViewUpdateService.PACKAGE_ADDED);
+                WebViewUpdateService.PACKAGE_ADDED_REPLACED);
 
         // Ensure we use firstPackage
         checkPreparationPhasesForPackage(firstPackage, 2 /* second preparation for this package */);
@@ -654,5 +678,143 @@
         checkPreparationPhasesForPackage(nonChosenPackage, 1);
     }
 
-    // TODO (gsennton) add more tests for ensuring killPackageDependents is called / not called
+    public void testRecoverFailedListingWebViewPackagesSettingsChange() {
+        checkRecoverAfterFailListingWebviewPackages(true);
+    }
+
+    public void testRecoverFailedListingWebViewPackagesAddedPackage() {
+        checkRecoverAfterFailListingWebviewPackages(false);
+    }
+
+    /**
+     * Test that we can recover correctly from failing to list WebView packages.
+     * settingsChange: whether to fail during changeProviderAndSetting or packageStateChanged
+     */
+    public void checkRecoverAfterFailListingWebviewPackages(boolean settingsChange) {
+        String firstPackage = "first";
+        String secondPackage = "second";
+        WebViewProviderInfo[] packages = new WebViewProviderInfo[] {
+            new WebViewProviderInfo(firstPackage, "", true /* default available */,
+                    false /* fallback */, null),
+            new WebViewProviderInfo(secondPackage, "", true /* default available */,
+                    false /* fallback */, null)};
+        checkCertainPackageUsedAfterWebViewBootPreparation(firstPackage, packages);
+
+        // Make both packages invalid so that we fail listing WebView packages
+        mTestSystemImpl.setPackageInfo(createPackageInfo(firstPackage, true /* enabled */,
+                    false /* valid */));
+        mTestSystemImpl.setPackageInfo(createPackageInfo(secondPackage, true /* enabled */,
+                    false /* valid */));
+
+        // Change package to hit the webview packages listing problem.
+        if (settingsChange) {
+            mWebViewUpdateServiceImpl.changeProviderAndSetting(secondPackage);
+        } else {
+            mWebViewUpdateServiceImpl.packageStateChanged(secondPackage,
+                    WebViewUpdateService.PACKAGE_ADDED_REPLACED);
+        }
+
+        WebViewProviderResponse response = mWebViewUpdateServiceImpl.waitForAndGetProvider();
+        assertEquals(WebViewFactory.LIBLOAD_FAILED_LISTING_WEBVIEW_PACKAGES, response.status);
+
+        // Make second package valid and verify that we can load it again
+        mTestSystemImpl.setPackageInfo(createPackageInfo(secondPackage, true /* enabled */,
+                    true /* valid */));
+
+        mWebViewUpdateServiceImpl.packageStateChanged(secondPackage,
+                WebViewUpdateService.PACKAGE_ADDED_REPLACED);
+
+
+        checkPreparationPhasesForPackage(secondPackage, 1);
+    }
+
+    public void testDontKillIfPackageReplaced() {
+        checkDontKillIfPackageRemoved(true);
+    }
+
+    public void testDontKillIfPackageRemoved() {
+        checkDontKillIfPackageRemoved(false);
+    }
+
+    public void checkDontKillIfPackageRemoved(boolean replaced) {
+        String firstPackage = "first";
+        String secondPackage = "second";
+        WebViewProviderInfo[] packages = new WebViewProviderInfo[] {
+            new WebViewProviderInfo(firstPackage, "", true /* default available */,
+                    false /* fallback */, null),
+            new WebViewProviderInfo(secondPackage, "", true /* default available */,
+                    false /* fallback */, null)};
+        checkCertainPackageUsedAfterWebViewBootPreparation(firstPackage, packages);
+
+        // Replace or remove the current webview package
+        if (replaced) {
+            mTestSystemImpl.setPackageInfo(
+                    createPackageInfo(firstPackage, true /* enabled */, false /* valid */));
+            mWebViewUpdateServiceImpl.packageStateChanged(firstPackage,
+                    WebViewUpdateService.PACKAGE_ADDED_REPLACED);
+        } else {
+            mTestSystemImpl.removePackageInfo(firstPackage);
+            mWebViewUpdateServiceImpl.packageStateChanged(firstPackage,
+                    WebViewUpdateService.PACKAGE_REMOVED);
+        }
+
+        checkPreparationPhasesForPackage(secondPackage, 1);
+
+        Mockito.verify(mTestSystemImpl, Mockito.never()).killPackageDependents(
+                Mockito.anyObject());
+    }
+
+    public void testKillIfSettingChanged() {
+        String firstPackage = "first";
+        String secondPackage = "second";
+        WebViewProviderInfo[] packages = new WebViewProviderInfo[] {
+            new WebViewProviderInfo(firstPackage, "", true /* default available */,
+                    false /* fallback */, null),
+            new WebViewProviderInfo(secondPackage, "", true /* default available */,
+                    false /* fallback */, null)};
+        checkCertainPackageUsedAfterWebViewBootPreparation(firstPackage, packages);
+
+        mWebViewUpdateServiceImpl.changeProviderAndSetting(secondPackage);
+
+        checkPreparationPhasesForPackage(secondPackage, 1);
+
+        Mockito.verify(mTestSystemImpl).killPackageDependents(Mockito.eq(firstPackage));
+    }
+
+    /**
+     * Test that we kill apps using an old provider when we change the provider setting, even if the
+     * new provider is not the one we intended to change to.
+     */
+    public void testKillIfChangeProviderIncorrectly() {
+        String firstPackage = "first";
+        String secondPackage = "second";
+        String thirdPackage = "third";
+        WebViewProviderInfo[] packages = new WebViewProviderInfo[] {
+            new WebViewProviderInfo(firstPackage, "", true /* default available */,
+                    false /* fallback */, null),
+            new WebViewProviderInfo(secondPackage, "", true /* default available */,
+                    false /* fallback */, null),
+            new WebViewProviderInfo(thirdPackage, "", true /* default available */,
+                    false /* fallback */, null)};
+        setupWithPackages(packages);
+        setEnabledAndValidPackageInfos(packages);
+
+        // Start with the setting pointing to the third package
+        mTestSystemImpl.updateUserSetting(null, thirdPackage);
+
+        mWebViewUpdateServiceImpl.prepareWebViewInSystemServer();
+        checkPreparationPhasesForPackage(thirdPackage, 1);
+
+        mTestSystemImpl.setPackageInfo(
+                createPackageInfo(secondPackage, true /* enabled */, false /* valid */));
+
+        // Try to switch to the invalid second package, this should result in switching to the first
+        // package, since that is more preferred than the third one.
+        assertEquals(firstPackage,
+                mWebViewUpdateServiceImpl.changeProviderAndSetting(secondPackage));
+
+        checkPreparationPhasesForPackage(firstPackage, 1);
+
+        Mockito.verify(mTestSystemImpl).killPackageDependents(Mockito.eq(thirdPackage));
+    }
 }