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

commit 5d716c4e1fa6bfd6bf2cee13b244ad4b9f645047
Author: Varun Khaneja <vakh@chromium.org>
Date: Tue Oct 10 00:54:11 2017

Send database update notification from PVer4 code

Bug:  771697 

TBR=dglazkov
for trivial changes in
  chrome/browser/chrome_notification_types.h
  chrome/browser/extensions/blacklist.cc

Change-Id: If920aeed593d958a48419b36d676a3be999bace1
Reviewed-on: https://chromium-review.googlesource.com/706204
Commit-Queue: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507556}
[modify] https://crrev.com/5d716c4e1fa6bfd6bf2cee13b244ad4b9f645047/chrome/browser/chrome_notification_types.h
[modify] https://crrev.com/5d716c4e1fa6bfd6bf2cee13b244ad4b9f645047/chrome/browser/extensions/blacklist.cc
[modify] https://crrev.com/5d716c4e1fa6bfd6bf2cee13b244ad4b9f645047/chrome/browser/safe_browsing/local_database_manager.cc
[modify] https://crrev.com/5d716c4e1fa6bfd6bf2cee13b244ad4b9f645047/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc
[add] https://crrev.com/5d716c4e1fa6bfd6bf2cee13b244ad4b9f645047/components/safe_browsing/db/notification_types.h
[modify] https://crrev.com/5d716c4e1fa6bfd6bf2cee13b244ad4b9f645047/components/safe_browsing/db/v4_local_database_manager.cc
[modify] https://crrev.com/5d716c4e1fa6bfd6bf2cee13b244ad4b9f645047/components/safe_browsing/db/v4_local_database_manager.h
[modify] https://crrev.com/5d716c4e1fa6bfd6bf2cee13b244ad4b9f645047/components/safe_browsing/db/v4_local_database_manager_unittest.cc

In light of the recent email about bad Chrome extensions getting installed, increasing its priority and requesting merge to M62.

In the absence of this fix, Chrome won't disable the blacklisted extension until next restart.

nparker@ supports the merge request.

This bug requires manual review: We are only 6 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

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

This seems like a fairly large change. Our last M62 Beta is scheduled for tomorrow and stable is planned for next week. Can you please explain why is this critical enough to be considered for M62?

Why:
---
Recently we found out that some malicious extensions got through our review process and were installed for >37K users (I haven't seen the latest number on this).

Our solution was to blacklist those extensions and say that by blacklisting them, they'll get disabled on users machines. Due to this bug, that's only partly true.

Due to this bug, Chrome does not automatically disable installed extensions after fetching the latest update from Safe Browsing. Instead, it only disables them on startup. This is a regression that I caught last week while doing code review.


Size of change:
--------------
The only change is here: https://chromium-review.googlesource.com/c/chromium/src/+/706204/5/components/safe_browsing/db/v4_local_database_manager.cc (added 4 lines)

The rest of the CL is just scaffolding because the enum moved from chrome:: under chrome/ to safe_browsing:: under components/ because the enum needed to be used in the Safe Browsing component.

Thanks for more details. Have you already tested this well in Canary? Is this a safe merge overall?

> Have you already tested this well in Canary?
It was committed 20 hours ago and hasn't made it into a release yet: https://storage.googleapis.com/chromium-find-releases-static/5d7.html#5d716c4e1fa6bfd6bf2cee13b244ad4b9f645047

I'll wait for 1 day to monitor any crashes on Canary but I do not expect any.

> Is this a safe merge overall?

I am quite confident that it is, but I want to wait for today's Canary before merging.

Thanks - before merging, we need to ensure that it's landed in Canary first. I'm rejecting the merge for now. Once it's well tested, please re-apply merge-request label. 

Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

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

Update: the bug isn't completely fixed yet since the notification is being sent to the incorrect NotificationServiceImpl so it gets ignored.
https://chromium-review.googlesource.com/c/chromium/src/+/711096 fixes that.

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

commit d0a211f1bdb5f0be97fbcf77edf2ac00a9a1ce0a
Author: Varun Khaneja <vakh@chromium.org>
Date: Wed Oct 11 21:26:11 2017

Post the update notification on the UI thread.

The notification needs to be posted on the UI thread because the
extension blacklist checker is observing UI thread's notification
service.

Verified by logging that V4LocalDatabaseManager::CheckExtensionIDs
gets called after posting the notification.

Bug:  771697 
Change-Id: I9af1f52e9d10e4e148ea281fc4474b9378a7fd9e
Reviewed-on: https://chromium-review.googlesource.com/711096
Commit-Queue: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Jialiu Lin <jialiul@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508120}
[modify] https://crrev.com/d0a211f1bdb5f0be97fbcf77edf2ac00a9a1ce0a/components/safe_browsing/db/notification_types.h
[modify] https://crrev.com/d0a211f1bdb5f0be97fbcf77edf2ac00a9a1ce0a/components/safe_browsing/db/v4_local_database_manager.cc
[modify] https://crrev.com/d0a211f1bdb5f0be97fbcf77edf2ac00a9a1ce0a/components/safe_browsing/db/v4_local_database_manager.h

The 2 CLs here are in Canary now.

CL 1: 5d716c4e1fa6bfd6bf2cee13b244ad4b9f645047 in 63.0.3237.0
CL 2: d0a211f1bdb5f0be97fbcf77edf2ac00a9a1ce0a in 63.0.3238.0

There have been no issues reported for them so far and I am confident that there will be no stability regression due to these.

Requesting merge for reasons stated in #c3 and #c8

This bug requires manual review: We are only 4 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

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

Confirmed with vakh@ offline. Here's the summary:

- Currently there are about 37K users that are impacted with bad malware extensions. Since those users will be required to restart the browser to have the blacklist take into effect, for users that don't restart they will still be impacted with these bad extensions.

- The fix for M62 removes the need for browser restart, and allows users to check with Safebrowsing servers for blacklist at some interval.

- However, update to a new version of Chrome requires a restart anyways, so technically this issue will be solved for the blacklisted extensions, since both fixes require a restart.

However, to solve this issue long-term and reduce the need for future in case there are more blacklisted extensions, I am in favor of approving the merge to M62. Branch:3202. Confirmed with vakh@ that it's a simple 3 line fix, and a safe merge overall. 

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

commit f712f44dd0def1e1e6c5bd6bb80def9bf2da88a5
Author: Varun Khaneja <vakh@chromium.org>
Date: Mon Oct 16 20:39:44 2017

[M62] Send DB update notification from PVer4 code

- Cherrypicked from 5d716c4e1fa6bfd6bf2cee13b244ad4b9f645047 (https://crrev.com/c/706204)
- Cherrypicked from d0a211f1bdb5f0be97fbcf77edf2ac00a9a1ce0a (https://crrev.com/c/711096)

Bug:  771697 
Change-Id: Id6875861f11afa6049f63d2ab6945bb0a776d75a
Reviewed-on: https://chromium-review.googlesource.com/721723
Reviewed-by: Nathan Parker <nparker@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#696}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/f712f44dd0def1e1e6c5bd6bb80def9bf2da88a5/chrome/browser/chrome_notification_types.h
[modify] https://crrev.com/f712f44dd0def1e1e6c5bd6bb80def9bf2da88a5/chrome/browser/extensions/blacklist.cc
[modify] https://crrev.com/f712f44dd0def1e1e6c5bd6bb80def9bf2da88a5/chrome/browser/safe_browsing/local_database_manager.cc
[modify] https://crrev.com/f712f44dd0def1e1e6c5bd6bb80def9bf2da88a5/chrome/browser/safe_browsing/safe_browsing_service_browsertest.cc
[add] https://crrev.com/f712f44dd0def1e1e6c5bd6bb80def9bf2da88a5/components/safe_browsing_db/notification_types.h
[modify] https://crrev.com/f712f44dd0def1e1e6c5bd6bb80def9bf2da88a5/components/safe_browsing_db/v4_local_database_manager.cc
[modify] https://crrev.com/f712f44dd0def1e1e6c5bd6bb80def9bf2da88a5/components/safe_browsing_db/v4_local_database_manager.h
[modify] https://crrev.com/f712f44dd0def1e1e6c5bd6bb80def9bf2da88a5/components/safe_browsing_db/v4_local_database_manager_unittest.cc

Adding severity label post fix, so that security bugs have all the required labels.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

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