Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
910953002:
Move SafeBrowsing to the blocking pool via an experiment. (Closed)
|
Created:
5 years, 10 months ago by Alexei Svitkine (slow) Modified:
5 years, 10 months ago CC:
chromium-reviews, grt+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove SafeBrowsing to the blocking pool via an experiment.
Most of the change is updating the threading checks.
Using an experiment so that we can see what impact, if any,
this has on start up performance. I suspect it's small, since
it's just one of a thousand cuts - but we'll see.
Plan is to clean out the experiment in a follow-up CL (making
the new behavior the default) once we've gotten enough data
from the wild.
BUG=450037
Committed: https://crrev.com/be033cca03e50e1bba363d7dc2e56ad38f9196da
Cr-Commit-Position: refs/heads/master@{#317857}
Patch Set 1 : (Version which defaults to new behavior to verify bots.) #Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 29
Patch Set 4 : Address gab's comments. #Patch Set 5 : Fix method name in test. #
Total comments: 25
Patch Set 6 : Address more comments. #
Total comments: 5
Patch Set 7 : Rebase. #Patch Set 8 : Address more comments. #Patch Set 9 : Fix browsertest. #Messages
Total messages: 29 (12 generated)
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #1 (id:80001) has been deleted
asvitkine@chromium.org changed reviewers: + gab@chromium.org
gab: PTAL This turned out more hairy than I expected.
grt@chromium.org changed reviewers: + grt@chromium.org
https://codereview.chromium.org/910953002/diff/130001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/910953002/diff/130001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:1623: base::MessageLoop::current()->PostTask(FROM_HERE, threads in the blocking pool run a message loop, so this probably won't work. db_task_runner_->PostTask accomplishes the desired effect, no?
https://codereview.chromium.org/910953002/diff/130001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/910953002/diff/130001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:1623: base::MessageLoop::current()->PostTask(FROM_HERE, On 2015/02/18 19:14:05, grt wrote: > threads in the blocking pool run a message loop, so this probably won't work. > db_task_runner_->PostTask accomplishes the desired effect, no? Done.
Woot, lg, thanks! https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/database_manager.cc (right): https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:658: // Don't create the thread under "BlockingPool" experimental group - instead "BlockingPool" is not really an experimental group, is it? It's more a variation of LightSpeed.SBThreadingMode? (you know more than I, but this wording feels wrong to me) https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:665: pool->GetSequencedTaskRunner(pool->GetSequenceToken()); I think we also want SKIP_ON_SHUTDOWN behaviour here. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:765: safe_browsing_thread_.reset(); Previously this guaranteed that the |safe_browsing_thread_| went away before the IO thread (which was not necessarily a good thing as blocking the IO thread is not typically desired, but it did provide this guarantee). Now with the BlockingPool, I'm not sure what the shutdown properties are, worth digging into to make sure it's still okay. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/database_manager.h (right): https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.h:403: // is only ever accessed in sequence via the runner. It's mainly used to post tasks to the safe_browsing_thread_ of BlockingPool variants thereof -- verification is only secondary isn't it? https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:678: // DDCHECK(db_task_runner_->RunsTasksOnCurrentThread()); s/DDCHECK/DCHECK https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:868: SafeBrowsingDatabaseNew::DatabaseStateManager::DatabaseStateManager( Please insert these two just below ~ThreadSafeStateManager(){} above on line 630 (I know this is not exactly where they line up in the header, but when I added those I decided to put them between SafeBrowsingDatabase:: and SafeBrowsingDatabaseNew:: implementation as otherwise having an implementation in the middle of another one felt weird). (and of course I just realized that SafeBrowsingDatabaseNew::GetStore() is misaligned, screwing most of that up anyways...) https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.h (right): https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.h:510: // is only ever accessed in sequence via the runner. s/in sequence via the runner/via the runner (i.e. sequence can't be verified from this object, it's just guaranteed by it) https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_store_file.cc (right): https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_store_file.cc:567: task_runner_ = nullptr; No need to do this, just have a commented out: // DCHECK(task_runner_->RunsTasksOnCurrentThread()); with the same comment above. The reason this was like this before is that the DCHECK was made in the parent NonThreadSafe class' destructor. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_store_file.cc:573: return !task_runner_ || task_runner_->RunsTasksOnCurrentThread(); You can get rid of this I think and just inline DCHECK(task_runner_->RunsTasksOnCurrentThread()); everywhere I think given the above suggestion. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_store_file.h (right): https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_store_file.h:17: #include "base/threading/non_thread_safe.h" rm this include https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_store_file.h:238: // is only ever accessed in sequence via the runner. s/in sequence via the runner/from the runner https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc (right): https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc:60: corruption_detected_ = false; Remove this line now that it's properly taken care of in the constructor. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc:757: store_.reset(new SafeBrowsingStoreFile(task_runner_)); ASSERT_TRUE(!task_runner_->HasPendingTasks()); before this line maybe? Otherwise this doesn't really simulate a "crash" as the task_runner_ is not empty. Same on 290.
https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:1623: db_task_runner_->PostTask( Oh also, interesting that this posts back to itself, I thought the comments on safe_browsing_thread_ said this wasn't allowed.. (just an observation, nothing you're making worse FWIW).
asvitkine@chromium.org changed reviewers: + mattm@chromium.org
Addressed comments, PTAL. Also +mattm@ for owners review. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/database_manager.cc (right): https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:658: // Don't create the thread under "BlockingPool" experimental group - instead On 2015/02/19 14:38:22, gab wrote: > "BlockingPool" is not really an experimental group, is it? It's more a variation > of LightSpeed.SBThreadingMode? (you know more than I, but this wording feels > wrong to me) Updated comment. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:665: pool->GetSequencedTaskRunner(pool->GetSequenceToken()); On 2015/02/19 14:38:22, gab wrote: > I think we also want SKIP_ON_SHUTDOWN behaviour here. Done. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:765: safe_browsing_thread_.reset(); On 2015/02/19 14:38:22, gab wrote: > Previously this guaranteed that the |safe_browsing_thread_| went away before the > IO thread (which was not necessarily a good thing as blocking the IO thread is > not typically desired, but it did provide this guarantee). > > Now with the BlockingPool, I'm not sure what the shutdown properties are, worth > digging into to make sure it's still okay. Hmm, so by switching to SKIP_ON_SHUTDOWN as you suggested in another comment, this will be a behavior change. This previous code would actually run all pending tasks (synchronously on IO thread). SKIP_ON_SHUTDOWN will just skip running them. I think ideally we want SKIP_ON_SHUTDOWN - i.e. if we can be in a state where just ignoring any pending tasks doesn't result in any corruption, etc. I guess I don't know enough about SB to know whether this is the case - however since this is equivalent to what would happen on e.g. a crash, I think there's a good chance that it actually should be OK (else crashes would cause a similar type of corruption and expose the same problems). Also, I took a cursory look at the ownership model and ultimately it looks like the tree of SB objects is own by SB service which hangs off the g_browser_process which is shut down on the UI thread. I believe UI thread is the last to go, so this should be safe in terms of ownership (i.e. the underlying SB objects shouldn't go away while tasks are still trying to run). At least, as far as I understand. So I propose to go ahead with SKIP_ON_SHUTDOWN behavior and if there are any problems (e.g. crashes), the experiment will reveal them. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/database_manager.h (right): https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.h:403: // is only ever accessed in sequence via the runner. On 2015/02/19 14:38:23, gab wrote: > It's mainly used to post tasks to the safe_browsing_thread_ of BlockingPool > variants thereof -- verification is only secondary isn't it? Indeed. Updated comment. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:678: // DDCHECK(db_task_runner_->RunsTasksOnCurrentThread()); On 2015/02/19 14:38:23, gab wrote: > s/DDCHECK/DCHECK Done. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:868: SafeBrowsingDatabaseNew::DatabaseStateManager::DatabaseStateManager( On 2015/02/19 14:38:23, gab wrote: > Please insert these two just below ~ThreadSafeStateManager(){} above on line 630 > (I know this is not exactly where they line up in the header, but when I added > those I decided to put them between SafeBrowsingDatabase:: and > SafeBrowsingDatabaseNew:: implementation as otherwise having an implementation > in the middle of another one felt weird). > > (and of course I just realized that SafeBrowsingDatabaseNew::GetStore() is > misaligned, screwing most of that up anyways...) Done. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:1623: db_task_runner_->PostTask( On 2015/02/19 14:41:20, gab wrote: > Oh also, interesting that this posts back to itself, I thought the comments on > safe_browsing_thread_ said this wasn't allowed.. (just an observation, nothing > you're making worse FWIW). Acknowledged. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.h (right): https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.h:510: // is only ever accessed in sequence via the runner. On 2015/02/19 14:38:23, gab wrote: > s/in sequence via the runner/via the runner > > (i.e. sequence can't be verified from this object, it's just guaranteed by it) Done. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_store_file.cc (right): https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_store_file.cc:567: task_runner_ = nullptr; On 2015/02/19 14:38:23, gab wrote: > No need to do this, just have a commented out: > > // DCHECK(task_runner_->RunsTasksOnCurrentThread()); > > with the same comment above. > > The reason this was like this before is that the DCHECK was made in the parent > NonThreadSafe class' destructor. The issue is that Close() has that check. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_store_file.cc:573: return !task_runner_ || task_runner_->RunsTasksOnCurrentThread(); On 2015/02/19 14:38:23, gab wrote: > You can get rid of this I think and just inline > > DCHECK(task_runner_->RunsTasksOnCurrentThread()); > > everywhere I think given the above suggestion. I prefer keeping it a separate method, which doesn't add any overhead in official builds (since its only called from inside DCHECKs and the unused method will be stripped out by the linker) while making the checks both more readable (i.e. don't need to read impl. details of how the check is done) and more easily updatable if this is part of the code is refactored again. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_store_file.h (right): https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_store_file.h:17: #include "base/threading/non_thread_safe.h" On 2015/02/19 14:38:23, gab wrote: > rm this include Done. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_store_file.h:238: // is only ever accessed in sequence via the runner. On 2015/02/19 14:38:23, gab wrote: > s/in sequence via the runner/from the runner Done. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc (right): https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc:60: corruption_detected_ = false; On 2015/02/19 14:38:23, gab wrote: > Remove this line now that it's properly taken care of in the constructor. Done. https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc:757: store_.reset(new SafeBrowsingStoreFile(task_runner_)); On 2015/02/19 14:38:23, gab wrote: > ASSERT_TRUE(!task_runner_->HasPendingTasks()); > > before this line maybe? Otherwise this doesn't really simulate a "crash" as the > task_runner_ is not empty. > > Same on 290. Done.
lgtm https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/database_manager.cc (right): https://codereview.chromium.org/910953002/diff/150001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:765: safe_browsing_thread_.reset(); On 2015/02/20 15:42:44, Alexei Svitkine wrote: > On 2015/02/19 14:38:22, gab wrote: > > Previously this guaranteed that the |safe_browsing_thread_| went away before > the > > IO thread (which was not necessarily a good thing as blocking the IO thread is > > not typically desired, but it did provide this guarantee). > > > > Now with the BlockingPool, I'm not sure what the shutdown properties are, > worth > > digging into to make sure it's still okay. > > Hmm, so by switching to SKIP_ON_SHUTDOWN as you suggested in another comment, > this will be a behavior change. This previous code would actually run all > pending tasks (synchronously on IO thread). > > SKIP_ON_SHUTDOWN will just skip running them. > > I think ideally we want SKIP_ON_SHUTDOWN - i.e. if we can be in a state where > just ignoring any pending tasks doesn't result in any corruption, etc. I guess I > don't know enough about SB to know whether this is the case - however since this > is equivalent to what would happen on e.g. a crash, I think there's a good > chance that it actually should be OK (else crashes would cause a similar type of > corruption and expose the same problems). > > Also, I took a cursory look at the ownership model and ultimately it looks like > the tree of SB objects is own by SB service which hangs off the > g_browser_process which is shut down on the UI thread. I believe UI thread is > the last to go, so this should be safe in terms of ownership (i.e. the > underlying SB objects shouldn't go away while tasks are still trying to run). At > least, as far as I understand. > > So I propose to go ahead with SKIP_ON_SHUTDOWN behavior and if there are any > problems (e.g. crashes), the experiment will reveal them. Should also keep an eye on the FAILURE_DATABASE_CORRUPT_HANDLER histogram to see if people in the experiment have any higher instance of needing to reset the database. https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/database_manager.cc (right): https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:1084: void SafeBrowsingDatabaseManager::CheckDownloadUrlOnSBThread( Should also keep an eye on SB2.DownloadUrlCheckDuration histogram to see if moving the checks to the blocking pool has a negative effect on that (Dunno how much contention there is for the blocking pool.)
lg, couple comments, mostly around more checks and const-ness. Thanks, Gab https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/database_manager.cc (left): https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:653: DCHECK(!safe_browsing_thread_.get()); This DCHECK ensured StartOnIOThread() was only ever called once, perhaps add DCHECK(!safe_browsing_task_runner_) at the top of this method to keep this check? https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/database_manager.cc (right): https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:660: const std::string sb_threading_mode = How about const bool use_blocking_pool = variations::...() == "BlockingPool"; instead of splitting the logic between this statement and the conditional below? https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:1084: void SafeBrowsingDatabaseManager::CheckDownloadUrlOnSBThread( On 2015/02/21 02:03:06, mattm wrote: > Should also keep an eye on SB2.DownloadUrlCheckDuration histogram to see if > moving the checks to the blocking pool has a negative effect on that (Dunno how > much contention there is for the blocking pool.) Side-note: we are also contemplating reducing thread priority on the blocking pool, perhaps the download list should just be updated on the |safe_browsing_task_runner_| while being thread-safe to lookup (like most other lists)? https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:364: // on SafeBrowsing Thread. Update this comment and enforce it now that we have access to the task runner? https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.h (right): https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.h:37: scoped_refptr<base::SequencedTaskRunner> db_task_runner, I think this can be scoped_refptr<const base::SequencedTaskRunner> for the same reasons as suggested below (only needed for checks, shouldn't be used to post tasks). https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.h:412: scoped_refptr<base::SequencedTaskRunner> db_task_runner); const, i.e.: scoped_refptr<const base::SequencedTaskRunner> (should only be used for verification, not to post tasks) https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.h:422: scoped_refptr<base::SequencedTaskRunner> db_task_runner_; scoped_refptr<const base::SequencedTaskRunner> https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.h:511: scoped_refptr<base::SequencedTaskRunner> db_task_runner_; scoped_refptr<const base::SequencedTaskRunner> https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.h:640: scoped_refptr<base::SequencedTaskRunner> db_task_runner_; scoped_refptr<const base::SequencedTaskRunner> https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_store_file.h (right): https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_store_file.h:129: scoped_refptr<base::SequencedTaskRunner> task_runner); Also const
New patchsets have been uploaded after l-g-t-m from mattm@chromium.org
PTAL. https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/database_manager.cc (left): https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:653: DCHECK(!safe_browsing_thread_.get()); On 2015/02/23 16:33:55, gab wrote: > This DCHECK ensured StartOnIOThread() was only ever called once, perhaps add > DCHECK(!safe_browsing_task_runner_) at the top of this method to keep this > check? The "if (enabled_) return;" code suggests that this actually can be called multiple times. I'd rather not change that in this CL, so added the check below that to match the previous check location. https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/database_manager.cc (right): https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:660: const std::string sb_threading_mode = On 2015/02/23 16:33:55, gab wrote: > How about > > const bool use_blocking_pool = variations::...() == "BlockingPool"; > > instead of splitting the logic between this statement and the conditional below? Done. https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:1084: void SafeBrowsingDatabaseManager::CheckDownloadUrlOnSBThread( On 2015/02/23 16:33:55, gab wrote: > On 2015/02/21 02:03:06, mattm wrote: > > Should also keep an eye on SB2.DownloadUrlCheckDuration histogram to see if > > moving the checks to the blocking pool has a negative effect on that (Dunno > how > > much contention there is for the blocking pool.) > > Side-note: we are also contemplating reducing thread priority on the blocking > pool, perhaps the download list should just be updated on the > |safe_browsing_task_runner_| while being thread-safe to lookup (like most other > lists)? Acknowledged. https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:364: // on SafeBrowsing Thread. On 2015/02/23 16:33:55, gab wrote: > Update this comment and enforce it now that we have access to the task runner? Done. https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.h (right): https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.h:37: scoped_refptr<base::SequencedTaskRunner> db_task_runner, On 2015/02/23 16:33:55, gab wrote: > I think this can be > > scoped_refptr<const base::SequencedTaskRunner> > > for the same reasons as suggested below (only needed for checks, shouldn't be > used to post tasks). Can't be, see comment later. https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.h:412: scoped_refptr<base::SequencedTaskRunner> db_task_runner); On 2015/02/23 16:33:55, gab wrote: > const, i.e.: > > scoped_refptr<const base::SequencedTaskRunner> > > (should only be used for verification, not to post tasks) Done. https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.h:422: scoped_refptr<base::SequencedTaskRunner> db_task_runner_; On 2015/02/23 16:33:55, gab wrote: > scoped_refptr<const base::SequencedTaskRunner> Done. https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.h:511: scoped_refptr<base::SequencedTaskRunner> db_task_runner_; On 2015/02/23 16:33:55, gab wrote: > scoped_refptr<const base::SequencedTaskRunner> Done. https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.h:640: scoped_refptr<base::SequencedTaskRunner> db_task_runner_; On 2015/02/23 16:33:55, gab wrote: > scoped_refptr<const base::SequencedTaskRunner> SafeBrowsingDatabaseNew::HandleCorruptDatabase() does a post task on the runner, so can't be const. I've updated the comment. https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_store_file.h (right): https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_store_file.h:129: scoped_refptr<base::SequencedTaskRunner> task_runner); On 2015/02/23 16:33:55, gab wrote: > Also const Done.
https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/database_manager.cc (left): https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:653: DCHECK(!safe_browsing_thread_.get()); On 2015/02/23 19:03:27, Alexei Svitkine wrote: > On 2015/02/23 16:33:55, gab wrote: > > This DCHECK ensured StartOnIOThread() was only ever called once, perhaps add > > DCHECK(!safe_browsing_task_runner_) at the top of this method to keep this > > check? > > The "if (enabled_) return;" code suggests that this actually can be called > multiple times. I'd rather not change that in this CL, so added the check below > that to match the previous check location. Actually the start/stop methods can be called multiple times (ex. if safebrowsing is disabled and re-enabled). So if you want to dcheck that, you should also clear safe_browsing_task_runner_ in DoStopOnIOThread.
lgtm w/ one last comment tweak and Matt's last comment https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/database_manager.cc (left): https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:653: DCHECK(!safe_browsing_thread_.get()); On 2015/02/23 19:03:27, Alexei Svitkine wrote: > On 2015/02/23 16:33:55, gab wrote: > > This DCHECK ensured StartOnIOThread() was only ever called once, perhaps add > > DCHECK(!safe_browsing_task_runner_) at the top of this method to keep this > > check? > > The "if (enabled_) return;" code suggests that this actually can be called > multiple times. I'd rather not change that in this CL, so added the check below > that to match the previous check location. Right that's what I meant. https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:653: DCHECK(!safe_browsing_thread_.get()); On 2015/02/23 19:27:36, mattm wrote: > On 2015/02/23 19:03:27, Alexei Svitkine wrote: > > On 2015/02/23 16:33:55, gab wrote: > > > This DCHECK ensured StartOnIOThread() was only ever called once, perhaps add > > > DCHECK(!safe_browsing_task_runner_) at the top of this method to keep this > > > check? > > > > The "if (enabled_) return;" code suggests that this actually can be called > > multiple times. I'd rather not change that in this CL, so added the check > below > > that to match the previous check location. > > Actually the start/stop methods can be called multiple times (ex. if > safebrowsing is disabled and re-enabled). So if you want to dcheck that, you > should also clear safe_browsing_task_runner_ in DoStopOnIOThread. Ah, good point, makes sense to me. https://codereview.chromium.org/910953002/diff/210001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/910953002/diff/210001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:1767: // anyways. This comment (and use-case) below is still correct, but should be updated to refer to the task runner rather than "thread".
https://codereview.chromium.org/910953002/diff/210001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.h (right): https://codereview.chromium.org/910953002/diff/210001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.h:37: scoped_refptr<base::SequencedTaskRunner> db_task_runner, const scoped_refptr<base::SequencedTaskRunner>& db_task_runner, here and everywhere else so that the refcount isn't bumped and dropped needlessly for the function calls.
Thanks, PTAL. https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/database_manager.cc (left): https://codereview.chromium.org/910953002/diff/190001/chrome/browser/safe_bro... chrome/browser/safe_browsing/database_manager.cc:653: DCHECK(!safe_browsing_thread_.get()); On 2015/02/23 19:27:36, mattm wrote: > On 2015/02/23 19:03:27, Alexei Svitkine wrote: > > On 2015/02/23 16:33:55, gab wrote: > > > This DCHECK ensured StartOnIOThread() was only ever called once, perhaps add > > > DCHECK(!safe_browsing_task_runner_) at the top of this method to keep this > > > check? > > > > The "if (enabled_) return;" code suggests that this actually can be called > > multiple times. I'd rather not change that in this CL, so added the check > below > > that to match the previous check location. > > Actually the start/stop methods can be called multiple times (ex. if > safebrowsing is disabled and re-enabled). So if you want to dcheck that, you > should also clear safe_browsing_task_runner_ in DoStopOnIOThread. Done. https://codereview.chromium.org/910953002/diff/210001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/910953002/diff/210001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:1767: // anyways. On 2015/02/23 19:53:33, gab wrote: > This comment (and use-case) below is still correct, but should be updated to > refer to the task runner rather than "thread". Done. Also updated the method name that said OnMainThread() to OnMainTaskRunner() and corresponding enum. https://codereview.chromium.org/910953002/diff/210001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.h (right): https://codereview.chromium.org/910953002/diff/210001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.h:37: scoped_refptr<base::SequencedTaskRunner> db_task_runner, On 2015/02/23 20:37:53, grt wrote: > const scoped_refptr<base::SequencedTaskRunner>& db_task_runner, > here and everywhere else so that the refcount isn't bumped and dropped > needlessly for the function calls. I only added it to the function params, not the members. I believe it's not safe to add it to the members, since SB can be started and stopped - and we clear the refptr in database_manager.cc when stopped. So this would make the const refs invalid. (While tasks may still be running.) Note: See previous discussion around SKIP_ON_SHUTDOWN upthread - for details - basically in the new code, we don't want to synchronously execute all the remaining tasks on IO thread - which the old code does when stopping the thread. (We can monitor the experiment to ensure there are no problems with this semantic change - and fix any if they arise.)
lgtm https://codereview.chromium.org/910953002/diff/210001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.h (right): https://codereview.chromium.org/910953002/diff/210001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.h:37: scoped_refptr<base::SequencedTaskRunner> db_task_runner, On 2015/02/24 18:08:58, Alexei Svitkine wrote: > On 2015/02/23 20:37:53, grt wrote: > > const scoped_refptr<base::SequencedTaskRunner>& db_task_runner, > > here and everywhere else so that the refcount isn't bumped and dropped > > needlessly for the function calls. > > I only added it to the function params, not the members. This is correct, thanks. Passing them around as arguments should always be done by const ref, as the caller's reference to the object is sufficient to keep it alive for the duration of the function call. It is the callee's responsibility to hold its own scoped_refptr that lives beyond the function call if needed.
The CQ bit was checked by asvitkine@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, mattm@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/910953002/#ps270001 (title: "Fix browsertest.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/910953002/270001
Message was sent while issue was closed.
Committed patchset #9 (id:270001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/be033cca03e50e1bba363d7dc2e56ad38f9196da Cr-Commit-Position: refs/heads/master@{#317857} |
