Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1684643002:
Avoid BrowserThread in profile_sync_service_autofill_unittest.cc (Closed)
|
Created:
4 years, 10 months ago by vabr (Chromium) Modified:
4 years, 10 months ago CC:
chromium-reviews, maxbogue+watch_chromium.org, plaree+watch_chromium.org, tim+watch_chromium.org, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid BrowserThread in profile_sync_service_autofill_unittest.cc
This CL gets rid of the content-provided threads in
profile_sync_service_autofill_unittest, in favour of base::Thread. This is in
preparation to componentising this unit test.
BUG=581640
Committed: https://crrev.com/6b1e9715eed76b95f079c82663d7d5c8dee9ef74
Cr-Commit-Position: refs/heads/master@{#376457}
Patch Set 1 : #
Total comments: 8
Patch Set 2 : Rebased, but pending self-review #Patch Set 3 : Self reviewed #Patch Set 4 : Does stopping the threads explicitly help with ASAN failures? #
Total comments: 4
Patch Set 5 : Just rebased #Patch Set 6 : Clean up clean-up: what does ASAN say? #Patch Set 7 : Just rebased #Patch Set 8 : Fixing LSAN for Typed URLs? #Patch Set 9 : Fix some leaks #Patch Set 10 : Pulled the extra thread into the abstract test base + deleting properly on sync thread #Patch Set 11 : Destroy SyncService in the base class #
Total comments: 23
Patch Set 12 : Rebase and address comments #Patch Set 13 : Just rebased #Patch Set 14 : Const ref scoped_refptr + fix scoped_ptr usage #
Total comments: 2
Patch Set 15 : Just rebased #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 39 (13 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== [WIP] Avoid BrowserThread in profile_sync_service_autofill_unittest.cc This is just a backup, WIP. COMMIT=false TODO: Also switch TestPSS to get the threads injected. This CL gets rid of the content-provided threads in profile_sync_service_autofill_unittest, in favour of base::Thread. This is in preparation to componentising this unit test. BUG=581640 ========== to ========== Avoid BrowserThread in profile_sync_service_autofill_unittest.cc This CL gets rid of the content-provided threads in profile_sync_service_autofill_unittest, in favour of base::Thread. This is in preparation to componentising this unit test. BUG=581640 ==========
vabr@chromium.org changed reviewers: + sdefresne@chromium.org
Hi sdefresne@, Because you participated in the discussion leading to this CL, would you mind having a look? After addressing your comments, I will then add the sync OWNERS. Thanks! Vaclav
https://codereview.chromium.org/1684643002/diff/40001/chrome/browser/sync/pro... File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right): https://codereview.chromium.org/1684643002/diff/40001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:128: base::SingleThreadTaskRunner* g_ui_thread = nullptr; nit: I personally would have tried to first decouple from Profile and KeyedService factories before decoupling from content::BrowserThread (in order to avoid having to resort to such hacks). Since this is temporary and will go away, I'm okay with this hack. https://codereview.chromium.org/1684643002/diff/40001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:265: TokenWebDataServiceFake() : TokenWebData(g_ui_thread, g_db_thread) {} nit: DCHECK(g_ui_thread); DCHECK(g_db_thread); https://codereview.chromium.org/1684643002/diff/40001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:295: db_thread_(g_db_thread), Can't you access ui_thread_/db_thread_ from super class? https://codereview.chromium.org/1684643002/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test_profile_sync_service.cc (right): https://codereview.chromium.org/1684643002/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test_profile_sync_service.cc:157: init_params.db_thread = content::BrowserThread::GetMessageLoopProxyForThread( Will you componentize TestProfileSyncService? If so, can you remove those usage of content::BrowserThread too?
lgtm to unblock you
On 2016/02/12 14:31:09, sdefresne wrote: > lgtm to unblock you Hi Sylvain, Sorry for my lack of update here. I was not blocked by you, I have been working on removing //chrome DEPS first, as you suggested (and I found necessary for more reasons). I am currently done with that for profile_sync_service_autofill_unittest.cc, and mostly done for profile_sync_service_typed_url_unittest.cc (there I still struggle with creating and killing things on the right threads). The WIP is in https://codereview.chromium.org/1646553002/, but please do not review yet, that CL will be changing substantially (I update it every day to have a back-up). Once that CL is finished, I'll rebase this one on that, and will ping you for review again. I hope at that point this CL will be simpler (the globals will be gone for sure). Cheers, Vaclav
On 2016/02/12 14:38:27, vabr (Chromium) wrote: > On 2016/02/12 14:31:09, sdefresne wrote: > > lgtm to unblock you > > Hi Sylvain, > > Sorry for my lack of update here. I was not blocked by you, I have been working > on removing //chrome DEPS first, as you suggested (and I found necessary for > more reasons). > > I am currently done with that for profile_sync_service_autofill_unittest.cc, and > mostly done for profile_sync_service_typed_url_unittest.cc (there I still > struggle with creating and killing things on the right threads). Hah, and now the lifetime issues seem to have been solved. Hopefully stuff will move quicker now. :)
vabr@chromium.org changed reviewers: + zea@chromium.org
@sdefresne: Thanks for the review so far. I rebased on another CL, feel free to have a second look. @zea: Please review the whole CL. (Sorry for the avalanche, this is the last one for these tests.) Thank you, Vaclav P.S. The ASAN bot has been reporting some use-after free and memory leaks on code outside the UI thread. I cannot reproduce locally, and the bot seems stuck at the moment. So I'm sending this off to you with the hope that if my last fix won't work, then the necessary further changes won't be big enough to waste your review efforts so far. https://codereview.chromium.org/1684643002/diff/40001/chrome/browser/sync/pro... File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right): https://codereview.chromium.org/1684643002/diff/40001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:128: base::SingleThreadTaskRunner* g_ui_thread = nullptr; On 2016/02/10 10:56:57, sdefresne wrote: > nit: I personally would have tried to first decouple from Profile and > KeyedService factories before decoupling from content::BrowserThread (in order > to avoid having to resort to such hacks). Since this is temporary and will go > away, I'm okay with this hack. Done, in https://codereview.chromium.org/1646553002/. https://codereview.chromium.org/1684643002/diff/40001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:265: TokenWebDataServiceFake() : TokenWebData(g_ui_thread, g_db_thread) {} On 2016/02/10 10:56:58, sdefresne wrote: > nit: DCHECK(g_ui_thread); DCHECK(g_db_thread); Done. https://codereview.chromium.org/1684643002/diff/40001/chrome/browser/sync/pro... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:295: db_thread_(g_db_thread), On 2016/02/10 10:56:57, sdefresne wrote: > Can't you access ui_thread_/db_thread_ from super class? I'm afraid I cannot, it looks like they are private, and not even the task posting is exposed. https://codereview.chromium.org/1684643002/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test_profile_sync_service.cc (right): https://codereview.chromium.org/1684643002/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test_profile_sync_service.cc:157: init_params.db_thread = content::BrowserThread::GetMessageLoopProxyForThread( On 2016/02/10 10:56:58, sdefresne wrote: > Will you componentize TestProfileSyncService? If so, can you remove those usage > of content::BrowserThread too? Indeed! Done.
A couple questions, otherwise LG https://codereview.chromium.org/1684643002/diff/100001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right): https://codereview.chromium.org/1684643002/diff/100001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:410: db_thread_.Stop(); Doesn't this always happen on destruction? https://codereview.chromium.org/1684643002/diff/100001/components/browser_syn... File components/browser_sync/browser/profile_sync_test_util.cc (right): https://codereview.chromium.org/1684643002/diff/100001/components/browser_syn... components/browser_sync/browser/profile_sync_test_util.cc:217: activate_model_creation_ ? base::ThreadTaskRunnerHandle::Get() : nullptr, Why use the current thread as the file thread? Why not support injecting the file thread too? Or is it just that no tests care about a real file thread?
Thanks, Nicolas! Questions answered below. Cheers, Vaclav https://codereview.chromium.org/1684643002/diff/100001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right): https://codereview.chromium.org/1684643002/diff/100001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:410: db_thread_.Stop(); On 2016/02/13 01:28:55, Nicolas Zea wrote: > Doesn't this always happen on destruction? It does, the only question is with timing. My suspicion with the ASAN failures is that some of the thread-using objects were being destroyed before the thread, and then once the thread ran the remaining tasks, it missed the other destroyed objects. Once the ASAN bots work again (I could not reproduce the failures with ASAN locally), I'll verify that. On the above hypothesis, I can either keep the Stop() call explicit, or move the thread low enough in the list of data members that it gets destroyed before the other objects get destroyed. The former is less prone to somebody introducing a bug later by reordering the data member list, the second is less verbose. Let me know if you have any preferences. https://codereview.chromium.org/1684643002/diff/100001/components/browser_syn... File components/browser_sync/browser/profile_sync_test_util.cc (right): https://codereview.chromium.org/1684643002/diff/100001/components/browser_syn... components/browser_sync/browser/profile_sync_test_util.cc:217: activate_model_creation_ ? base::ThreadTaskRunnerHandle::Get() : nullptr, On 2016/02/13 01:28:55, Nicolas Zea wrote: > Why use the current thread as the file thread? Why not support injecting the > file thread too? Or is it just that no tests care about a real file thread? The latter -- no tests seemed to care so far, so I kept it simple.
> It does, the only question is with timing. My suspicion with the ASAN failures > is that some of the thread-using objects were being destroyed before the thread, > and then once the thread ran the remaining tasks, it missed the other destroyed > objects. Once the ASAN bots work again (I could not reproduce the failures with > ASAN locally), I'll verify that. > > On the above hypothesis, I can either keep the Stop() call explicit, or move the > thread low enough in the list of data members that it gets destroyed before the > other objects get destroyed. The former is less prone to somebody introducing a > bug later by reordering the data member list, the second is less verbose. Let > me know if you have any preferences. Looking at the trybot results, the above hypothesis was wrong. The leaks and use-after-free are still there even with the explicit earlier Thread::Stop(). I still cannot reproduce locally, will investigate this more and then update the CL.
Patch set 6 fixed the use-after-free. I am still working on the leaks, but at least figured out why I could not reproduce the failures locally: apparently LSAN needs to be enabled separately when building ASAN. Doing that now.
lg, I'll delegate to zea for the full review
On 2016/02/15 10:00:02, sdefresne wrote: > lg, I'll delegate to zea for the full review Thanks! Status update: Could reproduce the LSAN errors locally. In case of the autofill test, the AutofillWebDataBackendImpl is not getting deleted after this patch, so I'm trying to figure out which threads/run loops or shutdown methods are not running any more. Will update this CL once that is fixed, hopefully it will happen before the US holidays are over. :) Cheers, Vaclav
More update: Half of the LSAN failures are fixed, and I am close to fixing the rest (ProfileSyncServiceBundle needs to spun the MessageLoop it owns after deleting some other parts it contains), but will finish tomorrow. Once done, I'll ping zea@ on this CL for a review. Cheers, Vaclav
Hi Nicolas, This CL is now ready for review. Thanks! Vaclav https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right): https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:413: sync_service_->Shutdown(); @zea -- Do you think it is a problem that in this case, Shutdown is called twice on the sync_service_? If it is a problem, I can expose a method of the base test class to shut it down, which will check that it only does that once.
https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/ab... File chrome/browser/sync/abstract_profile_sync_service_test.h (right): https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/ab... chrome/browser/sync/abstract_profile_sync_service_test.h:61: base::Thread extra_thread_; nit:I think "data_type_thread" is clearer Although, given that nothing within the PSS really uses this thread, should it be part of the abstract PSS? Why not just have it part of the test itself, living next to the PSS? https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right): https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:148: scoped_refptr<base::SequencedTaskRunner> ui_thread) nit: pass by const ref, i.e. const scoped_refptr<...>& ui_thread, here and elsewhere you pass scoped_refptrs https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:413: sync_service_->Shutdown(); On 2016/02/16 16:41:57, vabr (Chromium) wrote: > @zea -- Do you think it is a problem that in this case, Shutdown is called twice > on the sync_service_? If it is a problem, I can expose a method of the base test > class to shut it down, which will check that it only does that once. I don't see any issue with this. https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right): https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:204: void ShutdownSyncableService(base::WaitableEvent* event) { nit: Maybe rename this DeleteSyncableService https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:233: history_service_.reset(); Are these two calls necessary? If so might be good to comment why. https://codereview.chromium.org/1684643002/diff/240001/components/browser_syn... File components/browser_sync/browser/profile_sync_test_util.h (right): https://codereview.chromium.org/1684643002/diff/240001/components/browser_syn... components/browser_sync/browser/profile_sync_test_util.h:165: base::MessageLoop message_loop_; Is this used for anything? If it's just here so you can call ThreadTaskRunnerHandle::Get(), I wonder if it should be a member of the test class itself, rather than this helper class.
https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right): https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:220: base::WaitableEvent sync_loop_tasks_done(false, false); nit: seems like this could be done with a RunLoop a bit more simply (by posting the QuitClosure)
Thanks, Nicolas. I addressed your comments. I also pushed back on passing scoped_refptr by reference, using the QuitClosure, and excluding the MessageLoop from the bundle object. Please see below. Thanks! Vaclav https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/ab... File chrome/browser/sync/abstract_profile_sync_service_test.h (right): https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/ab... chrome/browser/sync/abstract_profile_sync_service_test.h:61: base::Thread extra_thread_; On 2016/02/16 22:44:50, Nicolas Zea wrote: > nit:I think "data_type_thread" is clearer Done. > > Although, given that nothing within the PSS really uses this thread, should it > be part of the abstract PSS? Why not just have it part of the test itself, > living next to the PSS? I'm not sure I understand. This is inside the the test itself (AbstractProfileSyncServiceTest), there is no abstract PSS. There is the TestPSS, but that one is defined elsewhere and does not include the thread. https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right): https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:148: scoped_refptr<base::SequencedTaskRunner> ui_thread) On 2016/02/16 22:44:50, Nicolas Zea wrote: > nit: pass by const ref, i.e. const scoped_refptr<...>& ui_thread, here and > elsewhere you pass scoped_refptrs But why? The point of a scoped_[ref]ptr is to ensure that the pointed object is not deleted. Having a reference to a scoped_refptr instead of the scoped_refptr itself is not giving this guarantee. Is performance your concern? I know refcounting is a potential hotspot in Blink, but here in unit tests? And the size of scoped_refptr should be comparable to the size of a reference to it, so having a longer call stack should also not be an issue. https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right): https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:204: void ShutdownSyncableService(base::WaitableEvent* event) { On 2016/02/16 22:44:50, Nicolas Zea wrote: > nit: Maybe rename this DeleteSyncableService Done and moved this to https://codereview.chromium.org/1646553002/. https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:220: base::WaitableEvent sync_loop_tasks_done(false, false); On 2016/02/16 23:40:16, Nicolas Zea wrote: > nit: seems like this could be done with a RunLoop a bit more simply (by posting > the QuitClosure) I don't think so. When I tried that, MessageLoop::QuitNow() complained about the loop to be quit not being the current one. I'm still a bit unsure about how threads and message loops interact, but I suppose that a different thread (Sync thread vs. the main test thread) cannot have the same message loop. Or is there a way to specify which message loop to run? https://codereview.chromium.org/1646553002/#ps260001 is what I did in attempt to implement your suggestion. Perhaps there is a better way to do so? https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:233: history_service_.reset(); On 2016/02/16 22:44:50, Nicolas Zea wrote: > Are these two calls necessary? If so might be good to comment why. The former is not, the latter got an explanation and was moved up just below the source of the need for the respin. https://codereview.chromium.org/1684643002/diff/240001/components/browser_syn... File components/browser_sync/browser/profile_sync_test_util.h (right): https://codereview.chromium.org/1684643002/diff/240001/components/browser_syn... components/browser_sync/browser/profile_sync_test_util.h:165: base::MessageLoop message_loop_; On 2016/02/16 22:44:51, Nicolas Zea wrote: > Is this used for anything? Yes, it is needed for getting the current task runner, e.g., when creating the URLRequestContextGetter. > If it's just here so you can call > ThreadTaskRunnerHandle::Get(), I wonder if it should be a member of the test > class itself, rather than this helper class. The bundle is meant to be a single data member to aggregate to get all support for PSS. I wanted to avoid comments like: "If you use the bundle, make sure to also have the MessageLoop before it." So I went with including it in the bundle. Do you see any advantages in excluding it?
https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/ab... File chrome/browser/sync/abstract_profile_sync_service_test.h (right): https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/ab... chrome/browser/sync/abstract_profile_sync_service_test.h:61: base::Thread extra_thread_; On 2016/02/17 13:55:45, vabr (Chromium) wrote: > On 2016/02/16 22:44:50, Nicolas Zea wrote: > > nit:I think "data_type_thread" is clearer > Done. > > > > > Although, given that nothing within the PSS really uses this thread, should it > > be part of the abstract PSS? Why not just have it part of the test itself, > > living next to the PSS? > > I'm not sure I understand. This is inside the the test itself > (AbstractProfileSyncServiceTest), there is no abstract PSS. There is the > TestPSS, but that one is defined elsewhere and does not include the thread. Sorry, I meant part of the test fixture itself (i.e. the class that inherits from testing::Test). This is similar to my other comment about having the test fixture own the message loop. It seems like this exists here just to remove code from the test, even though this actual class doesn't use the thread. In general I find it cleaner to have members live within/next to whatever component uses them. Is that clearer? https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right): https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:148: scoped_refptr<base::SequencedTaskRunner> ui_thread) On 2016/02/17 13:55:45, vabr (Chromium) wrote: > On 2016/02/16 22:44:50, Nicolas Zea wrote: > > nit: pass by const ref, i.e. const scoped_refptr<...>& ui_thread, here and > > elsewhere you pass scoped_refptrs > > But why? The point of a scoped_[ref]ptr is to ensure that the pointed object is > not deleted. Having a reference to a scoped_refptr instead of the scoped_refptr > itself is not giving this guarantee. > > Is performance your concern? I know refcounting is a potential hotspot in Blink, > but here in unit tests? And the size of scoped_refptr should be comparable to > the size of a reference to it, so having a longer call stack should also not be > an issue. This is primarily for consistency. In general it's good to pass scoped_refptr by const ref to avoid ref churn (you add and then remove a ref when you pass by value), and this is something I try to keep consistent within all the sync code. I can't seem to find a strict style guide about this, but https://www.chromium.org/developers/smart-pointer-guidelines does mention it as an option, and it's good to be consistent with the rest of the sync code.. Also note that I'm not saying the |ui_thread_| member should be a const scoped_refptr<..>&, just the param here. You're correct that the member should add a new reference to ensure lifetime is correct. https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right): https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:220: base::WaitableEvent sync_loop_tasks_done(false, false); On 2016/02/17 13:55:45, vabr (Chromium) wrote: > On 2016/02/16 23:40:16, Nicolas Zea wrote: > > nit: seems like this could be done with a RunLoop a bit more simply (by > posting > > the QuitClosure) > > I don't think so. When I tried that, MessageLoop::QuitNow() complained about the > loop to be quit not being the current one. > > I'm still a bit unsure about how threads and message loops interact, but I > suppose that a different thread (Sync thread vs. the main test thread) cannot > have the same message loop. > > Or is there a way to specify which message loop to run? > https://codereview.chromium.org/1646553002/#ps260001 is what I did in attempt to > implement your suggestion. Perhaps there is a better way to do so? Ah, in that case you need to do PostTaskAndReply. See for example FakeSyncManager::WaitForSyncThread() in fake_sync_manager.cc https://codereview.chromium.org/1684643002/diff/240001/components/browser_syn... File components/browser_sync/browser/profile_sync_test_util.h (right): https://codereview.chromium.org/1684643002/diff/240001/components/browser_syn... components/browser_sync/browser/profile_sync_test_util.h:165: base::MessageLoop message_loop_; On 2016/02/17 13:55:45, vabr (Chromium) wrote: > On 2016/02/16 22:44:51, Nicolas Zea wrote: > > Is this used for anything? > Yes, it is needed for getting the current task runner, e.g., when creating the > URLRequestContextGetter. > > > If it's just here so you can call > > ThreadTaskRunnerHandle::Get(), I wonder if it should be a member of the test > > class itself, rather than this helper class. > > The bundle is meant to be a single data member to aggregate to get all support > for PSS. I wanted to avoid comments like: "If you use the bundle, make sure to > also have the MessageLoop before it." So I went with including it in the bundle. > Do you see any advantages in excluding it? My concern here is that tests often instantiate default message loops themselves. Since you're basically just ensuring that a default message loop exists, and that might be something that multiple components need, it seems better to have the test fixture do it. This is especially true if the test itself needs to for example pump the message loop to ensure all pending tasks a finished. Needing a message loop is pretty common in tests, and not really something sync specific.l
Thanks, Nicolas! I addressed your comments, and also fixed one thing in the existing code (see my last comment, about scoped_ptr). PTAL. Cheers, vaclav https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/ab... File chrome/browser/sync/abstract_profile_sync_service_test.h (right): https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/ab... chrome/browser/sync/abstract_profile_sync_service_test.h:61: base::Thread extra_thread_; On 2016/02/17 22:05:43, Nicolas Zea wrote: > On 2016/02/17 13:55:45, vabr (Chromium) wrote: > > On 2016/02/16 22:44:50, Nicolas Zea wrote: > > > nit:I think "data_type_thread" is clearer > > Done. > > > > > > > > Although, given that nothing within the PSS really uses this thread, should > it > > > be part of the abstract PSS? Why not just have it part of the test itself, > > > living next to the PSS? > > > > I'm not sure I understand. This is inside the the test itself > > (AbstractProfileSyncServiceTest), there is no abstract PSS. There is the > > TestPSS, but that one is defined elsewhere and does not include the thread. > > Sorry, I meant part of the test fixture itself (i.e. the class that inherits > from testing::Test). > > This is similar to my other comment about having the test fixture own the > message loop. It seems like this exists here just to remove code from the test, > even though this actual class doesn't use the thread. In general I find it > cleaner to have members live within/next to whatever component uses them. > > Is that clearer? Sorry, Nicolas, I still am not sure I follow. In my current CL, the base::Thread is part of the test fixture AbstractProfileSyncServiceTest (which inherits from testing::Test). I now also added the MessageLoop to that, following you other comment. Could you please confirm which class you want me to put the thread into? https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right): https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:148: scoped_refptr<base::SequencedTaskRunner> ui_thread) On 2016/02/17 22:05:43, Nicolas Zea wrote: > On 2016/02/17 13:55:45, vabr (Chromium) wrote: > > On 2016/02/16 22:44:50, Nicolas Zea wrote: > > > nit: pass by const ref, i.e. const scoped_refptr<...>& ui_thread, here and > > > elsewhere you pass scoped_refptrs > > > > But why? The point of a scoped_[ref]ptr is to ensure that the pointed object > is > > not deleted. Having a reference to a scoped_refptr instead of the > scoped_refptr > > itself is not giving this guarantee. > > > > Is performance your concern? I know refcounting is a potential hotspot in > Blink, > > but here in unit tests? And the size of scoped_refptr should be comparable to > > the size of a reference to it, so having a longer call stack should also not > be > > an issue. > > This is primarily for consistency. In general it's good to pass scoped_refptr by > const ref to avoid ref churn (you add and then remove a ref when you pass by > value), and this is something I try to keep consistent within all the sync code. Thanks for the explanation! I understand your point. You recently sped up sync significantly, so I have no issues taking your word for the refcounting churn being significant enough to remove for performance reasons. And lastly, you are the owner here, so I am now changing the added arguments to use const ref for scopers (and callbacks also). I still think this a topic of interest beyond this CL, so I started a thread on chromium-dev and Cc-ed you on it. My main question there is why to pass references to scoped_refptr instead of just a naked pointer? > > I can't seem to find a strict style guide about this, but > https://www.chromium.org/developers/smart-pointer-guidelines does mention it as > an option, and it's good to be consistent with the rest of the sync code.. > > Also note that I'm not saying the |ui_thread_| member should be a const > scoped_refptr<..>&, just the param here. You're correct that the member should > add a new reference to ensure lifetime is correct. Note that even when the passed argument is wrapped in a scoped_refptr inside the method, there is generally still a chance that the passed scoped_refptr is destroyed on another thread when the function on the current thread is being called. This is not really a concern here, it can only happen in code where objects hop across threads, which itself seems like design issues. https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right): https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:220: base::WaitableEvent sync_loop_tasks_done(false, false); On 2016/02/17 22:05:43, Nicolas Zea wrote: > On 2016/02/17 13:55:45, vabr (Chromium) wrote: > > On 2016/02/16 23:40:16, Nicolas Zea wrote: > > > nit: seems like this could be done with a RunLoop a bit more simply (by > > posting > > > the QuitClosure) > > > > I don't think so. When I tried that, MessageLoop::QuitNow() complained about > the > > loop to be quit not being the current one. > > > > I'm still a bit unsure about how threads and message loops interact, but I > > suppose that a different thread (Sync thread vs. the main test thread) cannot > > have the same message loop. > > > > Or is there a way to specify which message loop to run? > > https://codereview.chromium.org/1646553002/#ps260001 is what I did in attempt > to > > implement your suggestion. Perhaps there is a better way to do so? > > Ah, in that case you need to do PostTaskAndReply. See for example > FakeSyncManager::WaitForSyncThread() in fake_sync_manager.cc Good tip, thanks! Done (in the parent CL https://codereview.chromium.org/1646553002/). https://codereview.chromium.org/1684643002/diff/240001/components/browser_syn... File components/browser_sync/browser/profile_sync_test_util.h (right): https://codereview.chromium.org/1684643002/diff/240001/components/browser_syn... components/browser_sync/browser/profile_sync_test_util.h:165: base::MessageLoop message_loop_; On 2016/02/17 22:05:43, Nicolas Zea wrote: > On 2016/02/17 13:55:45, vabr (Chromium) wrote: > > On 2016/02/16 22:44:51, Nicolas Zea wrote: > > > Is this used for anything? > > Yes, it is needed for getting the current task runner, e.g., when creating the > > URLRequestContextGetter. > > > > > If it's just here so you can call > > > ThreadTaskRunnerHandle::Get(), I wonder if it should be a member of the test > > > class itself, rather than this helper class. > > > > The bundle is meant to be a single data member to aggregate to get all support > > for PSS. I wanted to avoid comments like: "If you use the bundle, make sure to > > also have the MessageLoop before it." So I went with including it in the > bundle. > > Do you see any advantages in excluding it? > > My concern here is that tests often instantiate default message loops > themselves. Since you're basically just ensuring that a default message loop > exists, and that might be something that multiple components need, it seems > better to have the test fixture do it. > > This is especially true if the test itself needs to for example pump the message > loop to ensure all pending tasks a finished. Needing a message loop is pretty > common in tests, and not really something sync specific.l SGTM, I pulled the MessageLoop out of the bundle. https://codereview.chromium.org/1684643002/diff/300001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right): https://codereview.chromium.org/1684643002/diff/300001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:677: WaitableEvent* wait_for_syncapi) Here and below I replaced passing a scoped_ptr by pointer with passing the raw pointer. Passing a pointer to a scoped_ptr goes against the purpose of the scoped_ptr (as mentioned in https://www.chromium.org/developers/smart-pointer-guidelines). Either the ownership is transferred, in which case the scoped_ptr is passed/moved by value, or it stays with the caller, in which the raw pointer is enough.
LGTM! Thanks for your patience! https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/ab... File chrome/browser/sync/abstract_profile_sync_service_test.h (right): https://codereview.chromium.org/1684643002/diff/240001/chrome/browser/sync/ab... chrome/browser/sync/abstract_profile_sync_service_test.h:61: base::Thread extra_thread_; On 2016/02/18 13:05:00, vabr (Chromium) wrote: > On 2016/02/17 22:05:43, Nicolas Zea wrote: > > On 2016/02/17 13:55:45, vabr (Chromium) wrote: > > > On 2016/02/16 22:44:50, Nicolas Zea wrote: > > > > nit:I think "data_type_thread" is clearer > > > Done. > > > > > > > > > > > Although, given that nothing within the PSS really uses this thread, > should > > it > > > > be part of the abstract PSS? Why not just have it part of the test itself, > > > > living next to the PSS? > > > > > > I'm not sure I understand. This is inside the the test itself > > > (AbstractProfileSyncServiceTest), there is no abstract PSS. There is the > > > TestPSS, but that one is defined elsewhere and does not include the thread. > > > > Sorry, I meant part of the test fixture itself (i.e. the class that inherits > > from testing::Test). > > > > This is similar to my other comment about having the test fixture own the > > message loop. It seems like this exists here just to remove code from the > test, > > even though this actual class doesn't use the thread. In general I find it > > cleaner to have members live within/next to whatever component uses them. > > > > Is that clearer? > > Sorry, Nicolas, I still am not sure I follow. > > In my current CL, the base::Thread is part of the test fixture > AbstractProfileSyncServiceTest (which inherits from testing::Test). I now also > added the MessageLoop to that, following you other comment. > > Could you please confirm which class you want me to put the thread into? Ah, I missed that this inherits from testing::Test itself. Sorry about the confusion! https://codereview.chromium.org/1684643002/diff/300001/chrome/browser/sync/pr... File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right): https://codereview.chromium.org/1684643002/diff/300001/chrome/browser/sync/pr... chrome/browser/sync/profile_sync_service_autofill_unittest.cc:677: WaitableEvent* wait_for_syncapi) On 2016/02/18 13:05:00, vabr (Chromium) wrote: > Here and below I replaced passing a scoped_ptr by pointer with passing the raw > pointer. Passing a pointer to a scoped_ptr goes against the purpose of the > scoped_ptr (as mentioned in > https://www.chromium.org/developers/smart-pointer-guidelines). Either the > ownership is transferred, in which case the scoped_ptr is passed/moved by value, > or it stays with the caller, in which the raw pointer is enough. Acknowledged.
The CQ bit was checked by vabr@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/1684643002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684643002/320001
The CQ bit was unchecked by vabr@chromium.org
The CQ bit was checked by vabr@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/1684643002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684643002/320001
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 vabr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, zea@chromium.org Link to the patchset: https://codereview.chromium.org/1684643002/#ps320001 (title: "Just rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1684643002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684643002/320001
Message was sent while issue was closed.
Description was changed from ========== Avoid BrowserThread in profile_sync_service_autofill_unittest.cc This CL gets rid of the content-provided threads in profile_sync_service_autofill_unittest, in favour of base::Thread. This is in preparation to componentising this unit test. BUG=581640 ========== to ========== Avoid BrowserThread in profile_sync_service_autofill_unittest.cc This CL gets rid of the content-provided threads in profile_sync_service_autofill_unittest, in favour of base::Thread. This is in preparation to componentising this unit test. BUG=581640 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Avoid BrowserThread in profile_sync_service_autofill_unittest.cc This CL gets rid of the content-provided threads in profile_sync_service_autofill_unittest, in favour of base::Thread. This is in preparation to componentising this unit test. BUG=581640 ========== to ========== Avoid BrowserThread in profile_sync_service_autofill_unittest.cc This CL gets rid of the content-provided threads in profile_sync_service_autofill_unittest, in favour of base::Thread. This is in preparation to componentising this unit test. BUG=581640 Committed: https://crrev.com/6b1e9715eed76b95f079c82663d7d5c8dee9ef74 Cr-Commit-Position: refs/heads/master@{#376457} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/6b1e9715eed76b95f079c82663d7d5c8dee9ef74 Cr-Commit-Position: refs/heads/master@{#376457} |
