From 95f7e8e06dd0003a813f7a4b3c718c68d0fa4a01 Mon Sep 17 00:00:00 2001 From: Gustav Sennton Date: Thu, 14 Apr 2016 15:07:09 +0100 Subject: [PATCH] Fix the behaviour for explicitly switching WebView provider Before this CL when we tried to change WebView provider through the Dev setting and failed we would then have the setting point to the provider used before the change. With this CL we instead let the setting point to the intended target but also switch to the package which the WebViewUpdateService see as most fit (rather than just reverting back to the original choice). Add unit tests to verify we kill processes depending on WebView packages when switching WebView provider. Add unit tests to ensure we can recover from trying to change provider when there are no providers available. Bug: 27673076 Bug: 27635535 Change-Id: Ie7bb6afdf8acf4344cfb363947929c0b492775fb --- .../webkit/WebViewUpdateServiceImpl.java | 28 +-- .../android/server/webkit/TestSystemImpl.java | 6 +- .../webkit/WebViewUpdateServiceTest.java | 174 +++++++++++++++++- 3 files changed, 189 insertions(+), 19 deletions(-) diff --git a/services/core/java/com/android/server/webkit/WebViewUpdateServiceImpl.java b/services/core/java/com/android/server/webkit/WebViewUpdateServiceImpl.java index 8c01c4cf76a1f..7bd1f6372d099 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 @@ public class WebViewUpdateServiceImpl { /** * 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 ""; + } + // Perform the provider change if we chose a new provider + if (providerChanged) { + onWebViewProviderChanged(newPackage); } - 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 26b87c5d9283d..e33be400a8490 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 android.webkit.WebViewProviderInfo; import java.util.HashMap; public class TestSystemImpl implements SystemInterface { - private String mUserProvider = ""; + private String mUserProvider = null; private final WebViewProviderInfo[] mPackageConfigs; HashMap mPackages = new HashMap(); private boolean mFallbackLogicEnabled; @@ -105,6 +105,10 @@ public class TestSystemImpl implements SystemInterface { 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 7d404c9d09ed7..c03324aa9e325 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 class WebViewUpdateServiceTest extends AndroidTestCase { } 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 @@ public class WebViewUpdateServiceTest extends AndroidTestCase { 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 @@ public class WebViewUpdateServiceTest extends AndroidTestCase { 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 @@ public class WebViewUpdateServiceTest extends AndroidTestCase { 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 @@ public class WebViewUpdateServiceTest extends AndroidTestCase { 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 @@ public class WebViewUpdateServiceTest extends AndroidTestCase { 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 @@ public class WebViewUpdateServiceTest extends AndroidTestCase { 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)); + } }