This might actually be easier than expected. We'll see :)

Lazyboy was also thinking of looking into this at one point.  I'll let you two coordinate. :)

There are a couple of open questions here, though.  For instance, we should do this for now because adding permissions incorrectly disabled some extensions, but it's unclear (to me) whether it's the right course forever.  It could be nice to let extension authors backpedal if they add a permission that results in them being disabled, but it might also be the wrong incentive.

I didn't look into it at all after we brought this up, seems treib@ is already up to it, nice!

I'd also be wary about making this a generic solution (same as Devlin's concern @2), though. We might get reports of zombie extensions coming back to life.

I'd give my 2c: a) we could detect that the disable (exclusively because of DISABLE_PERMISSIONS_INCREASE) happened due to notifications permission and b) keep the fix for few milestones and revert.
I don't think a) is possible?

But overall, fixing these extensions make me happy 8c)

right, a) is impossible, unfortunately.

Prototype CL (which re-enables in all cases, not just for kNotifications):
https://codereview.chromium.org/2019423007/

Actually, a) might not be impossible.  We still store the granted permissions, so we'd just check whether or not the only delta is for notifications.

But if the extension has already removed that permission in the meantime (which at least one of the affected extensions did), there will be no delta.

Correct.

Just checking, there is no nightly available with the fix proopsal, correct?

I am interested in testing if the proposed fix addresses symptoms of  bug 619759 , which looks to me like some particular impact of Chromium 51 on a particular extension using search_provider setting override.

No nightly build, since the fix hasn't landed yet. It will hopefully land within a day or two, though.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6e51bca1d4a7213bbf926104457eb3e2a22f6999

commit 6e51bca1d4a7213bbf926104457eb3e2a22f6999
Author: treib <treib@chromium.org>
Date: Wed Jun 15 10:26:03 2016

Re-enable extensions disabled due to permission increase if they have all permissions

BUG= 616474 

Review-Url: https://codereview.chromium.org/2019423007
Cr-Commit-Position: refs/heads/master@{#399876}

[modify] https://crrev.com/6e51bca1d4a7213bbf926104457eb3e2a22f6999/chrome/browser/extensions/extension_service.cc
[modify] https://crrev.com/6e51bca1d4a7213bbf926104457eb3e2a22f6999/chrome/browser/extensions/extension_service.h
[modify] https://crrev.com/6e51bca1d4a7213bbf926104457eb3e2a22f6999/chrome/browser/extensions/extension_service_unittest.cc
[add] https://crrev.com/6e51bca1d4a7213bbf926104457eb3e2a22f6999/chrome/test/data/extensions/permissions/update_5/manifest.json
[modify] https://crrev.com/6e51bca1d4a7213bbf926104457eb3e2a22f6999/tools/metrics/histograms/histograms.xml

Devlin: This should probably be merged to M52, WDYT?

re #9: The fix should be in tomorrow's Canary release.

Your change meets the bar and is auto-approved for M52 (branch: 2743)

treib@, can you please let us know if this can be tested manually so that we can verify it on Canary ?

PS: Please also get this merged in to M52 branch once it is verified on Canary before 6/22 so that it gets picked for Beta promotion scheduled next week.

I've attached a test extension that allows to manually verify this change.

Instructions:

Install permissions_1.crx (by drag&dropping the file onto chrome://extensions). You'll see a dialog asking you to confirm the "Read your history" permission. Approve. You should now see an extension called "Permissions Update Test", and it should be enabled.

Update to permissions_2.crx (again by drag&dropping). You'll get a dialog asking to you approve the changed permissions (it added "Read your accessibility settings"). Click "Cancel". The extension should now be disabled.

Update to permissions_3.crx (again by drag&dropping). You should *not* get a dialog; the extension should silently get enabled again.
(Without the fix, you would get another dialog here, asking you to approve "Read your history" again.)

I hope this is clear; please let me know if there are any other questions!

Deleted: permissions_1.crx 518 bytes

permissions_1.crx 518 bytes Download

Deleted: permissions_2.crx 544 bytes

permissions_2.crx 544 bytes Download

Deleted: permissions_3.crx 518 bytes

permissions_3.crx 518 bytes Download

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Verified the issue by following the steps provided in comment # 16 on Mac 10.11.5,Win 7 and Ubuntu 14.04 using canary 53.0.2772.0 & 52.0.2743.46 and below are the observation.
1) Installed permissions_1.crx -- Extension is Enabled
2) Installed Update to permissions_2.crx and clicked Cancel -- Extension is Disabled 
3) Installed Update to permissions_3.crx and clicked Cancel -- No dialog/pop-up for permission was seen.

Note1 : Extension is still in Disabled mode on Mac and Win, Enabled on Ubuntu 14.04 using 53.0.2772.0.

Note2 : Extension is still in Disabled mode on Mac and Win, Dialog for permission is seen again on 14.04 using 52.0.2743.46.

terib@ : Could you please review the attached screen cast(Win) and let us know if any.

Deleted: 616474_June_20.mp4 2.3 MB

	616474_June_20.mp4 2.3 MB View Download

Indeed, I had tested this only on Linux. I've now tried on Windows as well, and as you say, the extension remains disabled. As far as I can tell, the fix works correctly, there is just a problem with the test setup: On Windows and Mac, there are restrictions on sideloaded extensions (i.e. not installed from the Chrome Web Store), and these are what keep the extension disabled.

So, good news: I *think* everything works as intended. Bad news: I don't know how to properly test it.

Devlin: Can you confirm that my theory makes sense? Any ideas?

Turns out that merging this requires merging https://codereview.chromium.org/2025303002 first. Since that CL only adds tests, no changes to prod code at all, I'm just gonna go ahead and merge it too.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/59fde879e0214d423c585ee6bc7d2edf30070bfe

commit 59fde879e0214d423c585ee6bc7d2edf30070bfe
Author: Marc Treib <treib@chromium.org>
Date: Mon Jun 20 13:01:05 2016

Extensions: Add test for granting nuisance and implied permissions on update

This is the test that should have accompanied https://codereview.chromium.org/1995533002

BUG= 603822 , 616474 

Review-Url: https://codereview.chromium.org/2025303002
Cr-Commit-Position: refs/heads/master@{#397344}
(cherry picked from commit fef128daf132927b488fea58466ab834ccf7e1b7)

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

Cr-Commit-Position: refs/branch-heads/2743@{#394}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/59fde879e0214d423c585ee6bc7d2edf30070bfe/chrome/browser/extensions/extension_service_unittest.cc
[add] https://crrev.com/59fde879e0214d423c585ee6bc7d2edf30070bfe/chrome/test/data/extensions/permissions/update.pem
[add] https://crrev.com/59fde879e0214d423c585ee6bc7d2edf30070bfe/chrome/test/data/extensions/permissions/update_1/manifest.json
[add] https://crrev.com/59fde879e0214d423c585ee6bc7d2edf30070bfe/chrome/test/data/extensions/permissions/update_2/manifest.json
[add] https://crrev.com/59fde879e0214d423c585ee6bc7d2edf30070bfe/chrome/test/data/extensions/permissions/update_3/manifest.json
[add] https://crrev.com/59fde879e0214d423c585ee6bc7d2edf30070bfe/chrome/test/data/extensions/permissions/update_4/manifest.json

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a0119b72d113598f972bd5fd9179322a2c9999a4

commit a0119b72d113598f972bd5fd9179322a2c9999a4
Author: Marc Treib <treib@chromium.org>
Date: Mon Jun 20 14:51:28 2016

Re-enable extensions disabled due to permission increase if they have all permissions

BUG= 616474 

Review-Url: https://codereview.chromium.org/2019423007
Cr-Commit-Position: refs/heads/master@{#399876}
(cherry picked from commit 6e51bca1d4a7213bbf926104457eb3e2a22f6999)

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

Cr-Commit-Position: refs/branch-heads/2743@{#396}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/a0119b72d113598f972bd5fd9179322a2c9999a4/chrome/browser/extensions/extension_service.cc
[modify] https://crrev.com/a0119b72d113598f972bd5fd9179322a2c9999a4/chrome/browser/extensions/extension_service.h
[modify] https://crrev.com/a0119b72d113598f972bd5fd9179322a2c9999a4/chrome/browser/extensions/extension_service_unittest.cc
[add] https://crrev.com/a0119b72d113598f972bd5fd9179322a2c9999a4/chrome/test/data/extensions/permissions/update_5/manifest.json
[modify] https://crrev.com/a0119b72d113598f972bd5fd9179322a2c9999a4/tools/metrics/histograms/histograms.xml

@20 yes, you're theory sounds right (we block off-webstore installs for mac/linux).  Testing this in code is fairly doable still (you can disable the webstore lockdown for testing, or can install an extension as if it came from the webstore).  Testing by hand is...tricky.  I think the webstore lockdown is only in Chrome, so installing Chromium might actually be the easiest way.  Otherwise, you probably need a webstore extension to test it.

Thanks for the reply! I did try a custom (i.e. non-Chrome-branded) build on Windows, and things did work as expected. So let's assume it's all good now.

We do have a unit test already, but it'd be nice to actually test the full end-to-end flow. Using an actual webstore extension is possible I guess, but extremely inconvenient and time-consuming (since it needs to be updated twice during each test run).

FYI from related  bug 619759 , if you want a current store extension with search_provider setting override permission that was showing the issue of staying disabled on Windows, you can use extension ID nmgcfemagnogdodbambjhdcmfcpicngl for testing.

Verified the issue as per the steps mentioned in the comment #16 on Windows 7, MAC (10.11.5) & Ubuntu Trusty (14.04) for Google Chrome Beta Version - 52.0.2743.49 and beneath mentioned is the observations.

1) Installed permissions_1.crx -- Extension is Enabled
2) Installed Update to permissions_2.crx and clicked Cancel -- Extension is Disabled 
3) Installed Update to permissions_3.crx -- No pop-up was displayed, but the extension was updated. [Like - Permissions Update Test 3]

Note 1 : Extension is still in Disabled Mode on MAC and Windows, Enabled on Ubuntu Trusty (14.04) using 52.0.2743.49

Note 2 : Extension is still in Disabled Mode on MAC and Windows, Dialog for permission is not seen again on Ubuntu Trusty (14.04) using 52.0.2743.49 but extension is Enabled.

@treib: Could you please have a quick look into the videos attached and let us know if this is the behavior expected. If yes, we shall add the TE-Verified labels.

Thank you.

Deleted: 616474 - MAC.mov 8.9 MB

616474 - MAC.mov 8.9 MB Download

Deleted: 616474 - Windows.mp4 2.0 MB

	616474 - Windows.mp4 2.0 MB View Download

Deleted: 616474 - Linux.ogv 2.7 MB

	616474 - Linux.ogv 2.7 MB View Download

Re #27: Yes, this is the expected behavior, thanks!

Marking it as Verified as per # 28

Thank you treib@ for the confirmation.