Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1700943003:
SafeBrowsing: DatabaseManager owns the V4GetHashProtocolManager (Closed)
|
Created:
4 years, 10 months ago by kcarattini Modified:
4 years, 10 months ago CC:
chromium-reviews, grt+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSafeBrowsing: DatabaseManager owns the V4GetHashProtocolManager
BUG=543161, 561867
Committed: https://crrev.com/95bba9fad50442a9ce7ae4fbcc50d1d845d5d399
Cr-Commit-Position: refs/heads/master@{#376088}
Committed: https://crrev.com/3110e563a36a59145bfa2c20c1b10460b761c1cc
Cr-Commit-Position: refs/heads/master@{#376676}
Patch Set 1 #Patch Set 2 : Make constructor explicit, remove unneeded include #Patch Set 3 : Fix BUILD file #
Total comments: 9
Patch Set 4 : Review Comments #Patch Set 5 : Move back to public #Patch Set 6 : Attempt to appease the trybots! #Patch Set 7 : Pass NULL for the url_context_getter #Messages
Total messages: 40 (15 generated)
kcarattini@chromium.org changed reviewers: + nparker@chromium.org, vakh@chromium.org
The CQ bit was checked by kcarattini@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1700943003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1700943003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by kcarattini@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1700943003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1700943003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
lgtm https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/local_database_manager.h (right): https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/local_database_manager.h:101: explicit LocalSafeBrowsingDatabaseManager( Can we get rid of the other constructor yet, to ensure we don't miss some cases? Or do you want to leave it for simplicity? If the latter, maybe it should be just for testing. https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service.h:105: virtual SafeBrowsingProtocolConfig GetProtocolConfig() const; Do these need to be public? https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service.h:108: virtual V4GetHashProtocolConfig GetV4GetHashProtocolConfig() const; I probably missed this before... but would this be better as "V4ProtocolConfig()", so it can be shared between get full hash code and update code? I realize I'm adding lots of constraints for steering this towards future compatibility, but these changes will make it a lot cleaner later. Thank you! https://codereview.chromium.org/1700943003/diff/40001/components/safe_browsin... File components/safe_browsing_db/database_manager.h (right): https://codereview.chromium.org/1700943003/diff/40001/components/safe_browsin... components/safe_browsing_db/database_manager.h:164: std::unique_ptr<V4GetHashProtocolManager> v4_get_hash_protocol_manager_; Hmm, didn't know we were using unique_ptr's (rather than scoped_ptr), but this bug implies that's the desire: https://code.google.com/p/chromium/issues/detail?id=554298. sgtm.
https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/local_database_manager.h (right): https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/local_database_manager.h:101: explicit LocalSafeBrowsingDatabaseManager( On 2016/02/17 15:53:27, nparker wrote: > Can we get rid of the other constructor yet, to ensure we don't miss some cases? > Or do you want to leave it for simplicity? If the latter, maybe it should be > just for testing. I think having two is simpler so I don't have to go change all the tests. I'll add a comment that this is just for testing. Plus I get to use C++11's fancy new constructor delegation! :) https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service.h:108: virtual V4GetHashProtocolConfig GetV4GetHashProtocolConfig() const; On 2016/02/17 15:53:28, nparker wrote: > I probably missed this before... but would this be better as > "V4ProtocolConfig()", so it can be shared between get full hash code and update > code? > > I realize I'm adding lots of constraints for steering this towards future > compatibility, but these changes will make it a lot cleaner later. Thank you! I figured the Update config would need a lot more than the GetHash config which is why I separated them. If you prefer to use the same one, I'll rename and move to util.cc in a followup cl.
On 2016/02/17 22:44:42, kcarattini wrote: > https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... > File chrome/browser/safe_browsing/local_database_manager.h (right): > > https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/local_database_manager.h:101: explicit > LocalSafeBrowsingDatabaseManager( > On 2016/02/17 15:53:27, nparker wrote: > > Can we get rid of the other constructor yet, to ensure we don't miss some > cases? > > Or do you want to leave it for simplicity? If the latter, maybe it should be > > just for testing. > > I think having two is simpler so I don't have to go change all the tests. I'll > add a comment that this is just for testing. Plus I get to use C++11's fancy new > constructor delegation! :) > > https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... > File chrome/browser/safe_browsing/safe_browsing_service.h (right): > > https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/safe_browsing_service.h:108: virtual > V4GetHashProtocolConfig GetV4GetHashProtocolConfig() const; > On 2016/02/17 15:53:28, nparker wrote: > > I probably missed this before... but would this be better as > > "V4ProtocolConfig()", so it can be shared between get full hash code and > update > > code? > > > > I realize I'm adding lots of constraints for steering this towards future > > compatibility, but these changes will make it a lot cleaner later. Thank you! > > I figured the Update config would need a lot more than the GetHash config which > is why I separated them. If you prefer to use the same one, I'll rename and > move to util.cc in a followup cl. I'm working on making that change so you can submit this as-is.
Thanks! Varun, still waiting for lgtm from you (and the trybots). Kendra https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/local_database_manager.h (right): https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/local_database_manager.h:101: explicit LocalSafeBrowsingDatabaseManager( On 2016/02/17 22:44:42, kcarattini wrote: > On 2016/02/17 15:53:27, nparker wrote: > > Can we get rid of the other constructor yet, to ensure we don't miss some > cases? > > Or do you want to leave it for simplicity? If the latter, maybe it should be > > just for testing. > > I think having two is simpler so I don't have to go change all the tests. I'll > add a comment that this is just for testing. Plus I get to use C++11's fancy new > constructor delegation! :) Done. https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_service.h (right): https://codereview.chromium.org/1700943003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_service.h:105: virtual SafeBrowsingProtocolConfig GetProtocolConfig() const; On 2016/02/17 15:53:28, nparker wrote: > Do these need to be public? Turns out yes -- blacklist_state_fetcher calls it. https://codereview.chromium.org/1700943003/diff/40001/components/safe_browsin... File components/safe_browsing_db/database_manager.h (right): https://codereview.chromium.org/1700943003/diff/40001/components/safe_browsin... components/safe_browsing_db/database_manager.h:164: std::unique_ptr<V4GetHashProtocolManager> v4_get_hash_protocol_manager_; On 2016/02/17 15:53:28, nparker wrote: > Hmm, didn't know we were using unique_ptr's (rather than scoped_ptr), but this > bug implies that's the desire: > https://code.google.com/p/chromium/issues/detail?id=554298. sgtm. Acknowledged.
lgtm
The CQ bit was checked by kcarattini@chromium.org to run a CQ dry run
Here's the CL where I am migrating some of the common code to v4_protocol_manager_util file. This file can then be included in the protocol manager for GetHash and Update.
On 2016/02/18 03:00:56, vakh wrote: > Here's the CL where I am migrating some of the common code to > v4_protocol_manager_util file. This file can then be included in the protocol > manager for GetHash and Update. Work in progress, I must add.
On 2016/02/18 03:01:12, vakh wrote: > On 2016/02/18 03:00:56, vakh wrote: > > Here's the CL where I am migrating some of the common code to > > v4_protocol_manager_util file. This file can then be included in the protocol > > manager for GetHash and Update. > > Work in progress, I must add. Oh, and the CL itself: https://codereview.chromium.org/1703413002/ :)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1700943003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1700943003/100001
Thanks! Please cc me on the review when it's ready. Kendra
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 kcarattini@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org, vakh@chromium.org Link to the patchset: https://codereview.chromium.org/1700943003/#ps100001 (title: "Attempt to appease the trybots!")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1700943003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1700943003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== SafeBrowsing: DatabaseManager owns the V4GetHashProtocolManager BUG=543161,561867 ========== to ========== SafeBrowsing: DatabaseManager owns the V4GetHashProtocolManager BUG=543161,561867 Committed: https://crrev.com/95bba9fad50442a9ce7ae4fbcc50d1d845d5d399 Cr-Commit-Position: refs/heads/master@{#376088} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/95bba9fad50442a9ce7ae4fbcc50d1d845d5d399 Cr-Commit-Position: refs/heads/master@{#376088}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/1709943002/ by henrika@chromium.org. The reason for reverting is: Speculative revert as Chrome sheriff. I suspect that this CL causes tons of tests on Mac to fail. See e.g. https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.9%20Tests%... Callstacks in all failing tests contains references to SafeBrowsing. 1 libbase.dylib 0x00000001195efbc3 _ZN4base5debug10StackTraceC1Ev + 35 2 sync_integration_tests 0x000000010a77225a _ZN4base5debug11LeakTrackerI29SystemURLRequestContextGetterEC2Ev + 58 3 sync_integration_tests 0x000000010a757783 _ZN4base5debug11LeakTrackerI29SystemURLRequestContextGetterEC1Ev + 35 4 sync_integration_tests 0x000000010a757739 _ZN29SystemURLRequestContextGetterC2EP8IOThread + 105 5 sync_integration_tests 0x000000010a7577db _ZN29SystemURLRequestContextGetterC1EP8IOThread + 43 6 sync_integration_tests 0x000000010a75c9a6 _ZN8IOThread24InitSystemRequestContextEv + 246 7 sync_integration_tests 0x000000010a75c866 _ZN8IOThread33system_url_request_context_getterEv + 358 8 sync_integration_tests 0x000000010a9b7cae _ZN18BrowserProcessImpl22system_request_contextEv + 270 9 sync_integration_tests 0x000000010b1c0e55 _ZN13safe_browsing19SafeBrowsingService10InitializeEv + 101 10 sync_integration_tests 0x000000010a9bc17b _ZN18BrowserProcessImpl25CreateSafeBrowsingServiceEv + 331 11 sync_integration_tests 0x000000010a9bbfe3 _ZN18BrowserProcessImpl21safe_browsing_serviceEv + 275 12 sync_integration_tests 0x000000010a8acf61 _ZN36ChromeResourceDispatcherHostDelegateC2Ev + 177 13 sync_integration_tests 0x000000010a8ad1d3 _ZN36ChromeResourceDispatcherHostDelegateC1Ev + 35 14 sync_integration_tests 0x000000010a9bcda5 _ZN18BrowserProcessImpl29ResourceDispatcherHostCreatedEv + 117 15 sync_integration_tests 0x000000010a6141c0 _ZN26ChromeContentBrowserClient29ResourceDispatcherHostCreatedEv + 320 16 libcontent.dylib 0x00000001212ba928 _ZN7content26ResourceDispatcherHostImplC2Ev + 1320 17 libcontent.dylib 0x00000001212baf63 _ZN7content26ResourceDispatcherHostImplC1Ev + 35.
Message was sent while issue was closed.
I don't only see this on Mac but on practically all platforms.
Message was sent while issue was closed.
Description was changed from ========== SafeBrowsing: DatabaseManager owns the V4GetHashProtocolManager BUG=543161,561867 Committed: https://crrev.com/95bba9fad50442a9ce7ae4fbcc50d1d845d5d399 Cr-Commit-Position: refs/heads/master@{#376088} ========== to ========== SafeBrowsing: DatabaseManager owns the V4GetHashProtocolManager BUG=543161,561867 Committed: https://crrev.com/95bba9fad50442a9ce7ae4fbcc50d1d845d5d399 Cr-Commit-Position: refs/heads/master@{#376088} ==========
There's an issue with passing the url_request_context_getter_ to the database manager on the UI thread. Passing null for now while I refactor in a followup cl the abaility to pass on the IO thread.
The CQ bit was checked by kcarattini@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org, vakh@chromium.org Link to the patchset: https://codereview.chromium.org/1700943003/#ps120001 (title: "Pass NULL for the url_context_getter")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1700943003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1700943003/120001
Message was sent while issue was closed.
Description was changed from ========== SafeBrowsing: DatabaseManager owns the V4GetHashProtocolManager BUG=543161,561867 Committed: https://crrev.com/95bba9fad50442a9ce7ae4fbcc50d1d845d5d399 Cr-Commit-Position: refs/heads/master@{#376088} ========== to ========== SafeBrowsing: DatabaseManager owns the V4GetHashProtocolManager BUG=543161,561867 Committed: https://crrev.com/95bba9fad50442a9ce7ae4fbcc50d1d845d5d399 Cr-Commit-Position: refs/heads/master@{#376088} ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== SafeBrowsing: DatabaseManager owns the V4GetHashProtocolManager BUG=543161,561867 Committed: https://crrev.com/95bba9fad50442a9ce7ae4fbcc50d1d845d5d399 Cr-Commit-Position: refs/heads/master@{#376088} ========== to ========== SafeBrowsing: DatabaseManager owns the V4GetHashProtocolManager BUG=543161,561867 Committed: https://crrev.com/95bba9fad50442a9ce7ae4fbcc50d1d845d5d399 Cr-Commit-Position: refs/heads/master@{#376088} Committed: https://crrev.com/3110e563a36a59145bfa2c20c1b10460b761c1cc Cr-Commit-Position: refs/heads/master@{#376676} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3110e563a36a59145bfa2c20c1b10460b761c1cc Cr-Commit-Position: refs/heads/master@{#376676} |
