New issue
Advanced search Search tips

Issue 904958 link

Starred by 1 user
Status: Fixed
Owner:
seblalancette@chromium.org
Closed: Nov 21
Cc:
vakh@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature



Sign in to add a comment

ChromePromptImpl::DisableExtensions should have a kill switch

Project Member Reported by joenotcharles@google.com, Nov 13 
ChromePromptImpl::DisableExtensions is triggered from the Chrome Cleanup Tool to remove extensions that are determined to be UwS. Access to this should be controlled by a kChromeCleanupDisableExtensionsFeature feature flag.

kChromeCleanupQuarantineFeature in srt_field_trial_win.h is a good model of how to implement the feature flag.

The implementation of DisableExtension is at 
https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc?rcl=b903c3070dcb045d64722ef8d738d171e66bf241&l=72.

If the feature is not enabled, the extension_ids parameter of ChromePromptImpl::PromptUser should also be ignored. (Treat it as if it is empty.) This is the list of extensions to delete that will be displayed to the user - if DisableExtension is turned off be the feature flag, we shouldn't display the extensions to the user.

Comment 1 by joenotcharles@google.com, Nov 13 

To test this, you can update https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/chrome_cleaner/srt_delete_extension_win_unittest.cc. Add a base::test::ScopedFeatureList to each existing test, that enables the new feature (see https://cs.chromium.org/chromium/src/base/test/scoped_feature_list.h), and add a new test that disables the feature and makes sure that no extensions are actually deleted.

Comment 2 by vakh@chromium.org, Nov 16

Labels: SafeBrowsing-Triaged
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 21 

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

commit 800d47ea53f943141dc4b9a9139db9d5099e1a23
Author: Sebastien Lalancette <seblalancette@chromium.org>
Date: Wed Nov 21 00:08:20 2018

Added a feature flag for ChromePromptImpl::DisableExtensions

Details:
Flag name: kChromeCleanupExtensionsFeature
When ON, extensions are included when prompting the user for UwS cleanup.
When OFF, extensions are NOT included when prompting the user for UwS cleanup.

Bug:  904958 
Change-Id: Ie483ddb64de11de015429557343f0ee5c4676ff5
Reviewed-on: https://chromium-review.googlesource.com/c/1337790
Commit-Queue: Sebastien Lalancette <seblalancette@chromium.org>
Reviewed-by: Joe Mason <joenotcharles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609859}
[modify] https://crrev.com/800d47ea53f943141dc4b9a9139db9d5099e1a23/chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.cc
[modify] https://crrev.com/800d47ea53f943141dc4b9a9139db9d5099e1a23/chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl.h
[modify] https://crrev.com/800d47ea53f943141dc4b9a9139db9d5099e1a23/chrome/browser/safe_browsing/chrome_cleaner/srt_delete_extension_win_unittest.cc
[modify] https://crrev.com/800d47ea53f943141dc4b9a9139db9d5099e1a23/chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.cc
[modify] https://crrev.com/800d47ea53f943141dc4b9a9139db9d5099e1a23/chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h

Comment 4 by seblalancette@google.com, Nov 21

Status: Fixed (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 12 

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

commit 264fc5fe59542dfabbd56185104ca9d4910311c5
Author: Daniel Bratell <bratell@opera.com>
Date: Wed Dec 12 22:01:16 2018

Don't compile Windows chrome_cleaner prompt code on other OSes

srt_chrome_prompt_impl.cc is code that is only used in Windows
but was compiled for all operating systems. Since it uses constants
that only exist in Windows it's kind of lucky that it doesn't
cause any problems, except in some jumbo build experiments where
there was linker errors.

(TBR for name change in BUILD.gn)
TBR=nparker@chromium.org

Bug:  904958 
Change-Id: If1e3f76c7bc913d4041d0cb38b4891d898d24506
Reviewed-on: https://chromium-review.googlesource.com/c/1350870
Reviewed-by: Daniel Bratell <bratell@opera.com>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Reviewed-by: Joe Mason <joenotcharles@google.com>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#616067}
[modify] https://crrev.com/264fc5fe59542dfabbd56185104ca9d4910311c5/chrome/browser/safe_browsing/BUILD.gn
[modify] https://crrev.com/264fc5fe59542dfabbd56185104ca9d4910311c5/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_impl_win.h
[modify] https://crrev.com/264fc5fe59542dfabbd56185104ca9d4910311c5/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h
[modify] https://crrev.com/264fc5fe59542dfabbd56185104ca9d4910311c5/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h
[modify] https://crrev.com/264fc5fe59542dfabbd56185104ca9d4910311c5/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc
[rename] https://crrev.com/264fc5fe59542dfabbd56185104ca9d4910311c5/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_scanner_results_win.cc
[rename] https://crrev.com/264fc5fe59542dfabbd56185104ca9d4910311c5/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_scanner_results_win.h
[rename] https://crrev.com/264fc5fe59542dfabbd56185104ca9d4910311c5/chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl_win.cc
[rename] https://crrev.com/264fc5fe59542dfabbd56185104ca9d4910311c5/chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl_win.h
[modify] https://crrev.com/264fc5fe59542dfabbd56185104ca9d4910311c5/chrome/browser/safe_browsing/chrome_cleaner/srt_delete_extension_win_unittest.cc
[modify] https://crrev.com/264fc5fe59542dfabbd56185104ca9d4910311c5/chrome/browser/ui/webui/settings/chrome_cleanup_handler.h
►

Sign in to add a comment