Extensions: Fix permission approvals across devices

When an extension is re-enabled through Sync, and the DISABLE_PERMISSIONS_INCREASE reason is removed, grant its permissions.

BUG=92626,296980

TBRing a trivial change (add a value to an enum) in histograms.xml.
TBR=asvitkine

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

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

9 files changed, 302 insertions, 36 deletions

diff --git a/chrome/browser/extensions/app_sync_data_unittest.cc b/chrome/browser/extensions/app_sync_data_unittest.cc
index ca4e7ef..0c0b28e 100644
--- a/chrome/browser/extensions/app_sync_data_unittest.cc
+++ b/chrome/browser/extensions/app_sync_data_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/extensions/app_sync_data.h"
 
+#include "extensions/common/extension.h"
 #include "sync/api/string_ordinal.h"
 #include "sync/protocol/app_specifics.pb.h"
 #include "sync/protocol/sync.pb.h"
@@ -15,6 +16,7 @@ const char kValidId[] = "abcdefghijklmnopabcdefghijklmnop";
 const char kName[] = "MyExtension";
 const char kValidVersion[] = "0.0.0.0";
 const char kValidUpdateUrl[] = "http://clients2.google.com/service/update2/crx";
+const int kValidDisableReasons = Extension::DISABLE_USER_ACTION;
 
 class AppSyncDataTest : public testing::Test {
  public:
@@ -27,7 +29,7 @@ class AppSyncDataTest : public testing::Test {
     extension_specifics->set_update_url(kValidUpdateUrl);
     extension_specifics->set_version(kValidVersion);
     extension_specifics->set_enabled(false);
-    extension_specifics->set_disable_reasons(0);
+    extension_specifics->set_disable_reasons(kValidDisableReasons);
     extension_specifics->set_incognito_enabled(true);
     extension_specifics->set_remote_install(false);
     extension_specifics->set_all_urls_enabled(true);
diff --git a/chrome/browser/extensions/extension_disabled_ui.cc b/chrome/browser/extensions/extension_disabled_ui.cc
index b66804f..de7f221 100644
--- a/chrome/browser/extensions/extension_disabled_ui.cc
+++ b/chrome/browser/extensions/extension_disabled_ui.cc
@@ -400,6 +400,9 @@ void ExtensionDisabledGlobalError::Observe(
   GlobalErrorServiceFactory::GetForProfile(service_->profile())->
       RemoveGlobalError(this);
 
+  // Make sure we don't call RemoveGlobalError again.
+  registrar_.RemoveAll();
+
   if (type == extensions::NOTIFICATION_EXTENSION_LOADED_DEPRECATED)
     user_response_ = REENABLE;
   else if (type == extensions::NOTIFICATION_EXTENSION_REMOVED)
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index c6e4779..07ab165 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -6488,18 +6488,14 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataNewExtension) {
           new syncer::FakeSyncChangeProcessor),
       scoped_ptr<syncer::SyncErrorFactory>(new syncer::SyncErrorFactoryMock()));
 
-  sync_pb::EntitySpecifics specifics;
-  sync_pb::ExtensionSpecifics* ext_specifics = specifics.mutable_extension();
-  ext_specifics->set_id(good_crx);
-  ext_specifics->set_version(base::Version("1").GetString());
-
   const base::FilePath path = data_dir().AppendASCII("good.crx");
   const ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
 
   struct TestCase {
     const char* name;  // For failure output only.
     bool sync_enabled;  // The "enabled" flag coming in from Sync.
-    int sync_disable_reasons;  // The disable reason(s) coming in from Sync.
+    // The disable reason(s) coming in from Sync, or -1 for "not set".
+    int sync_disable_reasons;
     // The disable reason(s) that should be set on the installed extension.
     // This will usually be the same as |sync_disable_reasons|, but see the
     // "Legacy" case.
@@ -6518,7 +6514,7 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataNewExtension) {
     // Legacy case (<M45): No disable reasons come in from Sync (see
     // crbug.com/484214). After installation, the reason should be set to
     // DISABLE_UNKNOWN_FROM_SYNC.
-    { "Legacy", false, 0, Extension::DISABLE_UNKNOWN_FROM_SYNC, true },
+    { "Legacy", false, -1, Extension::DISABLE_UNKNOWN_FROM_SYNC, true },
     // If the extension came in disabled due to a permissions increase, then the
     // user has *not* approved the permissions, and they shouldn't be granted.
     // crbug.com/484214
@@ -6529,16 +6525,20 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataNewExtension) {
   for (const TestCase& test_case : test_cases) {
     SCOPED_TRACE(test_case.name);
 
+    sync_pb::EntitySpecifics specifics;
+    sync_pb::ExtensionSpecifics* ext_specifics = specifics.mutable_extension();
+    ext_specifics->set_id(good_crx);
+    ext_specifics->set_version(base::Version("1").GetString());
     ext_specifics->set_enabled(test_case.sync_enabled);
-    ext_specifics->set_disable_reasons(test_case.sync_disable_reasons);
+    if (test_case.sync_disable_reasons != -1)
+      ext_specifics->set_disable_reasons(test_case.sync_disable_reasons);
 
     syncer::SyncData sync_data =
         syncer::SyncData::CreateLocalData(good_crx, "Name", specifics);
     syncer::SyncChange sync_change(FROM_HERE,
                                    syncer::SyncChange::ACTION_UPDATE,
                                    sync_data);
-    syncer::SyncChangeList list(1);
-    list[0] = sync_change;
+    syncer::SyncChangeList list(1, sync_change);
     extension_sync_service()->ProcessSyncChanges(FROM_HERE, list);
 
     ASSERT_TRUE(service()->pending_extension_manager()->IsIdPending(good_crx));
@@ -6708,6 +6708,225 @@ TEST_F(ExtensionServiceTest, ProcessSyncDataNotInstalled) {
   // TODO(akalin): Figure out a way to test |info.ShouldAllowInstall()|.
 }
 
+TEST_F(ExtensionServiceTest, ProcessSyncDataEnableDisable) {
+  InitializeEmptyExtensionService();
+  InitializeExtensionSyncService();
+  extension_sync_service()->MergeDataAndStartSyncing(
+      syncer::EXTENSIONS,
+      syncer::SyncDataList(),
+      scoped_ptr<syncer::SyncChangeProcessor>(
+          new syncer::FakeSyncChangeProcessor),
+      scoped_ptr<syncer::SyncErrorFactory>(new syncer::SyncErrorFactoryMock()));
+
+  const ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
+
+  struct TestCase {
+    const char* name;  // For failure output only.
+    // Set of disable reasons before any Sync data comes in. If this is != 0,
+    // the extension is disabled.
+    int previous_disable_reasons;
+    bool sync_enable;  // The enabled flag coming in from Sync.
+    // The disable reason(s) coming in from Sync, or -1 for "not set".
+    int sync_disable_reasons;
+    // The expected set of disable reasons after processing the Sync update. The
+    // extension should be disabled iff this is != 0.
+    int expect_disable_reasons;
+  } test_cases[] = {
+    { "NopEnable", 0, true, 0, 0 },
+    { "NopDisable", Extension::DISABLE_USER_ACTION, false,
+      Extension::DISABLE_USER_ACTION, Extension::DISABLE_USER_ACTION },
+    { "Disable", 0, false, Extension::DISABLE_USER_ACTION,
+      Extension::DISABLE_USER_ACTION },
+    { "DisableLegacy", 0, false, -1, Extension::DISABLE_UNKNOWN_FROM_SYNC },
+    { "AddDisableReason", Extension::DISABLE_REMOTE_INSTALL, false,
+      Extension::DISABLE_REMOTE_INSTALL | Extension::DISABLE_USER_ACTION,
+      Extension::DISABLE_REMOTE_INSTALL | Extension::DISABLE_USER_ACTION },
+    { "AddDisableReasonLegacy", Extension::DISABLE_USER_ACTION, false, -1,
+      Extension::DISABLE_USER_ACTION | Extension::DISABLE_UNKNOWN_FROM_SYNC},
+    { "RemoveDisableReason",
+      Extension::DISABLE_REMOTE_INSTALL | Extension::DISABLE_USER_ACTION, false,
+      Extension::DISABLE_USER_ACTION, Extension::DISABLE_USER_ACTION },
+    { "Enable", Extension::DISABLE_USER_ACTION, true, 0, 0 },
+    { "EnableLegacy", Extension::DISABLE_USER_ACTION, true, -1, 0 },
+  };
+
+  for (const TestCase& test_case : test_cases) {
+    SCOPED_TRACE(test_case.name);
+
+    std::string id;
+    std::string version;
+    // Don't keep |extension| around longer than necessary.
+    {
+      const Extension* extension =
+          InstallCRX(data_dir().AppendASCII("good.crx"), INSTALL_NEW);
+      // The extension should now be installed and enabled.
+      ASSERT_TRUE(extension);
+      id = extension->id();
+      version = extension->VersionString();
+    }
+    ASSERT_TRUE(registry()->enabled_extensions().Contains(id));
+
+    // Disable it if the test case says so.
+    if (test_case.previous_disable_reasons) {
+      service()->DisableExtension(id, test_case.previous_disable_reasons);
+      ASSERT_TRUE(registry()->disabled_extensions().Contains(id));
+    }
+
+    // Now a sync update comes in.
+    sync_pb::EntitySpecifics specifics;
+    sync_pb::ExtensionSpecifics* ext_specifics = specifics.mutable_extension();
+    ext_specifics->set_id(id);
+    ext_specifics->set_enabled(test_case.sync_enable);
+    ext_specifics->set_version(version);
+    if (test_case.sync_disable_reasons != -1)
+      ext_specifics->set_disable_reasons(test_case.sync_disable_reasons);
+
+    syncer::SyncData sync_data =
+        syncer::SyncData::CreateLocalData(good_crx, "Name", specifics);
+    syncer::SyncChange sync_change(FROM_HERE,
+                                   syncer::SyncChange::ACTION_UPDATE,
+                                   sync_data);
+    syncer::SyncChangeList list(1, sync_change);
+    extension_sync_service()->ProcessSyncChanges(FROM_HERE, list);
+
+    // Check expectations.
+    const bool expect_enabled = !test_case.expect_disable_reasons;
+    EXPECT_EQ(expect_enabled, service()->IsExtensionEnabled(id));
+    EXPECT_EQ(test_case.expect_disable_reasons, prefs->GetDisableReasons(id));
+
+    // Remove the extension again, so we can install it again for the next case.
+    UninstallExtension(id, false, expect_enabled ? Extension::ENABLED
+                                                 : Extension::DISABLED);
+  }
+}
+
+TEST_F(ExtensionServiceTest, ProcessSyncDataPermissionApproval) {
+  // This is the update URL specified in the test extension. Setting it here is
+  // necessary to make it considered syncable.
+  base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+      switches::kAppsGalleryUpdateURL,
+      "http://localhost/autoupdate/updates.xml");
+
+  InitializeEmptyExtensionService();
+  InitializeExtensionSyncService();
+  extension_sync_service()->MergeDataAndStartSyncing(
+      syncer::EXTENSIONS,
+      syncer::SyncDataList(),
+      scoped_ptr<syncer::SyncChangeProcessor>(
+          new syncer::FakeSyncChangeProcessor),
+      scoped_ptr<syncer::SyncErrorFactory>(new syncer::SyncErrorFactoryMock()));
+
+  const base::FilePath base_path =
+      data_dir().AppendASCII("permissions_increase");
+  const base::FilePath pem_path = base_path.AppendASCII("permissions.pem");
+  const base::FilePath path_v1 = base_path.AppendASCII("v1");
+  const base::FilePath path_v2 = base_path.AppendASCII("v2");
+
+  base::ScopedTempDir crx_dir;
+  ASSERT_TRUE(crx_dir.CreateUniqueTempDir());
+  const base::FilePath crx_path_v1 = crx_dir.path().AppendASCII("temp1.crx");
+  PackCRX(path_v1, pem_path, crx_path_v1);
+  const base::FilePath crx_path_v2 = crx_dir.path().AppendASCII("temp2.crx");
+  PackCRX(path_v2, pem_path, crx_path_v2);
+
+  const std::string v1("1");
+  const std::string v2("2");
+
+  const ExtensionPrefs* prefs = ExtensionPrefs::Get(profile());
+
+  struct TestCase {
+    const char* name;  // For failure output only.
+    const std::string& sync_version;  // The version coming in from Sync.
+    // The disable reason(s) coming in from Sync, or -1 for "not set".
+    int sync_disable_reasons;
+    // Whether the extension's permissions should be auto-granted.
+    bool expect_permissions_granted;
+  } test_cases[] = {
+    // Sync tells us to re-enable an older version. No permissions should be
+    // granted, since we can't be sure if the user actually approved the right
+    // set of permissions. Note that the extension will get disabled again the
+    // next time ExtensionService::CheckPermissionsIncrease runs because of the
+    // extra permissions.
+    { "OldVersion", v1, 0, false },
+    // Legacy case: Sync tells us to re-enable the extension, but doesn't
+    // specify disable reasons. No permissions should be granted.
+    { "Legacy", v2, -1, false },
+    // Sync tells us to re-enable the extension and explicitly removes the
+    // disable reasons. Now the extension should have its permissions granted.
+    { "GrantPermissions", v2, 0, true },
+  };
+
+  for (const TestCase& test_case : test_cases) {
+    SCOPED_TRACE(test_case.name);
+
+    std::string id;
+    // Don't keep |extension| around longer than necessary (it'll be destroyed
+    // during updating).
+    {
+      const Extension* extension = InstallCRX(crx_path_v1, INSTALL_NEW);
+      // The extension should now be installed and enabled.
+      ASSERT_TRUE(extension);
+      ASSERT_EQ(v1, extension->VersionString());
+      id = extension->id();
+    }
+    ASSERT_TRUE(registry()->enabled_extensions().Contains(id));
+
+    scoped_refptr<PermissionSet> granted_permissions_v1(
+        prefs->GetGrantedPermissions(id));
+
+    // Update to a new version with increased permissions.
+    UpdateExtension(id, crx_path_v2, DISABLED);
+
+    // Now the extension should be disabled due to a permissions increase.
+    {
+      const Extension* extension =
+          registry()->disabled_extensions().GetByID(id);
+      ASSERT_TRUE(extension);
+      ASSERT_EQ(v2, extension->VersionString());
+    }
+    ASSERT_TRUE(prefs->HasDisableReason(
+        id, Extension::DISABLE_PERMISSIONS_INCREASE));
+
+    // No new permissions should have been granted.
+    scoped_refptr<PermissionSet> granted_permissions_v2(
+        prefs->GetGrantedPermissions(id));
+    ASSERT_EQ(*granted_permissions_v1, *granted_permissions_v2);
+
+    // Now a sync update comes in.
+    sync_pb::EntitySpecifics specifics;
+    sync_pb::ExtensionSpecifics* ext_specifics = specifics.mutable_extension();
+    ext_specifics->set_id(id);
+    ext_specifics->set_enabled(true);
+    ext_specifics->set_version(test_case.sync_version);
+    if (test_case.sync_disable_reasons != -1)
+      ext_specifics->set_disable_reasons(test_case.sync_disable_reasons);
+
+    syncer::SyncData sync_data =
+        syncer::SyncData::CreateLocalData(good_crx, "Name", specifics);
+    syncer::SyncChange sync_change(FROM_HERE,
+                                   syncer::SyncChange::ACTION_UPDATE,
+                                   sync_data);
+    syncer::SyncChangeList list(1, sync_change);
+    extension_sync_service()->ProcessSyncChanges(FROM_HERE, list);
+
+    // Check expectations.
+    EXPECT_TRUE(registry()->GetExtensionById(id, ExtensionRegistry::ENABLED));
+    scoped_refptr<PermissionSet> granted_permissions(
+        prefs->GetGrantedPermissions(id));
+    if (test_case.expect_permissions_granted) {
+      scoped_refptr<PermissionSet> active_permissions(
+          prefs->GetActivePermissions(id));
+      EXPECT_EQ(*granted_permissions, *active_permissions);
+    } else {
+      EXPECT_EQ(*granted_permissions, *granted_permissions_v1);
+    }
+    EXPECT_EQ(Extension::DISABLE_NONE, prefs->GetDisableReasons(id));
+
+    // Remove the extension again, so we can install it again for the next case.
+    UninstallExtension(id, false);
+  }
+}
+
 #if defined(ENABLE_SUPERVISED_USERS)
 class ScopedSupervisedUserServiceDelegate
     : public SupervisedUserService::Delegate {
diff --git a/chrome/browser/extensions/extension_sync_data.cc b/chrome/browser/extensions/extension_sync_data.cc
index 6914bc1..17bb217 100644
--- a/chrome/browser/extensions/extension_sync_data.cc
+++ b/chrome/browser/extensions/extension_sync_data.cc
@@ -22,10 +22,13 @@ namespace {
 
 std::string GetExtensionSpecificsLogMessage(
     const sync_pb::ExtensionSpecifics& specifics) {
-  return base::StringPrintf("id: %s\nversion: %s\nupdate_url: %s",
-                            specifics.id().c_str(),
-                            specifics.version().c_str(),
-                            specifics.update_url().c_str());
+  return base::StringPrintf(
+      "id: %s\nversion: %s\nupdate_url: %s\nenabled: %i\ndisable_reasons: %i",
+      specifics.id().c_str(),
+      specifics.version().c_str(),
+      specifics.update_url().c_str(),
+      specifics.enabled(),
+      specifics.disable_reasons());
 }
 
 enum BadSyncDataReason {
@@ -41,6 +44,9 @@ enum BadSyncDataReason {
   // No ExtensionSpecifics in the EntitySpecifics.
   NO_EXTENSION_SPECIFICS,
 
+  // Enabled extensions can't have disable reasons.
+  BAD_DISABLE_REASONS,
+
   // Must be at the end.
   NUM_BAD_SYNC_DATA_REASONS
 };
@@ -55,6 +61,7 @@ void RecordBadSyncData(BadSyncDataReason reason) {
 ExtensionSyncData::ExtensionSyncData()
     : uninstalled_(false),
       enabled_(false),
+      supports_disable_reasons_(false),
       disable_reasons_(Extension::DISABLE_NONE),
       incognito_enabled_(false),
       remote_install_(false),
@@ -71,6 +78,7 @@ ExtensionSyncData::ExtensionSyncData(const Extension& extension,
     : id_(extension.id()),
       uninstalled_(false),
       enabled_(enabled),
+      supports_disable_reasons_(true),
       disable_reasons_(disable_reasons),
       incognito_enabled_(incognito_enabled),
       remote_install_(remote_install),
@@ -125,7 +133,8 @@ void ExtensionSyncData::PopulateExtensionSpecifics(
   specifics->set_update_url(update_url_.spec());
   specifics->set_version(version_.GetString());
   specifics->set_enabled(enabled_);
-  specifics->set_disable_reasons(disable_reasons_);
+  if (supports_disable_reasons_)
+    specifics->set_disable_reasons(disable_reasons_);
   specifics->set_incognito_enabled(incognito_enabled_);
   specifics->set_remote_install(remote_install_);
   if (all_urls_enabled_ != BOOLEAN_UNSET)
@@ -160,10 +169,22 @@ bool ExtensionSyncData::PopulateFromExtensionSpecifics(
     return false;
   }
 
+  // Enabled extensions can't have disable reasons. (The proto field may be
+  // unset, in which case it defaults to DISABLE_NONE.)
+  if (specifics.enabled() &&
+      specifics.disable_reasons() != Extension::DISABLE_NONE) {
+    LOG(ERROR) << "Attempt to sync bad ExtensionSpecifics "
+               << "(enabled extension can't have disable reasons):\n"
+               << GetExtensionSpecificsLogMessage(specifics);
+    RecordBadSyncData(BAD_DISABLE_REASONS);
+    return false;
+  }
+
   id_ = specifics.id();
   update_url_ = specifics_update_url;
   version_ = specifics_version;
   enabled_ = specifics.enabled();
+  supports_disable_reasons_ = specifics.has_disable_reasons();
   disable_reasons_ = specifics.disable_reasons();
   incognito_enabled_ = specifics.incognito_enabled();
   if (specifics.has_all_urls_enabled()) {
diff --git a/chrome/browser/extensions/extension_sync_data.h b/chrome/browser/extensions/extension_sync_data.h
index 5a94eab..03ce9bd 100644
--- a/chrome/browser/extensions/extension_sync_data.h
+++ b/chrome/browser/extensions/extension_sync_data.h
@@ -70,6 +70,7 @@ class ExtensionSyncData {
   // |version|).
   bool uninstalled() const { return uninstalled_; }
   bool enabled() const { return enabled_; }
+  bool supports_disable_reasons() const { return supports_disable_reasons_; }
   int disable_reasons() const { return disable_reasons_; }
   bool incognito_enabled() const { return incognito_enabled_; }
   bool remote_install() const { return remote_install_; }
@@ -90,6 +91,11 @@ class ExtensionSyncData {
   std::string id_;
   bool uninstalled_;
   bool enabled_;
+  // |supports_disable_reasons_| is true if the optional |disable_reasons_| was
+  // set to some value in the extension_specifics.proto. If not,
+  // |disable_reasons_| is given a default value and |supports_disable_reasons_|
+  // is false.
+  bool supports_disable_reasons_;
   int disable_reasons_;
   bool incognito_enabled_;
   bool remote_install_;
diff --git a/chrome/browser/extensions/extension_sync_service.cc b/chrome/browser/extensions/extension_sync_service.cc
index 12769c6..90f679f 100644
--- a/chrome/browser/extensions/extension_sync_service.cc
+++ b/chrome/browser/extensions/extension_sync_service.cc
@@ -491,8 +491,7 @@ bool ExtensionSyncService::ProcessExtensionSyncDataHelper(
   if (extension_sync_data.uninstalled()) {
     if (!extension_service_->UninstallExtensionHelper(
             extension_service_, id, extensions::UNINSTALL_REASON_SYNC)) {
-      LOG(WARNING) << "Could not uninstall extension " << id
-                   << " for sync";
+      LOG(WARNING) << "Could not uninstall extension " << id << " for sync";
     }
     return true;
   }
@@ -507,33 +506,43 @@ bool ExtensionSyncService::ProcessExtensionSyncDataHelper(
   }
 
   // Set user settings.
-  // If the extension has been disabled from sync, it may not have
-  // been installed yet, so we don't know if the disable reason was a
-  // permissions increase.  That will be updated once CheckPermissionsIncrease
-  // is called for it.
-  // However if the extension is marked as a remote install in sync, we know
-  // what the disable reason is, so set it to that directly. Note that when
-  // CheckPermissionsIncrease runs, it might still add permissions increase
-  // as a disable reason for the extension.
   if (extension_sync_data.enabled()) {
-    extension_service_->EnableExtension(id);
+    // Only grant permissions if the sync data explicitly sets the disable
+    // reasons to Extension::DISABLE_NONE (as opposed to the legacy (<M45) case
+    // where they're not set at all), and if the version from sync matches our
+    // local one. Otherwise we just enable it without granting permissions. If
+    // any permissions are missing, CheckPermissionsIncrease will soon disable
+    // it again.
+    DCHECK(!extension_sync_data.disable_reasons());
+    bool grant_permissions =
+        extension_sync_data.supports_disable_reasons() &&
+        extension &&
+        extension->version()->Equals(extension_sync_data.version());
+    if (grant_permissions)
+      extension_service_->GrantPermissionsAndEnableExtension(extension);
+    else
+      extension_service_->EnableExtension(id);
   } else if (!IsPendingEnable(id)) {
     int disable_reasons = extension_sync_data.disable_reasons();
     if (extension_sync_data.remote_install()) {
-      // In the non-legacy case (>=M45) where disable reasons are synced at all,
-      // DISABLE_REMOTE_INSTALL should be among them already.
-      DCHECK(!disable_reasons ||
-             (disable_reasons & Extension::DISABLE_REMOTE_INSTALL));
-      disable_reasons |= Extension::DISABLE_REMOTE_INSTALL;
-    }
-    if (!disable_reasons) {
+      if (!(disable_reasons & Extension::DISABLE_REMOTE_INSTALL)) {
+        // In the non-legacy case (>=M45) where disable reasons are synced at
+        // all, DISABLE_REMOTE_INSTALL should be among them already.
+        DCHECK(!extension_sync_data.supports_disable_reasons());
+        disable_reasons |= Extension::DISABLE_REMOTE_INSTALL;
+      }
+    } else if (!extension_sync_data.supports_disable_reasons()) {
       // Legacy case (<M45), from before we synced disable reasons (see
       // crbug.com/484214).
       disable_reasons = Extension::DISABLE_UNKNOWN_FROM_SYNC;
     }
 
-    extension_service_->DisableExtension(
-        id, Extension::DisableReason(disable_reasons));
+    // In the non-legacy case (>=M45), clear any existing disable reasons first.
+    // Otherwise sync can't remove just some of them.
+    if (extension_sync_data.supports_disable_reasons())
+      extensions::ExtensionPrefs::Get(profile_)->ClearDisableReasons(id);
+
+    extension_service_->DisableExtension(id, disable_reasons);
   }
 
   // We need to cache some version information here because setting the
diff --git a/extensions/common/permissions/permission_set.cc b/extensions/common/permissions/permission_set.cc
index 65236ac..f987411 100644
--- a/extensions/common/permissions/permission_set.cc
+++ b/extensions/common/permissions/permission_set.cc
@@ -141,6 +141,10 @@ bool PermissionSet::operator==(
       explicit_hosts_ == rhs.explicit_hosts_;
 }
 
+bool PermissionSet::operator!=(const PermissionSet& rhs) const {
+  return !(*this == rhs);
+}
+
 bool PermissionSet::Contains(const PermissionSet& set) const {
   return apis_.Contains(set.apis()) &&
          manifest_permissions_.Contains(set.manifest_permissions()) &&
diff --git a/extensions/common/permissions/permission_set.h b/extensions/common/permissions/permission_set.h
index 5321eda..e7b5311 100644
--- a/extensions/common/permissions/permission_set.h
+++ b/extensions/common/permissions/permission_set.h
@@ -56,6 +56,7 @@ class PermissionSet
       const PermissionSet* set1, const PermissionSet* set2);
 
   bool operator==(const PermissionSet& rhs) const;
+  bool operator!=(const PermissionSet& rhs) const;
 
   // Returns true if every API or host permission available to |set| is also
   // available to this. In other words, if the API permissions of |set| are a
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index b80b731..a9c18ac 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -49611,6 +49611,7 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   <int value="1" label="Bad version"/>
   <int value="2" label="Bad update URL"/>
   <int value="3" label="No ExtensionSpecifics"/>
+  <int value="4" label="Bad disable reasons"/>
 </enum>
 
 <enum name="BaseRelocationType" type="int">