Add warning for packaged apps trying to use a persistent background page

Packaged apps do not support a persistent background page, so the
manifest entry is ignored. To make this more obvious, this CL adds a
warning when validating the manifest.

R=rockot@chromium.org,asargent@chromium.org
BUG=586535

Review URL: https://codereview.chromium.org/1701523002

Cr-Commit-Position: refs/heads/master@{#377775}

7 files changed, 88 insertions, 5 deletions

diff --git a/chrome/common/extensions/manifest_tests/extension_manifests_background_unittest.cc b/chrome/common/extensions/manifest_tests/extension_manifests_background_unittest.cc
index a2683a2..3f44d29 100644
--- a/chrome/common/extensions/manifest_tests/extension_manifests_background_unittest.cc
+++ b/chrome/common/extensions/manifest_tests/extension_manifests_background_unittest.cc
@@ -109,4 +109,37 @@ TEST_F(ExtensionManifestBackgroundTest, BackgroundPageWebRequest) {
                      errors::kWebRequestConflictsWithLazyBackground);
 }
 
+TEST_F(ExtensionManifestBackgroundTest, BackgroundPagePersistentPlatformApp) {
+  scoped_refptr<Extension> extension =
+      LoadAndExpectSuccess("background_page_persistent_app.json");
+  ASSERT_TRUE(extension->is_platform_app());
+  ASSERT_TRUE(BackgroundInfo::HasBackgroundPage(extension.get()));
+  EXPECT_FALSE(BackgroundInfo::HasPersistentBackgroundPage(extension.get()));
+
+  std::string error;
+  std::vector<InstallWarning> warnings;
+  ManifestHandler::ValidateExtension(extension.get(), &error, &warnings);
+  // Persistent background pages are not supported for packaged apps.
+  // The persistent flag is ignored and a warining is printed.
+  EXPECT_EQ(1U, warnings.size());
+  EXPECT_EQ(errors::kInvalidBackgroundPersistentInPlatformApp,
+            warnings[0].message);
+}
+
+TEST_F(ExtensionManifestBackgroundTest, BackgroundPagePersistentInvalidKey) {
+  scoped_refptr<Extension> extension =
+      LoadAndExpectSuccess("background_page_invalid_persistent_key_app.json");
+  ASSERT_TRUE(extension->is_platform_app());
+  ASSERT_TRUE(BackgroundInfo::HasBackgroundPage(extension.get()));
+  EXPECT_FALSE(BackgroundInfo::HasPersistentBackgroundPage(extension.get()));
+
+  std::string error;
+  std::vector<InstallWarning> warnings;
+  ManifestHandler::ValidateExtension(extension.get(), &error, &warnings);
+  // The key 'background.persistent' is not supported for packaged apps.
+  EXPECT_EQ(1U, warnings.size());
+  EXPECT_EQ(errors::kBackgroundPersistentInvalidForPlatformApps,
+            warnings[0].message);
+}
+
 }  // namespace extensions
diff --git a/chrome/test/data/extensions/api_test/mdns/api-packaged-apps/manifest.json b/chrome/test/data/extensions/api_test/mdns/api-packaged-apps/manifest.json
index 94253d9..81cfe0a 100644
--- a/chrome/test/data/extensions/api_test/mdns/api-packaged-apps/manifest.json
+++ b/chrome/test/data/extensions/api_test/mdns/api-packaged-apps/manifest.json
@@ -7,8 +7,7 @@
   "permissions": ["mdns"],
   "app": {
     "background": {
-      "scripts": ["register_too_many_listeners.js"],
-      "persistent": true
+      "scripts": ["register_too_many_listeners.js"]
     }
   }
 }
diff --git a/chrome/test/data/extensions/manifest_tests/background_page_invalid_persistent_key_app.json b/chrome/test/data/extensions/manifest_tests/background_page_invalid_persistent_key_app.json
new file mode 100644
index 0000000..e79bd64
--- /dev/null
+++ b/chrome/test/data/extensions/manifest_tests/background_page_invalid_persistent_key_app.json
@@ -0,0 +1,13 @@
+{
+  "name": "test",
+  "version": "1",
+  "manifest_version": 2,
+  "background": {
+    "persistent": true
+  },
+  "app": {
+    "background": {
+      "page": "background_app/BackgroundApp.html"
+    }
+  }
+}
diff --git a/chrome/test/data/extensions/manifest_tests/background_page_persistent_app.json b/chrome/test/data/extensions/manifest_tests/background_page_persistent_app.json
new file mode 100644
index 0000000..00b5828
--- /dev/null
+++ b/chrome/test/data/extensions/manifest_tests/background_page_persistent_app.json
@@ -0,0 +1,11 @@
+{
+  "name": "test",
+  "version": "1",
+  "manifest_version": 2,
+  "app": {
+    "background": {
+      "page": "background_app/BackgroundApp.html",
+      "persistent": true
+    }
+  }
+}
diff --git a/extensions/common/manifest_constants.cc b/extensions/common/manifest_constants.cc
index 85775ef..51b5570 100644
--- a/extensions/common/manifest_constants.cc
+++ b/extensions/common/manifest_constants.cc
@@ -257,6 +257,8 @@ const char kAppsNotEnabled[] =
 const char kBackgroundPermissionNeeded[] =
     "Hosted apps that use 'background_page' must have the 'background' "
     "permission.";
+const char kBackgroundPersistentInvalidForPlatformApps[] =
+    "The key 'background.persistent' is not supported for packaged apps.";
 const char kBackgroundRequiredForPlatformApps[] =
     "Packaged apps must have a background page or background scripts.";
 const char kCannotAccessAboutUrl[] =
@@ -313,6 +315,9 @@ const char kInvalidBackgroundInHostedApp[] =
     "absolute HTTPS URL for the background page.";
 const char kInvalidBackgroundPersistent[] =
     "Invalid value for 'background.persistent'.";
+const char kInvalidBackgroundPersistentInPlatformApp[] =
+    "Invalid value for 'app.background.persistent'. Packaged apps do not "
+    "support persistent background pages and must use event pages.";
 const char kInvalidBackgroundPersistentNoPage[] =
     "Must specify one of background.page or background.scripts to use"
     " background.persistent.";
diff --git a/extensions/common/manifest_constants.h b/extensions/common/manifest_constants.h
index 81f8fc5..da756222 100644
--- a/extensions/common/manifest_constants.h
+++ b/extensions/common/manifest_constants.h
@@ -250,6 +250,7 @@ extern const char kActiveTabPermissionNotGranted[];
 extern const char kAllURLOrActiveTabNeeded[];
 extern const char kAppsNotEnabled[];
 extern const char kBackgroundPermissionNeeded[];
+extern const char kBackgroundPersistentInvalidForPlatformApps[];
 extern const char kBackgroundRequiredForPlatformApps[];
 extern const char kCannotAccessAboutUrl[];
 extern const char kCannotAccessChromeUrl[];
@@ -277,6 +278,7 @@ extern const char kInvalidBackgroundScript[];
 extern const char kInvalidBackgroundScripts[];
 extern const char kInvalidBackgroundInHostedApp[];
 extern const char kInvalidBackgroundPersistent[];
+extern const char kInvalidBackgroundPersistentInPlatformApp[];
 extern const char kInvalidBackgroundPersistentNoPage[];
 extern const char kInvalidBrowserAction[];
 extern const char kInvalidBrowseURL[];
diff --git a/extensions/common/manifest_handlers/background_info.cc b/extensions/common/manifest_handlers/background_info.cc
index 7263aae..c33d22c 100644
--- a/extensions/common/manifest_handlers/background_info.cc
+++ b/extensions/common/manifest_handlers/background_info.cc
@@ -297,12 +297,32 @@ bool BackgroundManifestHandler::Validate(
     const base::FilePath path = extension->GetResource(page_path).GetFilePath();
     if (path.empty() || !base::PathExists(path)) {
       *error =
-          l10n_util::GetStringFUTF8(
-              IDS_EXTENSION_LOAD_BACKGROUND_PAGE_FAILED,
-              page_path.LossyDisplayName());
+          l10n_util::GetStringFUTF8(IDS_EXTENSION_LOAD_BACKGROUND_PAGE_FAILED,
+                                    page_path.LossyDisplayName());
       return false;
     }
   }
+
+  if (extension->is_platform_app()) {
+    const std::string manifest_key =
+        std::string(keys::kPlatformAppBackground) + ".persistent";
+    bool is_persistent = false;
+    // Validate that packaged apps do not use a persistent background page.
+    if (extension->manifest()->GetBoolean(manifest_key, &is_persistent) &&
+        is_persistent) {
+      warnings->push_back(
+          InstallWarning(errors::kInvalidBackgroundPersistentInPlatformApp));
+    }
+    // Validate that packaged apps do not use the key 'background.persistent'.
+    // Use the dictionary directly to prevent an access check as
+    // 'background.persistent' is not available for packaged apps.
+    if (extension->manifest()->value()->Get(keys::kBackgroundPersistent,
+                                            NULL)) {
+      warnings->push_back(
+          InstallWarning(errors::kBackgroundPersistentInvalidForPlatformApps));
+    }
+  }
+
   return true;
 }