Merge "Update persistent WebView packages setting only when user changes it." into nyc-dev

services/core/java/com/android/server/webkit/WebViewUpdateServiceImpl.java

@@ -291,6 +291,13 @@ public class WebViewUpdateServiceImpl {
             try {
                 synchronized(mLock) {
                     mCurrentWebViewPackage = findPreferredWebViewPackage();
+                    // Don't persist the user-chosen setting across boots if the package being
+                    // chosen is not used (could be disabled or uninstalled) so that the user won't
+                    // be surprised by the device switching to using a certain webview package,
+                    // that was uninstalled/disabled a long time ago, if it is installed/enabled
+                    // again.
+                    mSystemInterface.updateUserSetting(mContext,
+                            mCurrentWebViewPackage.packageName);
                     onWebViewProviderChanged(mCurrentWebViewPackage);
                 }
             } catch (Throwable t) {
@@ -345,7 +352,6 @@ public class WebViewUpdateServiceImpl {
                 mAnyWebViewInstalled = true;
                 if (mNumRelroCreationsStarted == mNumRelroCreationsFinished) {
                     mCurrentWebViewPackage = newPackage;
-                    mSystemInterface.updateUserSetting(mContext, newPackage.packageName);
 
                     // The relro creations might 'finish' (not start at all) before
                     // WebViewFactory.onWebViewProviderChanged which means we might not know the
@@ -432,9 +438,12 @@ public class WebViewUpdateServiceImpl {
                 }
             }
 
-            // Could not find any enabled package either, use the most stable provider.
+            // Could not find any enabled package either, use the most stable and default-available
+            // provider.
             for (ProviderAndPackageInfo providerAndPackage : providers) {
-                return providerAndPackage.packageInfo;
+                if (providerAndPackage.provider.availableByDefault) {
+                    return providerAndPackage.packageInfo;
+                }
             }
 
             mAnyWebViewInstalled = false;

services/tests/servicestests/src/com/android/server/webkit/WebViewUpdateServiceTest.java

@@ -90,12 +90,13 @@ public class WebViewUpdateServiceTest extends AndroidTestCase {
         }
     }
 
-    private void checkCertainPackageUsedAfterWebViewPreparation(String expectedProviderName,
+    private void checkCertainPackageUsedAfterWebViewBootPreparation(String expectedProviderName,
             WebViewProviderInfo[] webviewPackages) {
-        checkCertainPackageUsedAfterWebViewPreparation(expectedProviderName, webviewPackages, 1);
+        checkCertainPackageUsedAfterWebViewBootPreparation(
+                expectedProviderName, webviewPackages, 1);
     }
 
-    private void checkCertainPackageUsedAfterWebViewPreparation(String expectedProviderName,
+    private void checkCertainPackageUsedAfterWebViewBootPreparation(String expectedProviderName,
             WebViewProviderInfo[] webviewPackages, int numRelros) {
         setupWithPackages(webviewPackages, true, numRelros);
         // Add (enabled and valid) package infos for each provider
@@ -156,6 +157,19 @@ public class WebViewUpdateServiceTest extends AndroidTestCase {
         return p;
     }
 
+    private void checkPreparationPhasesForPackage(String expectedPackage, int numPreparation) {
+        // Verify that onWebViewProviderChanged was called for the numPreparation'th time for the
+        // expected package
+        Mockito.verify(mTestSystemImpl, Mockito.times(numPreparation)).onWebViewProviderChanged(
+                Mockito.argThat(new IsPackageInfoWithName(expectedPackage)));
+
+        mWebViewUpdateServiceImpl.notifyRelroCreationCompleted();
+
+        WebViewProviderResponse response = mWebViewUpdateServiceImpl.waitForAndGetProvider();
+        assertEquals(WebViewFactory.LIBLOAD_SUCCESS, response.status);
+        assertEquals(expectedPackage, response.packageInfo.packageName);
+    }
+
 
     // ****************
     // Tests
@@ -164,7 +178,7 @@ public class WebViewUpdateServiceTest extends AndroidTestCase {
 
     public void testWithSinglePackage() {
         String testPackageName = "test.package.name";
-        checkCertainPackageUsedAfterWebViewPreparation(testPackageName,
+        checkCertainPackageUsedAfterWebViewBootPreparation(testPackageName,
                 new WebViewProviderInfo[] {
                     new WebViewProviderInfo(testPackageName, "",
                             true /*default available*/, false /* fallback */, null)});
@@ -176,12 +190,12 @@ public class WebViewUpdateServiceTest extends AndroidTestCase {
         WebViewProviderInfo[] packages = new WebViewProviderInfo[] {
             new WebViewProviderInfo(nonDefaultPackage, "", false, false, null),
             new WebViewProviderInfo(defaultPackage, "", true, false, null)};
-        checkCertainPackageUsedAfterWebViewPreparation(defaultPackage, packages);
+        checkCertainPackageUsedAfterWebViewBootPreparation(defaultPackage, packages);
     }
 
     public void testSeveralRelros() {
         String singlePackage = "singlePackage";
-        checkCertainPackageUsedAfterWebViewPreparation(
+        checkCertainPackageUsedAfterWebViewBootPreparation(
                 singlePackage,
                 new WebViewProviderInfo[] {
                     new WebViewProviderInfo(singlePackage, "", true /*def av*/, false, null)},
@@ -215,14 +229,8 @@ public class WebViewUpdateServiceTest extends AndroidTestCase {
 
         mWebViewUpdateServiceImpl.prepareWebViewInSystemServer();
 
-        Mockito.verify(mTestSystemImpl).onWebViewProviderChanged(
-                Mockito.argThat(new IsPackageInfoWithName(validPackage)));
 
-        mWebViewUpdateServiceImpl.notifyRelroCreationCompleted();
+        checkPreparationPhasesForPackage(validPackage, 1 /* first preparation for this package */);
-
-        WebViewProviderResponse response = mWebViewUpdateServiceImpl.waitForAndGetProvider();
-        assertEquals(WebViewFactory.LIBLOAD_SUCCESS, response.status);
-        assertEquals(validPackage, response.packageInfo.packageName);
 
         WebViewProviderInfo[] validPackages = mWebViewUpdateServiceImpl.getValidWebViewPackages();
         assertEquals(1, validPackages.length);
@@ -292,18 +300,10 @@ public class WebViewUpdateServiceTest extends AndroidTestCase {
 
     private void checkSwitchingProvider(WebViewProviderInfo[] packages, String initialPackage,
             String finalPackage) {
-        checkCertainPackageUsedAfterWebViewPreparation(initialPackage, packages);
+        checkCertainPackageUsedAfterWebViewBootPreparation(initialPackage, packages);
 
         mWebViewUpdateServiceImpl.changeProviderAndSetting(finalPackage);
-
+        checkPreparationPhasesForPackage(finalPackage, 1 /* first preparation for this package */);
-        Mockito.verify(mTestSystemImpl).onWebViewProviderChanged(
-                Mockito.argThat(new IsPackageInfoWithName(finalPackage)));
-
-        mWebViewUpdateServiceImpl.notifyRelroCreationCompleted();
-
-        WebViewProviderResponse secondResponse = mWebViewUpdateServiceImpl.waitForAndGetProvider();
-        assertEquals(WebViewFactory.LIBLOAD_SUCCESS, secondResponse.status);
-        assertEquals(finalPackage, secondResponse.packageInfo.packageName);
 
         Mockito.verify(mTestSystemImpl).killPackageDependents(Mockito.eq(initialPackage));
     }
@@ -455,14 +455,9 @@ public class WebViewUpdateServiceTest extends AndroidTestCase {
         mWebViewUpdateServiceImpl.prepareWebViewInSystemServer();
         Mockito.verify(mTestSystemImpl, Mockito.never()).uninstallAndDisablePackageForAllUsers(
                 Matchers.anyObject(), Matchers.anyObject());
-        Mockito.verify(mTestSystemImpl).onWebViewProviderChanged(
-                Mockito.argThat(new IsPackageInfoWithName(fallbackPackage)));
 
-        mWebViewUpdateServiceImpl.notifyRelroCreationCompleted();
+        checkPreparationPhasesForPackage(fallbackPackage,
-
+                1 /* first preparation for this package*/);
-        WebViewProviderResponse response = mWebViewUpdateServiceImpl.waitForAndGetProvider();
-        assertEquals(WebViewFactory.LIBLOAD_SUCCESS, response.status);
-        assertEquals(fallbackPackage, response.packageInfo.packageName);
 
         // Install primary package
         mTestSystemImpl.setPackageInfo(
@@ -470,17 +465,10 @@ public class WebViewUpdateServiceTest extends AndroidTestCase {
         mWebViewUpdateServiceImpl.packageStateChanged(primaryPackage,
                 WebViewUpdateService.PACKAGE_ADDED);
 
-        // Verify fallback disabled and primary package used as provider
+        // Verify fallback disabled, primary package used as provider, and fallback package killed
         Mockito.verify(mTestSystemImpl).uninstallAndDisablePackageForAllUsers(
                 Matchers.anyObject(), Mockito.eq(fallbackPackage));
-        Mockito.verify(mTestSystemImpl).onWebViewProviderChanged(
+        checkPreparationPhasesForPackage(primaryPackage, 1 /* first preparation for this package*/);
-                Mockito.argThat(new IsPackageInfoWithName(primaryPackage)));
-
-        // Finish the webview preparation and ensure primary package used and fallback killed
-        mWebViewUpdateServiceImpl.notifyRelroCreationCompleted();
-        response = mWebViewUpdateServiceImpl.waitForAndGetProvider();
-        assertEquals(WebViewFactory.LIBLOAD_SUCCESS, response.status);
-        assertEquals(primaryPackage, response.packageInfo.packageName);
         Mockito.verify(mTestSystemImpl).killPackageDependents(Mockito.eq(fallbackPackage));
     }
 
@@ -598,15 +586,72 @@ public class WebViewUpdateServiceTest extends AndroidTestCase {
         mWebViewUpdateServiceImpl.packageStateChanged(firstPackage,
                 WebViewUpdateService.PACKAGE_ADDED);
 
-        // Second time we call onWebViewProviderChanged for firstPackage
+        // Ensure we use firstPackage
-        Mockito.verify(mTestSystemImpl, Mockito.times(2)).onWebViewProviderChanged(
+        checkPreparationPhasesForPackage(firstPackage, 2 /* second preparation for this package */);
-                Mockito.argThat(new IsPackageInfoWithName(firstPackage)));
+    }
 
-        mWebViewUpdateServiceImpl.notifyRelroCreationCompleted();
+    /**
+     * Verify that even if a user-chosen package is removed temporarily we start using it again when
+     * it is added back.
+     */
+    public void testTempRemovePackageDoesntSwitchProviderPermanently() {
+        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);
 
-        response = mWebViewUpdateServiceImpl.waitForAndGetProvider();
+        // Explicitly use the second package
-        assertEquals(WebViewFactory.LIBLOAD_SUCCESS, response.status);
+        mWebViewUpdateServiceImpl.changeProviderAndSetting(secondPackage);
-        assertEquals(firstPackage, response.packageInfo.packageName);
+        checkPreparationPhasesForPackage(secondPackage, 1 /* first time for this package */);
+
+        // Remove second package (invalidate it) and verify that first package is used
+        mTestSystemImpl.setPackageInfo(createPackageInfo(secondPackage, true /* enabled */,
+                    false /* valid */));
+        mWebViewUpdateServiceImpl.packageStateChanged(secondPackage,
+                WebViewUpdateService.PACKAGE_ADDED);
+        checkPreparationPhasesForPackage(firstPackage, 2 /* second time for this package */);
+
+        // Now make the second package valid again and verify that it is used again
+        mTestSystemImpl.setPackageInfo(createPackageInfo(secondPackage, true /* enabled */,
+                    true /* valid */));
+        mWebViewUpdateServiceImpl.packageStateChanged(secondPackage,
+                WebViewUpdateService.PACKAGE_ADDED);
+        checkPreparationPhasesForPackage(secondPackage, 2 /* second time for this package */);
+    }
+
+    /**
+     * Ensure that we update the user-chosen setting across boots if the chosen package is no
+     * longer installed and valid.
+     */
+    public void testProviderSettingChangedDuringBootIfProviderNotAvailable() {
+        String chosenPackage = "chosenPackage";
+        String nonChosenPackage = "non-chosenPackage";
+        WebViewProviderInfo[] packages = new WebViewProviderInfo[] {
+            new WebViewProviderInfo(chosenPackage, "", true /* default available */,
+                    false /* fallback */, null),
+            new WebViewProviderInfo(nonChosenPackage, "", true /* default available */,
+                    false /* fallback */, null)};
+
+        setupWithPackages(packages);
+        // Only 'install' nonChosenPackage
+        mTestSystemImpl.setPackageInfo(
+                createPackageInfo(nonChosenPackage, true /* enabled */, true /* valid */));
+
+        // Set user-chosen package
+        mTestSystemImpl.updateUserSetting(null, chosenPackage);
+
+        mWebViewUpdateServiceImpl.prepareWebViewInSystemServer();
+
+        // Verify that we switch the setting to point to the current package
+        Mockito.verify(mTestSystemImpl).updateUserSetting(
+                Mockito.anyObject(), Mockito.eq(nonChosenPackage));
+        assertEquals(nonChosenPackage, mTestSystemImpl.getUserChosenWebViewProvider(null));
+
+        checkPreparationPhasesForPackage(nonChosenPackage, 1);
     }
 
     // TODO (gsennton) add more tests for ensuring killPackageDependents is called / not called