Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2555913002:
Implement origin specific Permissions Blacklisting. (Closed)
DescriptionImplement origin specific Permissions Blacklisting.
For Chrome users that have enabled Safe Browsing, when a new permission
request is made, the requesting origin is first checked with Safe
Browsing. If Safe Browsing has blacklisted the origin for the requested
permission, that permission is autoblocked. The origin will only be
autoblocked for blacklisted permissions, any other permissions will
default to the normal prompting for a decision from the user.
Unit tests added to permission_context_base_unittest.cc. Further tests
to follow.
Permissions Blacklisting will also require an update to the UI, which
will also follow.
BUG=561867
Review-Url: https://codereview.chromium.org/2555913002
Cr-Commit-Position: refs/heads/master@{#442501}
Committed: https://chromium.googlesource.com/chromium/src/+/62b8c3d48cfc7b4e090c0d33f409559b299fd9a1
Patch Set 1 #Patch Set 2 : Squashed branches #
Total comments: 29
Patch Set 3 : Address formatting review comments. #Patch Set 4 : Address review comments. #
Total comments: 4
Patch Set 5 : Address reviewer formatting comments. #
Total comments: 14
Patch Set 6 : Add in todos for meredithl at reviewers request. #
Total comments: 24
Patch Set 7 : Add initial implementation of SB timeout. #Patch Set 8 : Fixing indentation. #
Total comments: 4
Patch Set 9 : Nits #
Total comments: 35
Patch Set 10 : Thread safety + nits #Patch Set 11 : Client inherits RefCountedThreadSafe. #
Total comments: 38
Patch Set 12 : Comments, small changes + nits #Patch Set 13 : Fix spelling error. #
Total comments: 20
Patch Set 14 : Remove PostTasking, add quit_closure null checks. #
Total comments: 3
Patch Set 15 : Comments #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 88 (57 generated)
Description was changed from ========== Add whitespace to if condition. Fix formatting and add comments. Override PermissionContextBase::DecidePermission. Implements an override of DecidePermission which calls the base DecidePermission, and then runs the RespondToPermission callback. Implement setter for decide permission callback. Implement run_loop for blacklist tests. PermissionsBlacklisting tests now pass, as do all other tests that do not need to call RespondToPermission. RespondToPermission causes infinite looping as the quit closure is never run. Implement database mock and test method for PBL. Remove logging. Add feature check for PermissionsBlacklist Also null check on both the SafeBrowsingService and the SafeBrowsingDatabaseManager. FREEZE.unindexed BUG= ========== to ========== Implement origin specific PermissionsBlacklisting. When a new permission request is made, the requesting origin is first checked with SafeBrowsing. If SafeBrowsing has blacklisted the origin for the requested permission, it is autoblocked. Unit tests added to permission_context_base_unittest.cc. Further tests to follow. BUG=561867 Add whitespace to if condition. Fix formatting and add comments. Override PermissionContextBase::DecidePermission. Implements an override of DecidePermission which calls the base DecidePermission, and then runs the RespondToPermission callback. Implement setter for decide permission callback. Implement run_loop for blacklist tests. PermissionsBlacklisting tests now pass, as do all other tests that do not need to call RespondToPermission. RespondToPermission causes infinite looping as the quit closure is never run. Implement database mock and test method for PBL. Remove logging. Add feature check for PermissionsBlacklist Also null check on both the SafeBrowsingService and the SafeBrowsingDatabaseManager. FREEZE.unindexed BUG= ==========
Description was changed from ========== Implement origin specific PermissionsBlacklisting. When a new permission request is made, the requesting origin is first checked with SafeBrowsing. If SafeBrowsing has blacklisted the origin for the requested permission, it is autoblocked. Unit tests added to permission_context_base_unittest.cc. Further tests to follow. BUG=561867 Add whitespace to if condition. Fix formatting and add comments. Override PermissionContextBase::DecidePermission. Implements an override of DecidePermission which calls the base DecidePermission, and then runs the RespondToPermission callback. Implement setter for decide permission callback. Implement run_loop for blacklist tests. PermissionsBlacklisting tests now pass, as do all other tests that do not need to call RespondToPermission. RespondToPermission causes infinite looping as the quit closure is never run. Implement database mock and test method for PBL. Remove logging. Add feature check for PermissionsBlacklist Also null check on both the SafeBrowsingService and the SafeBrowsingDatabaseManager. FREEZE.unindexed BUG= ========== to ========== Implement origin specific Permissions Blacklisting. When a new permission request is made, the requesting origin is first checked with Safe Browsing. If Safe Browsing has blacklisted the origin for the requested permission, that permission is autoblocked. Unit tests added to permission_context_base_unittest.cc. Further tests to follow. BUG=561867 ==========
Description was changed from ========== Implement origin specific Permissions Blacklisting. When a new permission request is made, the requesting origin is first checked with Safe Browsing. If Safe Browsing has blacklisted the origin for the requested permission, that permission is autoblocked. Unit tests added to permission_context_base_unittest.cc. Further tests to follow. BUG=561867 ========== to ========== Implement origin specific Permissions Blacklisting. For Chrome users that have enabled Safe Browsing, when a new permission request is made, the requesting origin is first checked with Safe Browsing. If Safe Browsing has blacklisted the origin for the requested permission, that permission is autoblocked. The origin will only be autoblocked for blacklisted permissions, any other permissions will default to the normal prompting for a decision from the user. Unit tests added to permission_context_base_unittest.cc. Further tests to follow. Permissions Blacklisting will also require an update to the UI, which will also follow. BUG=561867 ==========
Description was changed from ========== Implement origin specific Permissions Blacklisting. For Chrome users that have enabled Safe Browsing, when a new permission request is made, the requesting origin is first checked with Safe Browsing. If Safe Browsing has blacklisted the origin for the requested permission, that permission is autoblocked. The origin will only be autoblocked for blacklisted permissions, any other permissions will default to the normal prompting for a decision from the user. Unit tests added to permission_context_base_unittest.cc. Further tests to follow. Permissions Blacklisting will also require an update to the UI, which will also follow. BUG=561867 ========== to ========== Implement origin specific Permissions Blacklisting. For Chrome users that have enabled Safe Browsing, when a new permission request is made, the requesting origin is first checked with Safe Browsing. If Safe Browsing has blacklisted the origin for the requested permission, that permission is autoblocked. The origin will only be autoblocked for blacklisted permissions, any other permissions will default to the normal prompting for a decision from the user. Unit tests added to permission_context_base_unittest.cc. Further tests to follow. Permissions Blacklisting will also require an update to the UI, which will also follow. BUG=561867 ==========
The CQ bit was checked by meredithl@google.com to run a CQ dry run
meredithl@google.com changed reviewers: + dominickn@chromium.org
Hey Dom. PTAL, thanks.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by meredithl@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Great work! https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:53: class PermissionsBlacklistSBClientImpl Add a comment above this class: "The client used when checking whether a permission has been blacklisted by Safe Browsing. The check is asynchronous and no state can be stored in PermissionContextBase while it is in flight (since additional permission requests may be made). Hence, the client is allocated on the heap and is responsible for deleting itself when it is finished. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:81: metadata.api_permissions.find(PermissionUtil::GetPermissionString( We need to verify that PermissionUtil::GetPermissionString has the right string. Can you ping awoz@ again? https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:83: // return to the callback with the result Nit: you can probably remove this comment. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:86: // result has been received and posted, the client can now free itself Nit: "The result has been received, so this object can now delete itself" https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:121: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Nit: put these newlines back in. :) https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:156: // The client will contact safe browsing, and invoke the callback with the Nit: "Safe Browsing" https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:182: // Log to the developer console that this permission is auto blocked. Nit: the comment here is probably unnecessary. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:15: #include "chrome/browser/safe_browsing/safe_browsing_service.h" Nit: I don't think you need this include in this file. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:85: // The requesting origin and permission have been checked by Safe Browsing Nit: update the first sentence of this comment to read: "Called when the requesting origin and permission have been checked by Safe Browsing. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:95: bool result); Nit: "result" is a little vague. Perhaps change it to "blocked_by_sb" or "permission_blocked"? https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:64: if (permissions_blacklist_.find(url) != permissions_blacklist_.end()) { Nit: remove the braces (not needed for a one line body) https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:72: std::set<std::string> permissions) { Nit: const std::set<std::string>& permissions https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:811: // SafeBrowsing for. Comment: "Tests that a blacklisted (URL, permission) pair has its permission request blocked." https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:816: std::set<std::string> blacklisted_permissions; Does this work? std::set<std::string> blacklisted_permissions{ PermissionUtil::GetPermissionString( content::PermissionType::GEOLOCATION) }; https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:825: // Tests a URL that is blacklisted for one permission requesting another of a Comment: "Tests that a URL with a blacklisted permission is permitted to request a non-blacklisted permission."
The CQ bit was checked by meredithl@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:53: class PermissionsBlacklistSBClientImpl On 2016/12/07 04:34:29, dominickn wrote: > Add a comment above this class: > > "The client used when checking whether a permission has been blacklisted by Safe > Browsing. The check is asynchronous and no state can be stored in > PermissionContextBase while it is in flight (since additional permission > requests may be made). Hence, the client is allocated on the heap and is > responsible for deleting itself when it is finished. Done. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:83: // return to the callback with the result On 2016/12/07 04:34:29, dominickn wrote: > Nit: you can probably remove this comment. Done. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:86: // result has been received and posted, the client can now free itself On 2016/12/07 04:34:29, dominickn wrote: > Nit: "The result has been received, so this object can now delete itself" Done. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:156: // The client will contact safe browsing, and invoke the callback with the On 2016/12/07 04:34:29, dominickn wrote: > Nit: "Safe Browsing" Done. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:182: // Log to the developer console that this permission is auto blocked. On 2016/12/07 04:34:28, dominickn wrote: > Nit: the comment here is probably unnecessary. Done. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:15: #include "chrome/browser/safe_browsing/safe_browsing_service.h" On 2016/12/07 04:34:29, dominickn wrote: > Nit: I don't think you need this include in this file. Done. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:85: // The requesting origin and permission have been checked by Safe Browsing On 2016/12/07 04:34:29, dominickn wrote: > Nit: update the first sentence of this comment to read: > > "Called when the requesting origin and permission have been checked by Safe > Browsing. Done. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:95: bool result); On 2016/12/07 04:34:29, dominickn wrote: > Nit: "result" is a little vague. Perhaps change it to "blocked_by_sb" or > "permission_blocked"? Done. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:64: if (permissions_blacklist_.find(url) != permissions_blacklist_.end()) { On 2016/12/07 04:34:29, dominickn wrote: > Nit: remove the braces (not needed for a one line body) Done. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:72: std::set<std::string> permissions) { On 2016/12/07 04:34:29, dominickn wrote: > Nit: const std::set<std::string>& permissions Done. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:811: // SafeBrowsing for. On 2016/12/07 04:34:29, dominickn wrote: > Comment: "Tests that a blacklisted (URL, permission) pair has its permission > request blocked." Done. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:816: std::set<std::string> blacklisted_permissions; On 2016/12/07 04:34:29, dominickn wrote: > Does this work? > > std::set<std::string> blacklisted_permissions{ > PermissionUtil::GetPermissionString( > content::PermissionType::GEOLOCATION) }; Done. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:816: std::set<std::string> blacklisted_permissions; On 2016/12/07 04:34:29, dominickn wrote: > Does this work? > > std::set<std::string> blacklisted_permissions{ > PermissionUtil::GetPermissionString( > content::PermissionType::GEOLOCATION) }; Yes it does! I have also fixed it in the TestPermissionBlacklistingAllowed method. https://codereview.chromium.org/2555913002/diff/20001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base_unittest.cc:825: // Tests a URL that is blacklisted for one permission requesting another of a On 2016/12/07 04:34:29, dominickn wrote: > Comment: "Tests that a URL with a blacklisted permission is permitted to request > a non-blacklisted permission." Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dominickn@chromium.org changed reviewers: + kcarattini@chromium.org
lgtm % nits. +kcarattini - PTAL thanks! https://codereview.chromium.org/2555913002/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:90: // result has been received, so the object can now delete itself Nit: "The result...", then full stop. https://codereview.chromium.org/2555913002/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2555913002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:85: // Browsing The result determines whether to auto-block the permission request Nit: full stop.
https://codereview.chromium.org/2555913002/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:90: // result has been received, so the object can now delete itself On 2016/12/08 00:04:03, dominickn wrote: > Nit: "The result...", then full stop. Done. https://codereview.chromium.org/2555913002/diff/60001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2555913002/diff/60001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.h:85: // Browsing The result determines whether to auto-block the permission request On 2016/12/08 00:04:03, dominickn wrote: > Nit: full stop. Done.
kcarattini@chromium.org changed reviewers: + awoz@google.com
Thanks, Meredith. This is looking great! awoz@ Can you please have a look at the metadata comment? Kendra https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:86: metadata.api_permissions.find(PermissionUtil::GetPermissionString( awoz@ Can you confirm that the string for a permissions in the SB metadata response matches that returned by PermissionUtil::GetPermissionString? https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:172: CheckPermissionsBlacklistResult(web_contents, id, requesting_origin, Can you add a TODO for some UMA metrics here? https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:187: if (permission_blocked) { Also add a TODO for UMA in this method. https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:194: callback.Run(CONTENT_SETTING_BLOCK); Please add a TODO here to consider setting the content setting and persist the decision.
https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:91: delete this; How will you handle cleanup on a timeout or no response from SB?
https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:91: delete this; On 2016/12/11 23:38:41, kcarattini wrote: > How will you handle cleanup on a timeout or no response from SB? Using weak_ptr for the Safe Browsing client implementation that will invalidate once OnCheckApiBlacklistUrlResult has been called on the client. https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:172: CheckPermissionsBlacklistResult(web_contents, id, requesting_origin, On 2016/12/11 23:22:44, kcarattini wrote: > Can you add a TODO for some UMA metrics here? Done. https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:187: if (permission_blocked) { On 2016/12/11 23:22:44, kcarattini wrote: > Also add a TODO for UMA in this method. Done. https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:194: callback.Run(CONTENT_SETTING_BLOCK); On 2016/12/11 23:22:44, kcarattini wrote: > Please add a TODO here to consider setting the content setting and persist the > decision. Done.
nparker@chromium.org changed reviewers: + nparker@chromium.org
https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:74: void StartCheck( Do this code properly handle the case where the webcontents/etc is deleted before the call back is done? https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:91: delete this; If you're going to have this obj own itself, you should more clearly define the ownership. One idea: How about making the constructor private, and making a static method to kick off one of these requests? That way you avoid the possibility that someone will create one of these on the stack and then have it call delete. https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:167: new PermissionsBlacklistSBClientImpl( A new'd bare pointer is kind of a red flag to me here. https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:18: #include "components/safe_browsing_db/database_manager.h" Rather than including the .h, you can should be able to do a forward declaration since you only need a pointer to the object defined here.
https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:227: You'll need to add blacklisting logic here as well. Please add a TODO. https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:167: new PermissionsBlacklistSBClientImpl( On 2016/12/12 19:02:25, Nathan Parker wrote: > A new'd bare pointer is kind of a red flag to me here. I don't disagree with you. Could PermissionContextBase just keep track of its in-progress SB checks like SB database manager does?
https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:227: On 2016/12/13 01:49:51, kcarattini wrote: > You'll need to add blacklisting logic here as well. Please add a TODO. As we discussed, no need to add the TODO but please mention in the Design Doc.
https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:86: metadata.api_permissions.find(PermissionUtil::GetPermissionString( On 2016/12/11 23:22:44, kcarattini wrote: > awoz@ Can you confirm that the string for a permissions in the SB metadata > response matches that returned by PermissionUtil::GetPermissionString? We are pretty sure that they aren't at this point, still waiting for confirmation. I can add a todo for a conversion method between what safe browsing returns and PermissionUtil::GetPermissionString. https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:227: On 2016/12/13 03:52:02, kcarattini wrote: > On 2016/12/13 01:49:51, kcarattini wrote: > > You'll need to add blacklisting logic here as well. Please add a TODO. > > As we discussed, no need to add the TODO but please mention in the Design Doc. Done.
https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/80001/chrome/browser/permissi... chrome/browser/permissions/permission_context_base.cc:227: On 2016/12/13 03:52:02, kcarattini wrote: > On 2016/12/13 01:49:51, kcarattini wrote: > > You'll need to add blacklisting logic here as well. Please add a TODO. > > As we discussed, no need to add the TODO but please mention in the Design Doc. I've also added a comment in the header file to make anyone reading aware of the potential inconsistency. https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:74: void StartCheck( On 2016/12/12 19:02:25, Nathan Parker wrote: > Do this code properly handle the case where the webcontents/etc is deleted > before the call back is done? Done. https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:74: void StartCheck( On 2016/12/12 19:02:25, Nathan Parker wrote: > Do this code properly handle the case where the webcontents/etc is deleted > before the call back is done? Updated the client to inherit from WebContentsObserver. https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:91: delete this; On 2016/12/12 19:02:25, Nathan Parker wrote: > If you're going to have this obj own itself, you should more clearly define the > ownership. > > One idea: How about making the constructor private, and making a static method > to kick off one of these requests? That way you avoid the possibility that > someone will create one of these on the stack and then have it call delete. Done. https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:167: new PermissionsBlacklistSBClientImpl( On 2016/12/12 19:02:25, Nathan Parker wrote: > A new'd bare pointer is kind of a red flag to me here. We went with the static request to instantiate a new client for the request through a private constructor, so this has been removed. https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:167: new PermissionsBlacklistSBClientImpl( On 2016/12/13 01:49:51, kcarattini wrote: > On 2016/12/12 19:02:25, Nathan Parker wrote: > > A new'd bare pointer is kind of a red flag to me here. > > I don't disagree with you. Could PermissionContextBase just keep track of its > in-progress SB checks like SB database manager does? Dom and I decided it was better to encapsulate as much as possible inside the client, rather than having to store anything in the PermissionContextBase. https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:18: #include "components/safe_browsing_db/database_manager.h" On 2016/12/12 19:02:25, Nathan Parker wrote: > Rather than including the .h, you can should be able to do a forward declaration > since you only need a pointer to the object defined here. Done.
The CQ bit was checked by meredithl@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by meredithl@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by meredithl@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still lgtm % nits https://codereview.chromium.org/2555913002/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:141: base::WeakPtrFactory<PermissionsBlacklistSBClientImpl> weak_ptr_factory_; Nit: DISALLOW_COPY_AND_ASSIGN here. https://codereview.chromium.org/2555913002/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:326: // is Nit: fix formatting here.
https://codereview.chromium.org/2555913002/diff/140001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:141: base::WeakPtrFactory<PermissionsBlacklistSBClientImpl> weak_ptr_factory_; On 2016/12/15 03:52:45, dominickn wrote: > Nit: DISALLOW_COPY_AND_ASSIGN here. Done. https://codereview.chromium.org/2555913002/diff/140001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:326: // is On 2016/12/15 03:52:45, dominickn wrote: > Nit: fix formatting here. Done.
lgtm https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:58: const int kCheckUrlTimeoutMs = 1000; This might be a bit short. Make sure there's (eventually) an UMA metric to show often this is hit.
The CQ bit was checked by meredithl@google.com to run a CQ dry run
https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:58: const int kCheckUrlTimeoutMs = 1000; On 2016/12/15 18:15:12, Nathan Parker wrote: > This might be a bit short. Make sure there's (eventually) an UMA metric to show > often this is hit. Acknowledged.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
meredithl@google.com changed reviewers: + raymes@chromium.org
Hi Raymes. PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:66: metadata.api_permissions = permissions_blacklist_.find(url)->second; nit: can we just store the result of permissions_blacklist_.find() above and reuse it here? https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:111: quit_closure_); Do we need this line? Could we instead just call quit_closure_.Run() after respond_permission_.Run() below? I might be missing something though, I know this part was tricky :) https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:145: quit_closure_.Run(); It's probably good to clear the quit_closure_ after it's been run to ensure that it's not run twice by mistake. https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:200: class TestPermissionsBlacklistingContext : public TestPermissionContext { nit: Rather than adding a new class here, I don't think it would be problematic to put this in TestPermissionContext with a function which enables the blacklisting for that test https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:16: #include "base/memory/weak_ptr.h" nit: this one is included in the header so not required here https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:125: base::Bind(callback_, permission_blocked)); I think there might be a few threading issues here that we need to take care of: -It recently came up that we need to be careful of how weak ptrs are used across threads. I think the current behavior may be racey because deletion of this class could happen on a different thread to the one that weak ptrs are dereferenced on. -We're calling delete from 2 different threads, which could be racey (e.g. delete could get called twice) -Similarly, the callback could get posted to the UI thread after the WebContents has been destroyed, in which case it would get run but with a deleted WebContents I think we should probably come back to the UI thread inside the class and check whether the WebContents is destroyed prior to running |callback_| and deleting this class. That will also help ensure this class is always created/deleted on the same thread. I don't think that would solve the WeakPtr issue though. I see 2 ways forward with that: 1) Use RefCountedThreadsafe for this class, which would ensure it lives until its outstanding callbacks are finished. This might be slightly simpler. 2) We don't really need to pass weak ptrs to the timer or the sb client, because they both allow cancelling outstanding requests. So we can ensure that they're not holding references to this class when it gets deleted. In both cases, if WebContentsDestroyed is called, I think we may want to go back to the IO thread and cancel the outstanding requests so they're not left hanging. https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:132: // is still freed properly. Hmm what does it mean that the WebContents should outlive the permission request? We might want to clarify a bit. My understanding here is that if the WebContents is destroyed, we cancel the permission requests and don't run |callback_|. The request is no longer valid at that point. Is that your understanding also? https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:138: base::OneShotTimer timer_; It can be helpful to document which threads different members are used on in multithreaded classes to help defend against racey access. https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:210: // results or timed out. We're not allocating the object here anymore so we probably don't need that part of the comment here. But it may be worth noting why it's safe to pass base::Unretained(this) in here. My understanding is that the callback passed in will not be run if the WebContents is destroyed while it's outstanding. PermissionContext is tied to the Profile which should always outlive the WebContents. Do you agree? https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:217: // TODO(meredithl) : add UMA metrics here. nit: It might be better to remove this TODO (and the one below about metrics) as I'm not sure if they give any useful information to someone reading the code. We could track the reminder for ourselves in a bug instead. https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:220: false); nit: We tend to document bool arguments so that it's clear to readers what they're for, e.g. false /* permission_blocked */); https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:224: void PermissionContextBase::CheckPermissionsBlacklistResult( very nitty: this does more than just check the permission blacklist result, so it might be worth naming it something more general, like ContinueRequestPermission (other ideas welcome too :). The bool parameter can be called permission_blacklisted or something to make it clear that part is coming from the blacklist. https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:243: callback.Run(CONTENT_SETTING_BLOCK); I think we might want to call NotifyPermissionSet here so that we update the tab context correctly. https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:90: void CheckPermissionsBlacklistResult( nit: should this be private? https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:101: // synchronously. nit: I think the fact that this doesn't handle the blacklist check isn't the intended behavior so we should mark it as such, e.g. having this as a TODO instead TODO(...): Ensure that this result accurately reflects whether the origin is blacklisted for this permission https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:178: GetSafeBrowsingDatabaseManager(); nit: virtuals have several downsides, e.g. the temptation for them to be overridden by subclasses. I think the SetXForTest pattern tends to be a better approach and it also makes it really clear that the function is only used in testing. e.g. in this case we would have the SafeBrowsingDatbaseManager as a member here which gets lazily initialized when it's needed and we could have SetSafeBrowsingDatabaseManagerForTest as a private member function which sets up the variable in tests.
https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:58: const int kCheckUrlTimeoutMs = 1000; On 2016/12/16 00:10:39, meredithl wrote: > On 2016/12/15 18:15:12, Nathan Parker wrote: > > This might be a bit short. Make sure there's (eventually) an UMA metric to > show > > often this is hit. > > Acknowledged. Just wondering how you arrived at 1000ms? What about a bit longer, say 3s or 5s (I think this was the standard v3 timeout)? Please also add this case to the bug you create for UMA metrics. https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:243: callback.Run(CONTENT_SETTING_BLOCK); On 2016/12/19 00:07:41, raymes wrote: > I think we might want to call NotifyPermissionSet here so that we update the tab > context correctly. Won't that result in a page action icon being shown? That's why it's not called for the kill switch. Either way the two should be consistent.
The CQ bit was checked by meredithl@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by meredithl@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:16: #include "base/memory/weak_ptr.h" On 2016/12/19 00:07:41, raymes wrote: > nit: this one is included in the header so not required here Done. https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:58: const int kCheckUrlTimeoutMs = 1000; On 2016/12/19 06:37:45, kcarattini wrote: > On 2016/12/16 00:10:39, meredithl wrote: > > On 2016/12/15 18:15:12, Nathan Parker wrote: > > > This might be a bit short. Make sure there's (eventually) an UMA metric to > > show > > > often this is hit. > > > > Acknowledged. > > Just wondering how you arrived at 1000ms? What about a bit longer, say 3s or 5s > (I think this was the standard v3 timeout)? > > Please also add this case to the bug you create for UMA metrics. This was just a value that Dominick decided on, the idea being that having to wait 1sec or more to see a permission prompt if the requesting origin isn't blacklisted could be confusing. https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:125: base::Bind(callback_, permission_blocked)); On 2016/12/19 00:07:41, raymes wrote: > I think there might be a few threading issues here that we need to take care of: > -It recently came up that we need to be careful of how weak ptrs are used across > threads. I think the current behavior may be racey because deletion of this > class could happen on a different thread to the one that weak ptrs are > dereferenced on. > -We're calling delete from 2 different threads, which could be racey (e.g. > delete could get called twice) > -Similarly, the callback could get posted to the UI thread after the WebContents > has been destroyed, in which case it would get run but with a deleted > WebContents > > I think we should probably come back to the UI thread inside the class and check > whether the WebContents is destroyed prior to running |callback_| and deleting > this class. That will also help ensure this class is always created/deleted on > the same thread. > > I don't think that would solve the WeakPtr issue though. I see 2 ways forward > with that: > 1) Use RefCountedThreadsafe for this class, which would ensure it lives until > its outstanding callbacks are finished. This might be slightly simpler. > 2) We don't really need to pass weak ptrs to the timer or the sb client, because > they both allow cancelling outstanding requests. So we can ensure that they're > not holding references to this class when it gets deleted. > > In both cases, if WebContentsDestroyed is called, I think we may want to go back > to the IO thread and cancel the outstanding requests so they're not left > hanging. Done. Removed weak pointers and instead using RefCountedThreadSafe. The client now returns to the UI thread to check if the web contents is still active, and if so it will run the callback. I'm not too sure of the part about returning to the IO thread to cancel outstanding requests though? Can you elaborate? https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:132: // is still freed properly. On 2016/12/19 00:07:41, raymes wrote: > Hmm what does it mean that the WebContents should outlive the permission > request? We might want to clarify a bit. My understanding here is that if the > WebContents is destroyed, we cancel the permission requests and don't run > |callback_|. The request is no longer valid at that point. Is that your > understanding also? Just a bad comment I think! Fixed. https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:138: base::OneShotTimer timer_; On 2016/12/19 00:07:41, raymes wrote: > It can be helpful to document which threads different members are used on in > multithreaded classes to help defend against racey access. Done. https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:210: // results or timed out. On 2016/12/19 00:07:41, raymes wrote: > We're not allocating the object here anymore so we probably don't need that part > of the comment here. But it may be worth noting why it's safe to pass > base::Unretained(this) in here. My understanding is that the callback passed in > will not be run if the WebContents is destroyed while it's outstanding. > PermissionContext is tied to the Profile which should always outlive the > WebContents. Do you agree? Yep. I've updated the comment. https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:217: // TODO(meredithl) : add UMA metrics here. On 2016/12/19 00:07:41, raymes wrote: > nit: It might be better to remove this TODO (and the one below about metrics) as > I'm not sure if they give any useful information to someone reading the code. We > could track the reminder for ourselves in a bug instead. Okay. Should I leave them in for now until a bug gets filed (if one hasn't already)? https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:220: false); On 2016/12/19 00:07:41, raymes wrote: > nit: We tend to document bool arguments so that it's clear to readers what > they're for, e.g. > false /* permission_blocked */); Done. https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:224: void PermissionContextBase::CheckPermissionsBlacklistResult( On 2016/12/19 00:07:41, raymes wrote: > very nitty: this does more than just check the permission blacklist result, so > it might be worth naming it something more general, like > ContinueRequestPermission (other ideas welcome too :). The bool parameter can be > called permission_blacklisted or something to make it clear that part is coming > from the blacklist. Done. https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:243: callback.Run(CONTENT_SETTING_BLOCK); On 2016/12/19 06:37:45, kcarattini wrote: > On 2016/12/19 00:07:41, raymes wrote: > > I think we might want to call NotifyPermissionSet here so that we update the > tab > > context correctly. > > Won't that result in a page action icon being shown? That's why it's not called > for the kill switch. Either way the two should be consistent. For now I'll leave it out for consistency. Is there any downside to it not being run? https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:90: void CheckPermissionsBlacklistResult( On 2016/12/19 00:07:41, raymes wrote: > nit: should this be private? Done. https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:101: // synchronously. On 2016/12/19 00:07:41, raymes wrote: > nit: I think the fact that this doesn't handle the blacklist check isn't the > intended behavior so we should mark it as such, e.g. having this as a TODO > instead TODO(...): Ensure that this result accurately reflects whether the > origin is blacklisted for this permission Done. https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:178: GetSafeBrowsingDatabaseManager(); On 2016/12/19 00:07:41, raymes wrote: > nit: virtuals have several downsides, e.g. the temptation for them to be > overridden by subclasses. I think the SetXForTest pattern tends to be a better > approach and it also makes it really clear that the function is only used in > testing. > e.g. in this case we would have the SafeBrowsingDatbaseManager as a member here > which gets lazily initialized when it's needed and we could have > SetSafeBrowsingDatabaseManagerForTest as a private member function which sets up > the variable in tests. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
My last comments; handing off to Raymes now. Thanks, Kendra https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:57: const int kCheckUrlTimeoutMs = 1000; I'm concerned that this is too short. How about we make it longer for now and measure the request time with an UMA metric? You can add a TODO to revisit this once we have some data about request time. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:243: } Do you need these 8 lines here or can you move them inside the if clause below (and change the "if" to just check whether the feature is enabled)? You aren't using the db_manager when the feature is off so there's no need for the extra call to SB Service, and I'd prefer to move it closer to where it's being used.
Thanks! https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:66: metadata.api_permissions = permissions_blacklist_.find(url)->second; On 2016/12/19 00:07:41, raymes wrote: > nit: can we just store the result of permissions_blacklist_.find() above and > reuse it here? I think you might have missed this comments from a previous patchset https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:111: quit_closure_); On 2016/12/19 00:07:41, raymes wrote: > Do we need this line? Could we instead just call quit_closure_.Run() after > respond_permission_.Run() below? I might be missing something though, I know > this part was tricky :) ditto https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:145: quit_closure_.Run(); On 2016/12/19 00:07:41, raymes wrote: > It's probably good to clear the quit_closure_ after it's been run to ensure that > it's not run twice by mistake. ditto https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:125: base::Bind(callback_, permission_blocked)); On 2016/12/20 07:38:27, meredithl wrote: > On 2016/12/19 00:07:41, raymes wrote: > > I think there might be a few threading issues here that we need to take care > of: > > -It recently came up that we need to be careful of how weak ptrs are used > across > > threads. I think the current behavior may be racey because deletion of this > > class could happen on a different thread to the one that weak ptrs are > > dereferenced on. > > -We're calling delete from 2 different threads, which could be racey (e.g. > > delete could get called twice) > > -Similarly, the callback could get posted to the UI thread after the > WebContents > > has been destroyed, in which case it would get run but with a deleted > > WebContents > > > > I think we should probably come back to the UI thread inside the class and > check > > whether the WebContents is destroyed prior to running |callback_| and deleting > > this class. That will also help ensure this class is always created/deleted on > > the same thread. > > > > I don't think that would solve the WeakPtr issue though. I see 2 ways forward > > with that: > > 1) Use RefCountedThreadsafe for this class, which would ensure it lives until > > its outstanding callbacks are finished. This might be slightly simpler. > > 2) We don't really need to pass weak ptrs to the timer or the sb client, > because > > they both allow cancelling outstanding requests. So we can ensure that they're > > not holding references to this class when it gets deleted. > > > > In both cases, if WebContentsDestroyed is called, I think we may want to go > back > > to the IO thread and cancel the outstanding requests so they're not left > > hanging. > > Done. > Removed weak pointers and instead using RefCountedThreadSafe. > The client now returns to the UI thread to check if the web contents is still > active, and if so it will run the callback. > I'm not too sure of the part about returning to the IO thread to cancel > outstanding requests though? Can you elaborate? I don't think the last bit is too important. It would just be an optimization to cancel the existing requests if the WebContents gets destroyed. But it's probably fine to rely on the timer finishing. https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:243: callback.Run(CONTENT_SETTING_BLOCK); On 2016/12/20 07:38:27, meredithl wrote: > On 2016/12/19 06:37:45, kcarattini wrote: > > On 2016/12/19 00:07:41, raymes wrote: > > > I think we might want to call NotifyPermissionSet here so that we update the > > tab > > > context correctly. > > > > Won't that result in a page action icon being shown? That's why it's not > called > > for the kill switch. Either way the two should be consistent. > > For now I'll leave it out for consistency. Is there any downside to it not being > run? I think this case is different to the kill switch because we want to allow the user to override the setting. Do we actually not want to show the page action in that case? https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:101: // synchronously. I don't see this updated :( https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:63: class PermissionsBlacklistingClient This is a fairly substantial class now. I think it could be good to consider moving it into a separate file in a followup CL. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:73: int timeout, Could we just make this a member variable of the class rather than passing it around? https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:84: ~PermissionsBlacklistingClient() override {} nit: constructor before destructor usually https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:120: // TODO(meredithl) : fix this for post task I'm not sure what this comment means? https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:132: // TODO(meredithl) : Implement correct permission string check. Hmm I'm not sure what this one means either? https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:147: if (is_active_) It doesn't look like is_active_ is initialized anywhere. That would result in undefined behavior. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:157: Observe(nullptr); Why is this needed? Is it just so we don't accidentally touch web_contents()? https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:170: // True if |callback_| should be invoked. For instance, if web_contents() is nit: Since the only reason why this would be false is if the WebContents is destroyed, I think you can just remove the "For instance" here. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:242: db_manager = sb_service->database_manager(); Hmm, we never set the instance variable here. I think we want to have db_manager_ = ... That should avoid needing the local variable as well, I think we could just use db_manager_ https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:243: } On 2016/12/20 19:57:10, kcarattini wrote: > Do you need these 8 lines here or can you move them inside the if clause below > (and change the "if" to just check whether the feature is enabled)? You aren't > using the db_manager when the feature is off so there's no need for the extra > call to SB Service, and I'd prefer to move it closer to where it's being used. Good thought. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:256: // TODO(meredithl) : add UMA metrics here. nit: here and elsewhere, no space before : in TODOs and capital letter at the start of sentences, e.g. TODO(raymes): The dog went to sleep. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:184: void SetSafeBrowsingDatabaseManagerForTests( nit: ForTest (without this s) is a bit more common https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:187: void SetTimeoutForSafeBrowsingClient(int timeout); nit: it should be made clear that this is just for tests too: SetSafeBrowingTimeoutForTest You could even consider just having the timeout as an extra param on the above function too. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:862: url, 0 /* timeout in ms */, CONTENT_SETTING_ASK); Is the timer guaranteed to fire before the test client returns?
The CQ bit was checked by meredithl@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:66: metadata.api_permissions = permissions_blacklist_.find(url)->second; On 2016/12/20 23:58:56, raymes (OOO until Jan 9) wrote: > On 2016/12/19 00:07:41, raymes wrote: > > nit: can we just store the result of permissions_blacklist_.find() above and > > reuse it here? > > I think you might have missed this comments from a previous patchset Done. https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:111: quit_closure_); On 2016/12/20 23:58:56, raymes (OOO until Jan 9) wrote: > On 2016/12/19 00:07:41, raymes wrote: > > Do we need this line? Could we instead just call quit_closure_.Run() after > > respond_permission_.Run() below? I might be missing something though, I know > > this part was tricky :) > > ditto Sorry Raymes, I did miss these. DecidePermission isn't called for every permission request, such as in the case of TestPermissionBlacklistingBlocked and TestRequestPermissionInvalidUrl. These automatically block the request, and run the callback, which for tests is TrackPermissionDecided. This meant that the run loop was never exited and tests would hang, so a simple solution was to post the quit_closure in TrackPermissionDecided to make sure the run loop exits for all tests. Is there a more straightforward way of doing this do you think? https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:145: quit_closure_.Run(); On 2016/12/20 23:58:56, raymes (OOO until Jan 9) wrote: > On 2016/12/19 00:07:41, raymes wrote: > > It's probably good to clear the quit_closure_ after it's been run to ensure > that > > it's not run twice by mistake. > > ditto By "clear" do you mean set to null? https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:200: class TestPermissionsBlacklistingContext : public TestPermissionContext { On 2016/12/19 00:07:41, raymes (OOO until Jan 9) wrote: > nit: Rather than adding a new class here, I don't think it would be problematic > to put this in TestPermissionContext with a function which enables the > blacklisting for that test You're right, but since we decided to go for the SetXForTests over virtual functions, that actually rendered this class entirely obsolete so I've removed it :) https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:125: base::Bind(callback_, permission_blocked)); On 2016/12/20 23:58:56, raymes (OOO until Jan 9) wrote: > On 2016/12/20 07:38:27, meredithl wrote: > > On 2016/12/19 00:07:41, raymes wrote: > > > I think there might be a few threading issues here that we need to take care > > of: > > > -It recently came up that we need to be careful of how weak ptrs are used > > across > > > threads. I think the current behavior may be racey because deletion of this > > > class could happen on a different thread to the one that weak ptrs are > > > dereferenced on. > > > -We're calling delete from 2 different threads, which could be racey (e.g. > > > delete could get called twice) > > > -Similarly, the callback could get posted to the UI thread after the > > WebContents > > > has been destroyed, in which case it would get run but with a deleted > > > WebContents > > > > > > I think we should probably come back to the UI thread inside the class and > > check > > > whether the WebContents is destroyed prior to running |callback_| and > deleting > > > this class. That will also help ensure this class is always created/deleted > on > > > the same thread. > > > > > > I don't think that would solve the WeakPtr issue though. I see 2 ways > forward > > > with that: > > > 1) Use RefCountedThreadsafe for this class, which would ensure it lives > until > > > its outstanding callbacks are finished. This might be slightly simpler. > > > 2) We don't really need to pass weak ptrs to the timer or the sb client, > > because > > > they both allow cancelling outstanding requests. So we can ensure that > they're > > > not holding references to this class when it gets deleted. > > > > > > In both cases, if WebContentsDestroyed is called, I think we may want to go > > back > > > to the IO thread and cancel the outstanding requests so they're not left > > > hanging. > > > > Done. > > Removed weak pointers and instead using RefCountedThreadSafe. > > The client now returns to the UI thread to check if the web contents is still > > active, and if so it will run the callback. > > I'm not too sure of the part about returning to the IO thread to cancel > > outstanding requests though? Can you elaborate? > > I don't think the last bit is too important. It would just be an optimization to > cancel the existing requests if the WebContents gets destroyed. But it's > probably fine to rely on the timer finishing. Ohh I see. So if WebContents goes away, call the CancelApiCheck function for this client. In that case a reference to the db manager would need to be stored in the client class, at the moment I just pass it in when the check is actually started. Is that okay? https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:243: callback.Run(CONTENT_SETTING_BLOCK); On 2016/12/20 23:58:56, raymes (OOO until Jan 9) wrote: > On 2016/12/20 07:38:27, meredithl wrote: > > On 2016/12/19 06:37:45, kcarattini wrote: > > > On 2016/12/19 00:07:41, raymes wrote: > > > > I think we might want to call NotifyPermissionSet here so that we update > the > > > tab > > > > context correctly. > > > > > > Won't that result in a page action icon being shown? That's why it's not > > called > > > for the kill switch. Either way the two should be consistent. > > > > For now I'll leave it out for consistency. Is there any downside to it not > being > > run? > > I think this case is different to the kill switch because we want to allow the > user to override the setting. Do we actually not want to show the page action in > that case? I think that makes sense. The kill switch is like the ban hammer, blacklisting is more of a suggested setting which the user can change. Is more UI bad in this case? https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2555913002/diff/160001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:101: // synchronously. On 2016/12/20 23:58:56, raymes (OOO until Jan 9) wrote: > I don't see this updated :( Haha, git add -p blues. Fixed! https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:57: const int kCheckUrlTimeoutMs = 1000; On 2016/12/20 19:57:10, kcarattini wrote: > I'm concerned that this is too short. How about we make it longer for now and > measure the request time with an UMA metric? You can add a TODO to revisit this > once we have some data about request time. Done. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:63: class PermissionsBlacklistingClient On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > This is a fairly substantial class now. I think it could be good to consider > moving it into a separate file in a followup CL. Yep, that's in the works, which I'll be detailing in the design doc. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:73: int timeout, On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > Could we just make this a member variable of the class rather than passing it > around? Done. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:84: ~PermissionsBlacklistingClient() override {} On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > nit: constructor before destructor usually Done. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:120: // TODO(meredithl) : fix this for post task On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > I'm not sure what this comment means? Sorry. Was an old comment for me to look at, forgot to remove it. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:132: // TODO(meredithl) : Implement correct permission string check. On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > Hmm I'm not sure what this one means either? Safe Browsing stringify the PermissionType enum, which don't match those returned by GetPermissionString(), so I need to do my own comparison or convert between the two. I'll update the comment to be more descriptive. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:147: if (is_active_) On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > It doesn't look like is_active_ is initialized anywhere. That would result in > undefined behavior. Done. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:157: Observe(nullptr); On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > Why is this needed? Is it just so we don't accidentally touch web_contents()? I believe so. Dominick suggested to put it in, and there are examples of it scattered (albeit inconsistently) throughout other parts of the code base. Whether or not it's actually needed here, I'm not sure. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:170: // True if |callback_| should be invoked. For instance, if web_contents() is On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > nit: Since the only reason why this would be false is if the WebContents is > destroyed, I think you can just remove the "For instance" here. Done. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:242: db_manager = sb_service->database_manager(); On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > Hmm, we never set the instance variable here. I think we want to have > db_manager_ = ... > > That should avoid needing the local variable as well, I think we could just use > db_manager_ Done. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:243: } On 2016/12/20 19:57:10, kcarattini wrote: > Do you need these 8 lines here or can you move them inside the if clause below > (and change the "if" to just check whether the feature is enabled)? You aren't > using the db_manager when the feature is off so there's no need for the extra > call to SB Service, and I'd prefer to move it closer to where it's being used. Ooh yep. Good catch. Done. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:256: // TODO(meredithl) : add UMA metrics here. On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > nit: here and elsewhere, no space before : in TODOs and capital letter at the > start of sentences, e.g. > TODO(raymes): The dog went to sleep. Done. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.h (right): https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:184: void SetSafeBrowsingDatabaseManagerForTests( On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > nit: ForTest (without this s) is a bit more common Done. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.h:187: void SetTimeoutForSafeBrowsingClient(int timeout); On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > nit: it should be made clear that this is just for tests too: > > SetSafeBrowingTimeoutForTest > > You could even consider just having the timeout as an extra param on the above > function too. Done. https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:862: url, 0 /* timeout in ms */, CONTENT_SETTING_ASK); On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > Is the timer guaranteed to fire before the test client returns? A parameter is passed to the test database manager which tells it whether it should run the resulting method on the client immediately, or sit and wait to try and mimic a timeout of Safe Browsing. Is this what you mean?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:111: quit_closure_); On 2016/12/29 06:23:34, meredithl wrote: > On 2016/12/20 23:58:56, raymes (OOO until Jan 9) wrote: > > On 2016/12/19 00:07:41, raymes wrote: > > > Do we need this line? Could we instead just call quit_closure_.Run() after > > > respond_permission_.Run() below? I might be missing something though, I know > > > this part was tricky :) > > > > ditto > > Sorry Raymes, I did miss these. DecidePermission isn't called for every > permission request, such as in the case of TestPermissionBlacklistingBlocked and > TestRequestPermissionInvalidUrl. These automatically block the request, and run > the callback, which for tests is TrackPermissionDecided. This meant that the run > loop was never exited and tests would hang, so a simple solution was to post the > quit_closure in TrackPermissionDecided to make sure the run loop exits for all > tests. Is there a more straightforward way of doing this do you think? Ah that makes sense, thanks for explaining. https://codereview.chromium.org/2555913002/diff/100001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:145: quit_closure_.Run(); On 2016/12/29 06:23:35, meredithl wrote: > On 2016/12/20 23:58:56, raymes (OOO until Jan 9) wrote: > > On 2016/12/19 00:07:41, raymes wrote: > > > It's probably good to clear the quit_closure_ after it's been run to ensure > > that > > > it's not run twice by mistake. > > > > ditto > > By "clear" do you mean set to null? > Sorry - yep exactly, you can call .Reset() https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:73: int timeout, On 2016/12/29 06:23:35, meredithl wrote: > On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > > Could we just make this a member variable of the class rather than passing it > > around? > > Done. Can we remove this as an argument from this function now and just use timeout_? https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:105: void StartCheck(const GURL& request_origin, int timeout) { nit: Can we remove timeout here and just use timeout_? https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:862: url, 0 /* timeout in ms */, CONTENT_SETTING_ASK); On 2016/12/29 06:23:35, meredithl wrote: > On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > > Is the timer guaranteed to fire before the test client returns? > > A parameter is passed to the test database manager which tells it whether it > should run the resulting method on the client immediately, or sit and wait to > try and mimic a timeout of Safe Browsing. Is this what you mean? Exactly! :) https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:69: std::map<GURL, std::set<std::string>>::iterator blacklisted_permissions = nit: It would be fine to use "auto" here. We also usually would use a const reference here, e.g. const auto& blacklisted_permissions = https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:80: if (perform_callback_) nit: can have DCHECK(!perform_callback_) here to avoid the if statement https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:126: quit_closure_); Can we just call quit_closure.Run() here instead of posting? https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:158: respond_permission_.Run(); Same here, we probably want to reset this after running it https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:160: quit_closure_.Run(); I wonder if we can just remove this line? https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:810: new MockSafeBrowsingDatabaseManager(true); nit: here and below, document the bool parameter that gets passed in. We always just put the parameter name in here, so in this case: new MockSafeBrowsingDatabaseManager(true /* perform_callback */); https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:826: true /* run client with result method immediately */); nit: just put perform_callback. If you want to add an additional comment, you can do that above this statement as usual. But I don't think it's necessary in this case. https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:846: false /* don't call client with result */); nit: same thing here
Thanks! https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:73: int timeout, On 2017/01/09 06:28:17, raymes (a bit slow) wrote: > On 2016/12/29 06:23:35, meredithl wrote: > > On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > > > Could we just make this a member variable of the class rather than passing > it > > > around? > > > > Done. > > Can we remove this as an argument from this function now and just use timeout_? It gets passed from here into the constructor to be set as a member variable there, is that sufficient? Or should timeout_ be static so it can be initialized elsewhere? https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:105: void StartCheck(const GURL& request_origin, int timeout) { On 2017/01/09 06:28:17, raymes (a bit slow) wrote: > nit: Can we remove timeout here and just use timeout_? Done. https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:69: std::map<GURL, std::set<std::string>>::iterator blacklisted_permissions = On 2017/01/09 06:28:18, raymes (a bit slow) wrote: > nit: It would be fine to use "auto" here. We also usually would use a const > reference here, e.g. > const auto& blacklisted_permissions = Done. Still getting used to C++11 :) https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:80: if (perform_callback_) On 2017/01/09 06:28:17, raymes (a bit slow) wrote: > nit: can have DCHECK(!perform_callback_) here to avoid the if statement Done. https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:126: quit_closure_); On 2017/01/09 06:28:17, raymes (a bit slow) wrote: > Can we just call quit_closure.Run() here instead of posting? It appears we can! These test methods have been hacked into place, so this must have been a hangover from how we were previously doing it. https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:158: respond_permission_.Run(); On 2017/01/09 06:28:17, raymes (a bit slow) wrote: > Same here, we probably want to reset this after running it Done. https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:160: quit_closure_.Run(); On 2017/01/09 06:28:18, raymes (a bit slow) wrote: > I wonder if we can just remove this line? respond_permission_ only gets set in the TestParallelRequests* methods after the first request is made, which means for the first request the run loop just spins indefinitely, because the quit_closure_ only gets run from the callback. I'll add a comment explaining this, unless there is a better way to achieve the same behaviour. It is a bit messy :/ https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:810: new MockSafeBrowsingDatabaseManager(true); On 2017/01/09 06:28:18, raymes (a bit slow) wrote: > nit: here and below, document the bool parameter that gets passed in. We always > just put the parameter name in here, so in this case: > new MockSafeBrowsingDatabaseManager(true /* perform_callback */); Done. https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:826: true /* run client with result method immediately */); On 2017/01/09 06:28:18, raymes (a bit slow) wrote: > nit: just put perform_callback. If you want to add an additional comment, you > can do that above this statement as usual. But I don't think it's necessary in > this case. Done. https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:846: false /* don't call client with result */); On 2017/01/09 06:28:18, raymes (a bit slow) wrote: > nit: same thing here Done.
lgtm https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:73: int timeout, On 2017/01/10 02:24:57, meredithl wrote: > On 2017/01/09 06:28:17, raymes (a bit slow) wrote: > > On 2016/12/29 06:23:35, meredithl wrote: > > > On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > > > > Could we just make this a member variable of the class rather than passing > > it > > > > around? > > > > > > Done. > > > > Can we remove this as an argument from this function now and just use > timeout_? > > It gets passed from here into the constructor to be set as a member variable > there, is that sufficient? Or should timeout_ be static so it can be initialized > elsewhere? Oh sorry, you're right, we need it here :) https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:98: is_active_(true) /* Web Contents is active upon construction */ { nit: don't use an inline comment here. I think it's ok to omit the comment in this case https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:641: base::Unretained(this), &permission_context, id, url, false, response)); Is this needed? https://codereview.chromium.org/2555913002/diff/260001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2555913002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:123: if (quit_closure_) { Add a comment explaining why this might be a null callback (i.e. in the case where quit_closure_ gets called in DecidePermission) https://codereview.chromium.org/2555913002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:160: } else if (quit_closure_) { Can we assume quit_closure_ is not null here?
https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/200001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:73: int timeout, On 2017/01/10 02:57:15, raymes (a bit slow) wrote: > On 2017/01/10 02:24:57, meredithl wrote: > > On 2017/01/09 06:28:17, raymes (a bit slow) wrote: > > > On 2016/12/29 06:23:35, meredithl wrote: > > > > On 2016/12/20 23:58:57, raymes (OOO until Jan 9) wrote: > > > > > Could we just make this a member variable of the class rather than > passing > > > it > > > > > around? > > > > > > > > Done. > > > > > > Can we remove this as an argument from this function now and just use > > timeout_? > > > > It gets passed from here into the constructor to be set as a member variable > > there, is that sufficient? Or should timeout_ be static so it can be > initialized > > elsewhere? > > Oh sorry, you're right, we need it here :) Acknowledged. https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base.cc (right): https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base.cc:98: is_active_(true) /* Web Contents is active upon construction */ { On 2017/01/10 02:57:15, raymes (a bit slow) wrote: > nit: don't use an inline comment here. I think it's ok to omit the comment in > this case Done. https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2555913002/diff/240001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:641: base::Unretained(this), &permission_context, id, url, false, response)); On 2017/01/10 02:57:15, raymes (a bit slow) wrote: > Is this needed? Turns out for simplicity it is, because we also want to test that a decision can be made when a) the site isn't blacklisted and b) safe browsing times out. https://codereview.chromium.org/2555913002/diff/260001/chrome/browser/permiss... File chrome/browser/permissions/permission_context_base_unittest.cc (right): https://codereview.chromium.org/2555913002/diff/260001/chrome/browser/permiss... chrome/browser/permissions/permission_context_base_unittest.cc:160: } else if (quit_closure_) { On 2017/01/10 02:57:15, raymes (a bit slow) wrote: > Can we assume quit_closure_ is not null here? I assume its safe to, as if decide_permission_ is set it should always run before TrackPermissionDecision, which has the null check.
The CQ bit was checked by meredithl@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by meredithl@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, nparker@chromium.org, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2555913002/#ps280001 (title: "Comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1484026489662070,
"parent_rev": "d8b62ea53246ab528786b9a8d312d0a3733e46e9", "commit_rev":
"62b8c3d48cfc7b4e090c0d33f409559b299fd9a1"}
Message was sent while issue was closed.
Description was changed from ========== Implement origin specific Permissions Blacklisting. For Chrome users that have enabled Safe Browsing, when a new permission request is made, the requesting origin is first checked with Safe Browsing. If Safe Browsing has blacklisted the origin for the requested permission, that permission is autoblocked. The origin will only be autoblocked for blacklisted permissions, any other permissions will default to the normal prompting for a decision from the user. Unit tests added to permission_context_base_unittest.cc. Further tests to follow. Permissions Blacklisting will also require an update to the UI, which will also follow. BUG=561867 ========== to ========== Implement origin specific Permissions Blacklisting. For Chrome users that have enabled Safe Browsing, when a new permission request is made, the requesting origin is first checked with Safe Browsing. If Safe Browsing has blacklisted the origin for the requested permission, that permission is autoblocked. The origin will only be autoblocked for blacklisted permissions, any other permissions will default to the normal prompting for a decision from the user. Unit tests added to permission_context_base_unittest.cc. Further tests to follow. Permissions Blacklisting will also require an update to the UI, which will also follow. BUG=561867 Review-Url: https://codereview.chromium.org/2555913002 Cr-Commit-Position: refs/heads/master@{#442501} Committed: https://chromium.googlesource.com/chromium/src/+/62b8c3d48cfc7b4e090c0d33f409... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/62b8c3d48cfc7b4e090c0d33f409... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
