From a9159040c86d94f7a3d45bdc7fe6c4ea24f58456 Mon Sep 17 00:00:00 2001 From: Gustav Sennton Date: Mon, 11 Apr 2016 16:32:52 +0100 Subject: Update persistent WebView packages setting only when user changes it. To ensure that we don't permanently change WebView implementation if the current package is temporarily uninstalled (e.g. when being replaced) we don't update our persistent setting unless the user explicitly changes WebView implementation (and on boot!). Unfortunately this will means that the Dev Setting for changing WebView implementation will work in a slightly less intuitive way. The persistent setting is now persistent across uninstalls and installs. I.e. the Dev Setting shows the current WebView implementation though that could differ to the value chosen by the user since the package chosen by the user could be uninstalled or disabled. In this case installing/enabling that package would again make the Dev Setting point to it. However, as a compromise, we do change the setting at boot so that if the currently chosen package is not valid we will change the setting so that it points to the package we currently use instead. Also ensure we only use WebView packages that are available-by-default if no WebView packages are enabled. Add unit test to ensure that if a user-chosen provider is uninstalled we switch back to using that provider when it is installed again. Add unit test to ensure we switch user-chosen provider at boot if the chosen one is uninstalled. Bug: 27673076 Change-Id: Icd27ae302798ebf695b9ef4bd4d5fd47fe4be02c --- .../server/webkit/WebViewUpdateServiceImpl.java | 15 ++- .../server/webkit/WebViewUpdateServiceTest.java | 137 ++++++++++++++------- 2 files changed, 103 insertions(+), 49 deletions(-) diff --git a/services/core/java/com/android/server/webkit/WebViewUpdateServiceImpl.java b/services/core/java/com/android/server/webkit/WebViewUpdateServiceImpl.java index df5d0274c20a..547bbfc6d1c8 100644 --- a/services/core/java/com/android/server/webkit/WebViewUpdateServiceImpl.java +++ b/services/core/java/com/android/server/webkit/WebViewUpdateServiceImpl.java @@ -283,6 +283,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) { @@ -337,7 +344,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 @@ -424,9 +430,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; 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 c00520dc3552..7d404c9d09ed 100644 --- a/services/tests/servicestests/src/com/android/server/webkit/WebViewUpdateServiceTest.java +++ b/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(); - WebViewProviderResponse response = mWebViewUpdateServiceImpl.waitForAndGetProvider(); - assertEquals(WebViewFactory.LIBLOAD_SUCCESS, response.status); - assertEquals(validPackage, response.packageInfo.packageName); + checkPreparationPhasesForPackage(validPackage, 1 /* first preparation for this package */); 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); - - 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); + checkPreparationPhasesForPackage(finalPackage, 1 /* first preparation for this package */); 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(); - WebViewProviderResponse response = mWebViewUpdateServiceImpl.waitForAndGetProvider(); - assertEquals(WebViewFactory.LIBLOAD_SUCCESS, response.status); - assertEquals(fallbackPackage, response.packageInfo.packageName); + checkPreparationPhasesForPackage(fallbackPackage, + 1 /* first preparation for this package*/); // 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( - 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); + checkPreparationPhasesForPackage(primaryPackage, 1 /* first preparation for this package*/); 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 - Mockito.verify(mTestSystemImpl, Mockito.times(2)).onWebViewProviderChanged( - Mockito.argThat(new IsPackageInfoWithName(firstPackage))); + // Ensure we use firstPackage + checkPreparationPhasesForPackage(firstPackage, 2 /* second preparation for this package */); + } - 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(); - assertEquals(WebViewFactory.LIBLOAD_SUCCESS, response.status); - assertEquals(firstPackage, response.packageInfo.packageName); + // Explicitly use the second package + mWebViewUpdateServiceImpl.changeProviderAndSetting(secondPackage); + 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 -- cgit v1.2.3-59-g8ed1b