New issue
Advanced search Search tips

Issue 619759 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Search_provider extension getting auto-disabled in Chromium 51 only if originally installed in Chromium 45 and earlier

Reported by dl-eng-i...@symantec.com, Jun 13 2016

Issue description

UserAgent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.84 Safari/537.36

Steps to reproduce the problem:
(all tested on Windows)
1. Install Chromium 45, new user profile, and extension nmgcfemagnogdodbambjhdcmfcpicngl.  Accept extension permissions.
2. Perform omnibox search in new tab.
3. Install Chromium 51
4. Perform omnibox search in new tab.
--
5. (uninstall chromium)
6. Install Chromium 50, new user profile, and extension nmgcfemagnogdodbambjhdcmfcpicngl.  Accept extension permissions
7. Perform omnibox search in new tab.
8. Install Chromium 51
9. Perform omnibox search in new tab.

What is the expected behavior?
Step 2, 4, 7, 9: search handled by extension, overriding Google search

What went wrong?
Step 4: search is Google search; three-bar menu says extension has new permissions, needs re-enablement

WebStore page: https://chrome.google.com/webstore/detail/norton-safe-search-as-def/nmgcfemagnogdodbambjhdcmfcpicngl/

Did this work before? N/A 

Chrome version: 51.0.2704.84  Channel: stable
OS Version: 6.1 (Windows 7, Windows Server 2008 R2)
Flash Version: Shockwave Flash 21.0 r0
 
Thought this might have been the issue fixed in:

https://bugs.chromium.org/p/chromium/issues/detail?id=603822

But the Chromium 51 I used was build 2704, so I think it had that issue fix already.
Adding more information to the issue:-

1) This issue is seen for customers who had installed Chrome version 45 or lesser and later they update to versions 46 -> 47 -> 48 -> 49 -> 50 -> 51.

Above scenario could be a common use case since most customers will be using Chrome browser for a while and it auto-upgrades


2) This issue was not seen until Chrome 50, only with 51 this issue got introduced



Comment 3 by treib@chromium.org, Jun 14 2016

Cc: treib@chromium.org rdevlin....@chromium.org
Sounds related to  issue 533086  - we had some workaround code for that bug which was removed in M51 (https://codereview.chromium.org/1774103002/).

The problem is expected when upgrading from M45 to M51 directly, but it should not occur if any intermediate version was installed.
Our testing experience described in Comment 2 shows it happens when doing real-world style successive updates if you start from C45 or earlier.  If you start with C46 or later, the issue doesn't happen with successive updates.
Confirmed again doing a progressive test sequence - used a series of Chromium installers below:

1) Installed from clean start Chromium45_Win_338390_mini_installer.exe
Installed extension; gave it permission; confirmed search_provider override works as expected; answered "keep changes" to confirm keeping search_provider override.

2) Installed Chromium46_Win_344814_mini_installer.exe
C&C menu item said extension re-enabling needed due to permission change (a known bug I believe); re-enabled and confirmed search_provider override works as expected.

3) Installed Chromium47_Win_344997_mini_installer.exe
C&C menu item said extension re-enabling needed due to permission change (a known bug I believe); re-enabled and confirmed search_provider override works as expected.

4) Chromium48_Win_359663_mini_installer.exe
Extension permission restored, no C&C menu prompt to re-enable extension; confirmed search_provider override works as expected.

5) Chromium49_Win_369931_mini_installer.exe
Extension permission restored, no C&C menu prompt to re-enable extension; confirmed search_provider override works as expected.

6) Chromium50_Win_378110_mini_installer.exe
Extension permission restored, no C&C menu prompt to re-enable extension; confirmed search_provider override works as expected.

7) Chromium51_Win_386255_mini_installer.exe
C&C menu item said extension re-enabling needed due to permission change ( bug 619759 ); re-enabled and confirmed search_provider override works as expected; unlike the buggy behavior with C46,C47 the re-enablement permission persists (C46,C47 would show the new permission needed each browser launch).
Any inputs on my attempt at a real-world incremental upgrade simulation?  

To me it resulted in an apparent bug in Chromium 51 disabling the extension.

I'll try to test with the fix for  bug 616474 .

Comment 7 by treib@chromium.org, Jun 16 2016

Hey! Sorry, I didn't get around to looking into this yesterday.
Yes, your incremental update scenario looks plausible. I *think* I figured out the problem, and if I'm right, the fix for  bug 616474  will also fix this.

The long story:
Before M46, Chrome only stored that an extension had the "searchProvider" permission, but didn't store the target (as in, "change your default search engine to bing.com" - the "bing.com" part wasn't stored).
Starting in M46, Chrome does store the parameter. To avoid the extension getting disabled for existing users, we added a hack that suppressed the permission warning. That hack was removed in M51 - we believed almost all users should have upgraded to a version >=46 by then.
However, due to  bug 603822 , for those users Chrome never actually stored the new permission with the target.
I tried a quick test, not doing every increment, but hitting the key ones:

1) Installed from clean start Chromium45_Win_338390_mini_installer.exe
Installed extension; gave it permission; confirmed search_provider override works as expected; answered "keep changes" to confirm keeping search_provider override.

2) Chromium49_Win_369931_mini_installer.exe
Extension permission restored, no C&C menu prompt to re-enable extension; confirmed search_provider override works as expected.

3) Chromium51_Win_386255_mini_installer.exe
C&C menu item said extension re-enabling needed due to permission change ( bug 619759 ); left it alone in that state to simulate a user impacted by the bug in C51 and not knowing what to do or ignoring re-enable UI hints.

4) Installed Win_399802_mini_installer.exe, thinking this was just after the latest branch point to latest Chrome canary (399800 branch point given for Chrome canary version 53.0.2768.0 from today);
C&C menu item still said extension re-enabling needed due to permission change

Thinking maybe I didn't pick up correct Canary version with fix for  bug 616474 , went for latest Win Chromium snapshot available right now:

5) Installed Win_400255_mini_installer.exe; 
C&C menu item still said extension re-enabling needed due to permission change

Left wondering if the fix is really included or the  bug 616474  fix did not really fix the underlying issue I see here in  bug 619759 

Comment 9 by treib@chromium.org, Jun 17 2016

Cc: -treib@chromium.org
Labels: -Type-Bug -Pri-2 Pri-1 Type-Bug-Regression
Owner: treib@chromium.org
Status: Assigned (was: Unconfirmed)
re #8: The version in your 4) would not have had the fix yet (that landed as 399876), but the one in 5) does have it.
And you're right, that CL by itself won't fix things for users in the bad state - we'll also have to reinstate the warning suppression hack (essentially, revert https://codereview.chromium.org/1774103002/) and leave it in for a few more versions. I'll handle that.

Thanks for your tireless testing and re-testing here! It's much appreciated!
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 17 2016

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

commit 0412744604981ba5d11ef68dfa356ed3d509fc21
Author: treib <treib@chromium.org>
Date: Fri Jun 17 16:16:17 2016

Reinstate evil hack to suppress permission warnings for searchProvider

(mostly a revert of https://codereview.chromium.org/1774103002/ )

BUG= 619759 

Original issue's description:
> Cleanup: remove evil hack to suppress permission warnings for searchProvider
>
> BUG= 533086 
>
> Committed: https://crrev.com/3e7756c7d39828136744fe7dffe1104eb5433843
> Cr-Commit-Position: refs/heads/master@{#379840}

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

[modify] https://crrev.com/0412744604981ba5d11ef68dfa356ed3d509fc21/chrome/common/extensions/permissions/chrome_permission_message_provider.cc
[modify] https://crrev.com/0412744604981ba5d11ef68dfa356ed3d509fc21/chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
[modify] https://crrev.com/0412744604981ba5d11ef68dfa356ed3d509fc21/extensions/common/permissions/settings_override_permission.cc

Comment 11 by treib@chromium.org, Jun 17 2016

Finally, the above CL, along with the fix for  bug 616474 , should fix this issue. If everything checks out, I'll try to get both merged to M52. (M51 is already in stable, so unfortunately it's too late for that. But affected users should then get fixed once they upgrade to M52.)

Comment 12 by treib@chromium.org, Jun 20 2016

Labels: Merge-Request-52

Comment 13 by tin...@google.com, Jun 20 2016

Labels: -Merge-Request-52 Merge-Approved-52 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M52 (branch: 2743)
Project Member

Comment 14 by bugdroid1@chromium.org, Jun 20 2016

Labels: -merge-approved-52 merge-merged-2743
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/476d033744d348352dbe1209ca1f76f0ee05ff12

commit 476d033744d348352dbe1209ca1f76f0ee05ff12
Author: Marc Treib <treib@chromium.org>
Date: Mon Jun 20 15:54:00 2016

Reinstate evil hack to suppress permission warnings for searchProvider

(mostly a revert of https://codereview.chromium.org/1774103002/ )

BUG= 619759 

Original issue's description:
> Cleanup: remove evil hack to suppress permission warnings for searchProvider
>
> BUG= 533086 
>
> Committed: https://crrev.com/3e7756c7d39828136744fe7dffe1104eb5433843
> Cr-Commit-Position: refs/heads/master@{#379840}

Review-Url: https://codereview.chromium.org/2071293002
Cr-Commit-Position: refs/heads/master@{#400436}
(cherry picked from commit 0412744604981ba5d11ef68dfa356ed3d509fc21)

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

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

[modify] https://crrev.com/476d033744d348352dbe1209ca1f76f0ee05ff12/chrome/common/extensions/permissions/chrome_permission_message_provider.cc
[modify] https://crrev.com/476d033744d348352dbe1209ca1f76f0ee05ff12/chrome/common/extensions/permissions/chrome_permission_message_provider_unittest.cc
[modify] https://crrev.com/476d033744d348352dbe1209ca1f76f0ee05ff12/extensions/common/permissions/settings_override_permission.cc

Comment 15 by treib@chromium.org, Jun 20 2016

Status: Fixed (was: Assigned)
I'm assuming this is fixed. Please re-open if you see it again.
I installed my search_provider overriding extension on Chromium 45, did incremental installs to Chromium 51.

I tested two situations:

1) re-enabling the extension in C51 in response to the "needs re-enablement" UI, then installing snapshot 400770, the extension retains its enabled permission.

2) leaving my extension in the "needs re-enablement" state, which is what C51's undesired behavior did.

Leaving it in this state on C51 and installing snapshot 400770, the extension stays in this state, needing re-enablement.


For me, #1 is fine behavior, glad to see the extension doesn't lose its permissions going from 51->latest.

However, I wrote this up hoping that #2 would not be the behavior - I expected the extension to get automatically re-enabled, with the mistaken permission loss incurred by C51 corrected.

Is that not your expected behavior of the fix?

Comment 17 by treib@chromium.org, Jun 21 2016

Status: Started (was: Fixed)
That's what the fix should have fixed. Apparently there's still something wrong :(

Unfortunately, I won't have time to look at this again this week (traveling).
Devlin, do you have some time to investigate?
Bumping to Devlin or any other thread watcher who can share an update - can some progress be made on this issue today or "soon" in time for M52?
I can't really promise any particular timeline.  We can certainly hope to resolve it soon, but I can't offer much more than that at the moment, sorry.

I don't have a sacrificial windows machine handy to test this - if you still have your setup handy, can you do me a favor and check the user data directory [1] for your Preferences file (it should be at <user_data_dir>/Default/Preferences if you have the default profile)?  There should be an entry in there with the extension id, which should have keys for granted_permissions, active_permissions, disable_reasons, etc.  For debugging this, it would be great if you can post the values for that pref in the different versions here, so that we can see more of what's going on (double-check that the disable reasons are what we expect, and not something funky [maybe from sync?], see what permissions are getting granted, etc).  (If you can, remember to scrub any personal info that might be lingering - there shouldn't be any for the extension entry, and we should only need the single pref rather than the full file, but just a friendly reminder.)

[1] https://www.chromium.org/user-experience/user-data-directory
I think the fix attempt really is working and my comment 16 repro must have had a mistake in the steps I performed.

I tried repro carefully twice while capturing secure_preferences files as you wanted in comment 19.

Each time I got to the last step of installing snapshot 400770 over my C51 where extension was left in "re-enablement needed" state, I observed the good behavior of 400770 restoring the extensions functionality with no permission UI interactions.

Comment 21 by treib@chromium.org, Jun 24 2016

Status: Fixed (was: Started)
Alright, thanks for checking again! In that case, I'll go back to assuming this is fixed :)
Can I tell if this is likely for M52 or likely for M53 and any way I can lobby it for inclusion in M52?

Comment 23 by treib@chromium.org, Jul 17 2016

The fix is already in M52, see comment 14. If you use Chrome Beta, everything should be good again.

If there's still anything wrong in M52, then at this point we most likely won't be able to fix it anymore.

Sign in to add a comment