Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2019423007:
Re-enable extensions disabled due to permission increase if they have all permissions (Closed)
|
Created:
4 years, 6 months ago by Marc Treib Modified:
4 years, 6 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@ext_update_test Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRe-enable extensions disabled due to permission increase if they have all permissions
BUG=616474
Committed: https://crrev.com/6e51bca1d4a7213bbf926104457eb3e2a22f6999
Cr-Commit-Position: refs/heads/master@{#399876}
Patch Set 1 #
Total comments: 10
Patch Set 2 : fix startup, add tests #
Total comments: 9
Patch Set 3 : review #Patch Set 4 : is_extension_installed -> is_extension_loaded #
Total comments: 2
Patch Set 5 : histogram nit #
Messages
Total messages: 26 (7 generated)
rdevlin.cronin@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
https://codereview.chromium.org/2019423007/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/2019423007/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service_unittest.cc:1525: TEST_F(ExtensionServiceTest, ReenableWithAllPermissionsGranted) { How does this differ from the last test? https://codereview.chromium.org/2019423007/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service_unittest.cc:1616: EXPECT_TRUE(prefs->HasDisableReason(id, Extension::DISABLE_USER_ACTION)); Could even just EXPECT_EQ() here, right? https://codereview.chromium.org/2019423007/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service_unittest.cc:1617: } How hard would it be to add a test that checks the extension gets re-enabled (if it has no more permissions increases) on startup (in addition to when the extension pushes an update?)
https://codereview.chromium.org/2019423007/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2019423007/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.cc:1645: // fact have all permissions, remove that disable reason. Might also be worth adding a TODO to decide whether or not we want this as default behavior, or just for a couple of mstones.
https://codereview.chromium.org/2019423007/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2019423007/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.cc:1645: // fact have all permissions, remove that disable reason. On 2016/06/02 16:06:45, Devlin wrote: > Might also be worth adding a TODO to decide whether or not we want this as > default behavior, or just for a couple of mstones. Yup, that's the main reason I hadn't actually sent this for review :) We haven't really decided if this is the right thing to do. But that's your decision ;) https://codereview.chromium.org/2019423007/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/2019423007/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service_unittest.cc:1525: TEST_F(ExtensionServiceTest, ReenableWithAllPermissionsGranted) { On 2016/06/02 16:05:55, Devlin wrote: > How does this differ from the last test? Right - this is essentially the same as the "version 5" part of the previous test. But that one is focused on granted permissions, and I'd kinda like a separate test for the re-enabling. Maybe just remove the newly-added part of the previous test? https://codereview.chromium.org/2019423007/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service_unittest.cc:1616: EXPECT_TRUE(prefs->HasDisableReason(id, Extension::DISABLE_USER_ACTION)); On 2016/06/02 16:05:55, Devlin wrote: > Could even just EXPECT_EQ() here, right? Sure, done. https://codereview.chromium.org/2019423007/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service_unittest.cc:1617: } On 2016/06/02 16:05:55, Devlin wrote: > How hard would it be to add a test that checks the extension gets re-enabled (if > it has no more permissions increases) on startup (in addition to when the > extension pushes an update?) Shouldn't be too hard - I guess we can simulate a restart by recreating the extensions service. I'll take a look tomorrow.
I see "done"s, but no updated patch set? https://codereview.chromium.org/2019423007/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2019423007/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.cc:1645: // fact have all permissions, remove that disable reason. On 2016/06/02 16:36:14, Marc Treib wrote: > On 2016/06/02 16:06:45, Devlin wrote: > > Might also be worth adding a TODO to decide whether or not we want this as > > default behavior, or just for a couple of mstones. > > Yup, that's the main reason I hadn't actually sent this for review :) > We haven't really decided if this is the right thing to do. But that's your > decision ;) Whoops, sorry for jumping the gun. Looked from the crbug, and didn't realize it wasn't actually published. My bad!
On 2016/06/03 15:26:58, Devlin wrote: > I see "done"s, but no updated patch set? Ah, sorry - actually only one trivial "done", so I didn't bother uploading a new patch set. I'll do that once I've added the "startup" test and ping you again. Thanks and sorry for the confusion!
PTAL - I've added tests for the "startup" scenario, and they actually uncovered a problem! See comment below. https://codereview.chromium.org/2019423007/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/2019423007/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service_unittest.cc:1525: TEST_F(ExtensionServiceTest, ReenableWithAllPermissionsGranted) { On 2016/06/02 16:36:14, Marc Treib wrote: > On 2016/06/02 16:05:55, Devlin wrote: > > How does this differ from the last test? > > Right - this is essentially the same as the "version 5" part of the previous > test. But that one is focused on granted permissions, and I'd kinda like a > separate test for the re-enabling. Maybe just remove the newly-added part of the > previous test? Done. (Removed the added part of the above test again) https://codereview.chromium.org/2019423007/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2019423007/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1639: // fact have all permissions, remove that disable reason. To make the "startup" case work, I had to pull this out of the "if (is_extension_installed)" - turns out |is_extension_installed| is false during startup, even if the extension was previously installed. Misleading names ftw :-/ In fact, I can't see a reason why this check is even there at all. Removing it doesn't seem to break anything, though from experience, that doesn't mean much...
mostly lg, but would like to see some UMA added https://codereview.chromium.org/2019423007/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2019423007/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1639: // fact have all permissions, remove that disable reason. On 2016/06/10 14:12:17, Marc Treib wrote: > To make the "startup" case work, I had to pull this out of the "if > (is_extension_installed)" - turns out |is_extension_installed| is false during > startup, even if the extension was previously installed. Misleading names ftw > :-/ > In fact, I can't see a reason why this check is even there at all. Removing it > doesn't seem to break anything, though from experience, that doesn't mean > much... Can you add a TODO to rename it on line 1442? Something like: // TODO([devlin|treib]): This is incorrect during startup, where the extension isn't already loaded, but may have been installed. (option b: rename it in this CL - up to you ;)) https://codereview.chromium.org/2019423007/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1640: if (previously_disabled && !is_privilege_increase && Add: // TODO(devlin): This was added to fix crbug.com/616474, but it's unclear // if this behavior should stay forever. https://codereview.chromium.org/2019423007/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1644: extension->id(), Extension::DISABLE_PERMISSIONS_INCREASE); I wonder if we should add some UMA for this. I'm expecting this to be relatively rare, but if we see ourselves re-enabling bunches of extensions, it'd at least be good to know.
https://codereview.chromium.org/2019423007/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2019423007/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1639: // fact have all permissions, remove that disable reason. On 2016/06/10 19:05:52, Devlin wrote: > On 2016/06/10 14:12:17, Marc Treib wrote: > > To make the "startup" case work, I had to pull this out of the "if > > (is_extension_installed)" - turns out |is_extension_installed| is false during > > startup, even if the extension was previously installed. Misleading names ftw > > :-/ > > In fact, I can't see a reason why this check is even there at all. Removing it > > doesn't seem to break anything, though from experience, that doesn't mean > > much... > > Can you add a TODO to rename it on line 1442? Something like: > // TODO([devlin|treib]): This is incorrect during startup, where the extension > isn't already loaded, but may have been installed. Done. I also added a TODO in CheckPermissionsIncrease, (a) to explain the same thing, a (2) since I think we don't actually need it there. > (option b: rename it in this CL - up to you ;)) Sure, I can do that - what would be a good name? is_extension_loaded? https://codereview.chromium.org/2019423007/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1640: if (previously_disabled && !is_privilege_increase && On 2016/06/10 19:05:52, Devlin wrote: > Add: > // TODO(devlin): This was added to fix crbug.com/616474, but it's unclear > // if this behavior should stay forever. Done. https://codereview.chromium.org/2019423007/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1644: extension->id(), Extension::DISABLE_PERMISSIONS_INCREASE); On 2016/06/10 19:05:52, Devlin wrote: > I wonder if we should add some UMA for this. I'm expecting this to be > relatively rare, but if we see ourselves re-enabling bunches of extensions, it'd > at least be good to know. Added a binary histogram. PTAL.
lgtm https://codereview.chromium.org/2019423007/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2019423007/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1639: // fact have all permissions, remove that disable reason. On 2016/06/13 11:54:59, Marc Treib wrote: > On 2016/06/10 19:05:52, Devlin wrote: > > On 2016/06/10 14:12:17, Marc Treib wrote: > > > To make the "startup" case work, I had to pull this out of the "if > > > (is_extension_installed)" - turns out |is_extension_installed| is false > during > > > startup, even if the extension was previously installed. Misleading names > ftw > > > :-/ > > > In fact, I can't see a reason why this check is even there at all. Removing > it > > > doesn't seem to break anything, though from experience, that doesn't mean > > > much... > > > > Can you add a TODO to rename it on line 1442? Something like: > > // TODO([devlin|treib]): This is incorrect during startup, where the extension > > isn't already loaded, but may have been installed. > > Done. > I also added a TODO in CheckPermissionsIncrease, (a) to explain the same thing, > a (2) since I think we don't actually need it there. > > > (option b: rename it in this CL - up to you ;)) > > Sure, I can do that - what would be a good name? is_extension_loaded? is_extension_loaded sgtm
https://codereview.chromium.org/2019423007/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2019423007/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1639: // fact have all permissions, remove that disable reason. On 2016/06/13 13:51:50, Devlin wrote: > On 2016/06/13 11:54:59, Marc Treib wrote: > > On 2016/06/10 19:05:52, Devlin wrote: > > > On 2016/06/10 14:12:17, Marc Treib wrote: > > > > To make the "startup" case work, I had to pull this out of the "if > > > > (is_extension_installed)" - turns out |is_extension_installed| is false > > during > > > > startup, even if the extension was previously installed. Misleading names > > ftw > > > > :-/ > > > > In fact, I can't see a reason why this check is even there at all. > Removing > > it > > > > doesn't seem to break anything, though from experience, that doesn't mean > > > > much... > > > > > > Can you add a TODO to rename it on line 1442? Something like: > > > // TODO([devlin|treib]): This is incorrect during startup, where the > extension > > > isn't already loaded, but may have been installed. > > > > Done. > > I also added a TODO in CheckPermissionsIncrease, (a) to explain the same > thing, > > a (2) since I think we don't actually need it there. > > > > > (option b: rename it in this CL - up to you ;)) > > > > Sure, I can do that - what would be a good name? is_extension_loaded? > > is_extension_loaded sgtm Done, and removed/updated the TODOs accordingly.
treib@chromium.org changed reviewers: + holte@chromium.org
+holte for histograms - PTAL!
histograms.xml lgtm % nit https://codereview.chromium.org/2019423007/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2019423007/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:15449: +<histogram name="Extensions.ResetPermissionsIncrease" units="boolean"> This should be enum="Boolean" instead of units="boolean".
https://codereview.chromium.org/2019423007/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2019423007/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:15449: +<histogram name="Extensions.ResetPermissionsIncrease" units="boolean"> On 2016/06/15 02:46:25, Steven Holte wrote: > This should be enum="Boolean" instead of units="boolean". Done.
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2019423007/#ps80001 (title: "histogram nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2019423007/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2019423007/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Re-enable extensions disabled due to permission increase if they have all permissions BUG=616474 ========== to ========== Re-enable extensions disabled due to permission increase if they have all permissions BUG=616474 Committed: https://crrev.com/6e51bca1d4a7213bbf926104457eb3e2a22f6999 Cr-Commit-Position: refs/heads/master@{#399876} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6e51bca1d4a7213bbf926104457eb3e2a22f6999 Cr-Commit-Position: refs/heads/master@{#399876} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
