Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2495123003:
Continue attempts to reinstall corrupt policy extensions across restarts (Closed)
|
Created:
4 years, 1 month ago by asargent_no_longer_on_chrome Modified:
4 years ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, stgao Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionContinue attempts to reinstall corrupt policy extensions across restarts
If a policy force-installed extension gets corrupted, we'll disable it
and start attempting to reinstall it. However, if chrome shuts down
before this is complete, we won't start trying again on any subsequent
run of the browser.
This also switches from using the ManagementPolicy :: Provider ::
UserMayModifySettings function to determine "policy force installed-ess"
to using ManagementPolicy :: Provider :: MustRemainEnabled instead, which
is more correct for the case of force installs (eg
"ExtensionInstallForcelist" in the policy settings).
BUG=664690
Committed: https://crrev.com/0de8a8bc49761792dc5a74f001395cdd11ddc4e6
Cr-Commit-Position: refs/heads/master@{#434012}
Patch Set 1 #Patch Set 2 : fix tests on chromeos #Patch Set 3 : cleanup #
Total comments: 8
Patch Set 4 : review feedback fixes #Patch Set 5 : merged latest from origin/master #
Messages
Total messages: 41 (28 generated)
The CQ bit was checked by asargent@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by asargent@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by asargent@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
asargent@chromium.org changed reviewers: + lazyboy@chromium.org
lazyboy: can you please review? The functional change is in chrome/browser/extensions/installed_loader.cc, and most of the rest of the CL is test changes (and a little lint cleanup).
lgtm with some comments. One concern to think about there. Also nit in CL description: s/Provder/Provider https://codereview.chromium.org/2495123003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/content_verifier_browsertest.cc (right): https://codereview.chromium.org/2495123003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/content_verifier_browsertest.cc:430: return extension->id() != id_; Should this be id() == id_ instead? We don't want extension |id_| to be disabled, right? https://codereview.chromium.org/2495123003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/installed_loader.cc (right): https://codereview.chromium.org/2495123003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/installed_loader.cc:8: #include <memory> Do you need these includes? I don't see any new code using them. https://codereview.chromium.org/2495123003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/installed_loader.cc:225: PendingExtensionManager* pending_manager = I'd add a note saying sth that explains this, as it's harder to figure out what's going on: If we shut down last time while corrupted policy force extension were being reinstalled and but the reinstallation didn't succeed/finish, retry. I also wonder if some clients can get into a state where this keeps happening on every startup, maybe because the policy has changed to not include |extension| anymore. Can that even happen? Just curious.
Description was changed from ========== Continue attempts to reinstall corrupt policy extensions across restarts If a policy force-installed extension gets corrupted, we'll disable it and start attempting to reinstall it. However, if chrome shuts down before this is complete, we won't start trying again on any subsequent run of the browser. This also switches from using the ManagementPolicy :: Provder :: UserMayModifySettings function to determine "policy force installed-ess" to using ManagementPolicy :: Provder :: MustRemainEnabled instead, which is more correct for the case of force installs (eg "ExtensionInstallForcelist" in the policy settings). BUG=664690 ========== to ========== Continue attempts to reinstall corrupt policy extensions across restarts If a policy force-installed extension gets corrupted, we'll disable it and start attempting to reinstall it. However, if chrome shuts down before this is complete, we won't start trying again on any subsequent run of the browser. This also switches from using the ManagementPolicy :: Provider :: UserMayModifySettings function to determine "policy force installed-ess" to using ManagementPolicy :: Provider :: MustRemainEnabled instead, which is more correct for the case of force installs (eg "ExtensionInstallForcelist" in the policy settings). BUG=664690 ==========
The CQ bit was checked by asargent@chromium.org to run a CQ dry run
CL description fixed. https://codereview.chromium.org/2495123003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/content_verifier_browsertest.cc (right): https://codereview.chromium.org/2495123003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/content_verifier_browsertest.cc:430: return extension->id() != id_; On 2016/11/16 01:44:53, lazyboy wrote: > Should this be id() == id_ instead? We don't want extension |id_| to be > disabled, right? Good catch - it took me a while to understand why this wasn't causing the test to fail. It turns out that because the extension gets installed with a location of EXTERNAL_POLICY_DOWNLOAD, the StandardManagementPolicy::MustRemainEnabled function returns true before my function here ever gets called (ManagementPolicy::ApplyToProviderList early-exits if any registered ManagementPolicy::Provider returns non-default value). Anyhow, fixed. https://codereview.chromium.org/2495123003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/installed_loader.cc (right): https://codereview.chromium.org/2495123003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/installed_loader.cc:8: #include <memory> On 2016/11/16 01:44:53, lazyboy wrote: > Do you need these includes? I don't see any new code using them. I'm just fixing lint errors since I'm modifying the file anyway (I try to be in the habit of running 'git cl lint' on my CLs and fixing any problems that show whether related to my changes or not) https://codereview.chromium.org/2495123003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/installed_loader.cc:225: PendingExtensionManager* pending_manager = On 2016/11/16 01:44:53, lazyboy wrote: > I'd add a note saying sth that explains this, as it's harder to figure out > what's going on: > If we shut down last time while corrupted policy force extension were being > reinstalled and but the reinstallation didn't succeed/finish, retry. Good feedback - I've added a comment. > I also wonder if some clients can get into a state where this keeps happening on > every startup, maybe because the policy has changed to not include |extension| > anymore. Can that even happen? Just curious. Good question - I think we are safe in this case. If the policy changes to not include the extension anymore, it will get uninstalled by ExtensionService::OnAllExternalProvidersReady (it iterates over all extensions, and any with an "external" location that aren't provided by any of the ExternalProvider's gets uninstalled).
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
asargent@chromium.org changed reviewers: + xiyuan@chromium.org
xiyuan: please review the two changes in chrome/browser/chromeos/* (I've just moved the two copies of the CreateAndInitializeLocalCache helper function into the shared location of chrome/browser/extensions/browsertest_util.h)
c/b/chromeos/* lgtm
slgtm https://codereview.chromium.org/2495123003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/installed_loader.cc (right): https://codereview.chromium.org/2495123003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/installed_loader.cc:8: #include <memory> On 2016/11/17 21:48:01, Antony Sargent wrote: > On 2016/11/16 01:44:53, lazyboy wrote: > > Do you need these includes? I don't see any new code using them. > > I'm just fixing lint errors since I'm modifying the file anyway (I try to be in > the habit of running 'git cl lint' on my CLs and fixing any problems that show > whether related to my changes or not) Good to know, thanks. https://codereview.chromium.org/2495123003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/installed_loader.cc:225: PendingExtensionManager* pending_manager = On 2016/11/17 21:48:01, Antony Sargent wrote: > On 2016/11/16 01:44:53, lazyboy wrote: > > I'd add a note saying sth that explains this, as it's harder to figure out > > what's going on: > > If we shut down last time while corrupted policy force extension were being > > reinstalled and but the reinstallation didn't succeed/finish, retry. > > Good feedback - I've added a comment. > > > > I also wonder if some clients can get into a state where this keeps happening > on > > every startup, maybe because the policy has changed to not include |extension| > > anymore. Can that even happen? Just curious. > > Good question - I think we are safe in this case. If the policy changes to not > include the extension anymore, it will get uninstalled by > ExtensionService::OnAllExternalProvidersReady (it iterates over all extensions, > and any with an "external" location that aren't provided by any of the > ExternalProvider's gets uninstalled). Acknowledged.
The CQ bit was checked by asargent@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2495123003/#ps60001 (title: "review feedback fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was checked by asargent@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2495123003/#ps80001 (title: "merged latest from origin/master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1479849081182000,
"parent_rev": "abd7cb80c5d576cb281151a4cc0cc38bdca98dd8", "commit_rev":
"9c9e3c3e505c92b3406d80da0ae454f77b37c1fd"}
Message was sent while issue was closed.
Description was changed from ========== Continue attempts to reinstall corrupt policy extensions across restarts If a policy force-installed extension gets corrupted, we'll disable it and start attempting to reinstall it. However, if chrome shuts down before this is complete, we won't start trying again on any subsequent run of the browser. This also switches from using the ManagementPolicy :: Provider :: UserMayModifySettings function to determine "policy force installed-ess" to using ManagementPolicy :: Provider :: MustRemainEnabled instead, which is more correct for the case of force installs (eg "ExtensionInstallForcelist" in the policy settings). BUG=664690 ========== to ========== Continue attempts to reinstall corrupt policy extensions across restarts If a policy force-installed extension gets corrupted, we'll disable it and start attempting to reinstall it. However, if chrome shuts down before this is complete, we won't start trying again on any subsequent run of the browser. This also switches from using the ManagementPolicy :: Provider :: UserMayModifySettings function to determine "policy force installed-ess" to using ManagementPolicy :: Provider :: MustRemainEnabled instead, which is more correct for the case of force installs (eg "ExtensionInstallForcelist" in the policy settings). BUG=664690 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Continue attempts to reinstall corrupt policy extensions across restarts If a policy force-installed extension gets corrupted, we'll disable it and start attempting to reinstall it. However, if chrome shuts down before this is complete, we won't start trying again on any subsequent run of the browser. This also switches from using the ManagementPolicy :: Provider :: UserMayModifySettings function to determine "policy force installed-ess" to using ManagementPolicy :: Provider :: MustRemainEnabled instead, which is more correct for the case of force installs (eg "ExtensionInstallForcelist" in the policy settings). BUG=664690 ========== to ========== Continue attempts to reinstall corrupt policy extensions across restarts If a policy force-installed extension gets corrupted, we'll disable it and start attempting to reinstall it. However, if chrome shuts down before this is complete, we won't start trying again on any subsequent run of the browser. This also switches from using the ManagementPolicy :: Provider :: UserMayModifySettings function to determine "policy force installed-ess" to using ManagementPolicy :: Provider :: MustRemainEnabled instead, which is more correct for the case of force installs (eg "ExtensionInstallForcelist" in the policy settings). BUG=664690 Committed: https://crrev.com/0de8a8bc49761792dc5a74f001395cdd11ddc4e6 Cr-Commit-Position: refs/heads/master@{#434012} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0de8a8bc49761792dc5a74f001395cdd11ddc4e6 Cr-Commit-Position: refs/heads/master@{#434012}
Message was sent while issue was closed.
stgao@chromium.org changed reviewers: + stgao@chromium.org
Message was sent while issue was closed.
FYI: this CL seems to introduce a new flaky test ContentVerifierPolicyTest.PolicyCorruptedOnStartup https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium.... One occurrence on Waterfall is: https://build.chromium.org/p/chromium.mac/builders/Mac10.11%20Tests/builds/39...
Message was sent while issue was closed.
Description was changed from ========== Continue attempts to reinstall corrupt policy extensions across restarts If a policy force-installed extension gets corrupted, we'll disable it and start attempting to reinstall it. However, if chrome shuts down before this is complete, we won't start trying again on any subsequent run of the browser. This also switches from using the ManagementPolicy :: Provider :: UserMayModifySettings function to determine "policy force installed-ess" to using ManagementPolicy :: Provider :: MustRemainEnabled instead, which is more correct for the case of force installs (eg "ExtensionInstallForcelist" in the policy settings). BUG=664690 Committed: https://crrev.com/0de8a8bc49761792dc5a74f001395cdd11ddc4e6 Cr-Commit-Position: refs/heads/master@{#434012} ========== to ========== Continue attempts to reinstall corrupt policy extensions across restarts If a policy force-installed extension gets corrupted, we'll disable it and start attempting to reinstall it. However, if chrome shuts down before this is complete, we won't start trying again on any subsequent run of the browser. This also switches from using the ManagementPolicy :: Provider :: UserMayModifySettings function to determine "policy force installed-ess" to using ManagementPolicy :: Provider :: MustRemainEnabled instead, which is more correct for the case of force installs (eg "ExtensionInstallForcelist" in the policy settings). BUG=664690 Committed: https://crrev.com/0de8a8bc49761792dc5a74f001395cdd11ddc4e6 Cr-Commit-Position: refs/heads/master@{#434012} ==========
Message was sent while issue was closed.
stgao@chromium.org changed reviewers: - stgao@chromium.org
Message was sent while issue was closed.
On 2016/11/28 20:05:13, stgao (slow on Monday) wrote: > FYI: this CL seems to introduce a new flaky test > ContentVerifierPolicyTest.PolicyCorruptedOnStartup > > https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium.... > > One occurrence on Waterfall is: > https://build.chromium.org/p/chromium.mac/builders/Mac10.11%20Tests/builds/39... I already disabled the test with: https://codereview.chromium.org/2527693003/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
