Flush SequenceWorkerPool tasks after each unit test.

Flush SequenceWorkerPool tasks after each test. Applies to unit_test, interactive_ui test, browser_tests... pretty much all gtest based content library test harnesses are affected.

The CL changes semantics and implementation of the existing FlushForTesting method (which wasn't suitable for this usage).

* The old method would wait for delayed tasks prior to continuing, the new method
will not run them but will delete them prior to continuing.

* The old method would deadlock if called after Shutdown had been called, the new method returns immediately in that case.

A few SWP unittests relied on the waiting for delayed task completion behavior. Those have been modified to explicitly wait thru other means.

BUG=168415, 166470
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186578

[michaeln, 2012-12-21 02:50:47]

https://codereview.chromium.org/11649032/diff/7003/base/threading/sequenced_w...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/11649032/diff/7003/base/threading/sequenced_w...
base/threading/sequenced_worker_pool.cc:640: is_idle_cv_.Signal();
FlushForTesting can wait forever if Shutdown happens after its wait has begun.
So we signal upon return to wake them.

Some existing tests are disabled due to this problem, and when adding my
additional calls to FlushForTesting more test timeouts show up.

[michaeln, 2012-12-21 03:02:04]

ptal

[michaeln, 2012-12-21 03:17:54]

https://codereview.chromium.org/11649032/diff/7003/base/threading/sequenced_w...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/11649032/diff/7003/base/threading/sequenced_w...
base/threading/sequenced_worker_pool.cc:392: // FlushForTesting()) until
IsIdle() goes to true or Shutdown completes.
I might be able to better normalize things by introducing a shutdown_complete_
bool + cv pair but I think the smaller changes here are enough to break the
deadlock... i'll see what the trybots think.

[michaeln, 2012-12-21 04:23:01]

trybots seem to like it!

[michaeln, 2012-12-21 22:15:54]

removed some suppressions and running thru the bots again

[not at google - send to devlin, 2012-12-21 22:17:13]

Btw leaving this one for akalin now since it's not really extension related
anymore, and this is obviously his domain not mine.

[michaeln, 2012-12-21 22:20:18]

also cc'ing thestig who's been adding suppressions that look quite related to
this

[michaeln, 2012-12-21 23:43:19]

https://codereview.chromium.org/11649032/diff/23001/base/threading/sequenced_...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/11649032/diff/23001/base/threading/sequenced_...
base/threading/sequenced_worker_pool.cc:631: shutdown_called_ = true;
Hi Fred, 

Not sure if you've been following along so I'll catch you up. In current sources
FlushForTesting() ends up waiting forever in cases around shutdown, either if
called after shutdown has completed, or while shutdown has started but is not
yet complete. So injecting calls to this method between each test (across many
/content and /chrome test targets) resulted in lots of test timeouts. I've also
noticed some existing test timeouts due to this.

So the changes to this file are intended to avoid that infinite wait.

This line right here might be a better place to add...
  is_idle_cv_.Signal();
... instead of the two callsites below.

That would make FlushForTesting() return immediately once Shutdown() had
started, whether FlushForTesting() was called before or after Shutdown().

[michaeln, 2012-12-22 00:34:00]

Drat, the latest tryrun still showing a problem in memory test: unit on
linux_valgrind when running BackgroundApplicationListModelTest.

Suppression (error hash=#70CB859E757653FE#):
For more info on using suppressions see
http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/mem...
{
<insert_a_suppression_name_here>
Memcheck:Leak
fun:_Znw*
fun:_ZN4base4BindIMN11dom_storage17DomStorageContextEFvRK4GURLE13scoped_refptrIS2_ES3_EENS_8CallbackINS_8internal9BindStateINSB_13FunctorTraitsIT_E12RunnableTypeENSF_7RunTypeEFvNSB_19CallbackParamTraitsIT0_E11StorageTypeENSI_IT1_E11StorageTypeEEE14UnboundRunTypeEEESE_RKSJ_RKSM_
fun:_ZN7content21DOMStorageContextImpl18DeleteLocalStorageERK4GURL
fun:_ZN7content20StoragePartitionImpl23AsyncClearDataForOriginERK4GURLPN3net23URLRequestContextGetterE
fun:_ZN10extensions11DataDeleter13StartDeletingEP7ProfileRKSsRK4GURL
fun:_ZN16ExtensionService18UninstallExtensionESsbPSbItN4base20string16_char_traitsESaItEE
fun:_ZN52BackgroundApplicationListModelTest_ExplicitTest_Test8TestBodyEv
}

[michaeln, 2012-12-22 02:21:18]

I'm starting to wonder if FlushForTesting() actually accomplishes that in all
case? Ikd, maybe somewhere in the case of a thread_being_created_?

[michaeln, 2012-12-28 01:36:02]

Hi Fred,

Use of SWP for dom storage has led me down this trail...

* problem: valgrind reported leaks apparently stemming from tasks that go unrun
 soln: flush tasks between tests

* problem: flush can hang if called after shutdown has been called resulting in
test time outs
 soln: dont do that, return immediately in that case

* cleanup: switch dom storage to use SWPs delay mechansims (instead of the
hackish trip thru the IO loop to produce the desired delay), but flush currently
waits for all delays to complete
 soln: change flush to not flush delayed tasks

* problem: some SWP unittests depend on flush blocking till delayed tasks
complete
 soln: make a variant that will block on delayed tasks too

... a longer and twistier road than i anticipated!

[akalin, 2012-12-28 09:50:18]

On 2012/12/28 01:36:02, michaeln wrote:
> Hi Fred,
> 
> Use of SWP for dom storage has led me down this trail...
> 
> * problem: valgrind reported leaks apparently stemming from tasks that go
unrun
>  soln: flush tasks between tests

Hm why is it that tasks not running causes leaks?  Are they not destroyed?  If
not, why not?  If so, why isn't it then destroying the relevant variables?

> * problem: flush can hang if called after shutdown has been called resulting
in
> test time outs
>  soln: dont do that, return immediately in that case

Although we can change SWP to handle flushing during shutdown similar to how
you're doing it, that introduces complications so I'd rather not do it unless
really needed (re. the above).

> * cleanup: switch dom storage to use SWPs delay mechansims (instead of the
> hackish trip thru the IO loop to produce the desired delay), but flush
currently
> waits for all delays to complete
>  soln: change flush to not flush delayed tasks
> 
> * problem: some SWP unittests depend on flush blocking till delayed tasks
> complete
>  soln: make a variant that will block on delayed tasks too

Ah, I didn't think of this.  Yeah, I'm okay with having something like
RunPendingTasks() / RunUntilIdle() for SWP.  I think this is your other CL?

[akalin, 2012-12-28 09:57:04]

On 2012/12/28 09:50:18, akalin wrote:
> Ah, I didn't think of this.  Yeah, I'm okay with having something like
> RunPendingTasks() / RunUntilIdle() for SWP.  I think this is your other CL?

Actually, maybe not.  Isn't this inherently racy?  And this is needed only for
flushing tasks between tests?  Yeah, if we can fix the leaks some other way that
would be better.

[michaeln, 2013-01-02 22:11:03]

> On 2012/12/28 01:36:02, michaeln wrote:
> > Hi Fred,
> > 
> > Use of SWP for dom storage has led me down this trail...
> > 
> > * problem: valgrind reported leaks apparently stemming from tasks that go
> unrun
> >  soln: flush tasks between tests
> 
> Hm why is it that tasks not running causes leaks?  Are they not destroyed?  If
> not, why not?  If so, why isn't it then destroying the relevant variables?

First, SWP is not shutdown in unittests ever.

Second, even it it where shutdown,  tasks that are not run are leaked by design
(i believe). The rational being that if dtors may assume that the task has
actually run and depend on artifacts of that. If tasks are destructed w/o having
been run, that assumption is broken.

> 
> > * problem: flush can hang if called after shutdown has been called resulting
> in
> > test time outs
> >  soln: dont do that, return immediately in that case
> 
> Although we can change SWP to handle flushing during shutdown similar to how
> you're doing it, that introduces complications so I'd rather not do it unless
> really needed (re. the above).

The fact that Shutdown() can lock up forever is a pretty bad bug that we should
fix.

> > * cleanup: switch dom storage to use SWPs delay mechansims (instead of the
> > hackish trip thru the IO loop to produce the desired delay), but flush
> currently
> > waits for all delays to complete
> >  soln: change flush to not flush delayed tasks
> > 
> > * problem: some SWP unittests depend on flush blocking till delayed tasks
> > complete
> >  soln: make a variant that will block on delayed tasks too
> 
> Ah, I didn't think of this.  Yeah, I'm okay with having something like
> RunPendingTasks() / RunUntilIdle() for SWP.  I think this is your other CL?

I can't make this switch over in dom storage until we have some reasonable
answers for these things in our test harnesses. I think flushing SWP tasks
between each test makes for good testing hygiene and has got to be part of the
recipe for a reasonable answer. Maybe we could shutdown and destroy the SWP
between each test fixture?

[michaeln, 2013-01-02 22:30:07]

> Maybe we could shutdown and destroy the SWP between each test fixture?

I think this might be an avenue to pursue in getting SWP tasks to be handled in
a more orderly fashion during testing.

FlushForTesting as currently composed works fine for SWP unittests, but is not
so great for the larger universe of browser_tests, interactive_ui tests, and
general chrome and content unittests many of which happen to indirectly utilize
content::BrowserThread's SWP.

So... a maybe different <<>>ForTesting method that completes all pending work
(possibly including 'delayed' work by completing timeouts immediately) and
results in all task closures being destructed prior to return.

[akalin, 2013-01-04 01:19:21]

On 2013/01/02 22:11:03, michaeln wrote:
> > On 2012/12/28 01:36:02, michaeln wrote:
> > > Hi Fred,
> > > 
> > > Use of SWP for dom storage has led me down this trail...
> > > 
> > > * problem: valgrind reported leaks apparently stemming from tasks that go
> > unrun
> > >  soln: flush tasks between tests
> > 
> > Hm why is it that tasks not running causes leaks?  Are they not destroyed? 
If
> > not, why not?  If so, why isn't it then destroying the relevant variables?
> 
> First, SWP is not shutdown in unittests ever.
> 
> Second, even it it where shutdown,  tasks that are not run are leaked by
design
> (i believe). The rational being that if dtors may assume that the task has
> actually run and depend on artifacts of that. If tasks are destructed w/o
having
> been run, that assumption is broken.

I don't understand this.  In general, destructors related to tasks shouldn't
assume the task is run.  If there are broken tasks that do, we should flush them
out.

Basically, there should be a barrier between unit-tests that makes sure
everything is cleaned up.  I'm confused as to why SWP isn't shutdown, or some
sort of "ShutdownForTesting" mode that fails if there are tasks left to be run,
or maybe just deletes them.

> 
> > 
> > > * problem: flush can hang if called after shutdown has been called
resulting
> > in
> > > test time outs
> > >  soln: dont do that, return immediately in that case
> > 
> > Although we can change SWP to handle flushing during shutdown similar to how
> > you're doing it, that introduces complications so I'd rather not do it
unless
> > really needed (re. the above).
> 
> The fact that Shutdown() can lock up forever is a pretty bad bug that we
should
> fix.

But it can only lock up when test-only functions are called.  Therefore, we
shouldn't complicated "real" code to fix the test-only code -- we should fix the
test-only code to not cause locks.  SWP code is already too complicated.

> > > * cleanup: switch dom storage to use SWPs delay mechansims (instead of the
> > > hackish trip thru the IO loop to produce the desired delay), but flush
> > currently
> > > waits for all delays to complete
> > >  soln: change flush to not flush delayed tasks
> > > 
> > > * problem: some SWP unittests depend on flush blocking till delayed tasks
> > > complete
> > >  soln: make a variant that will block on delayed tasks too
> > 
> > Ah, I didn't think of this.  Yeah, I'm okay with having something like
> > RunPendingTasks() / RunUntilIdle() for SWP.  I think this is your other CL?
> 
> I can't make this switch over in dom storage until we have some reasonable
> answers for these things in our test harnesses. I think flushing SWP tasks
> between each test makes for good testing hygiene and has got to be part of the
> recipe for a reasonable answer. Maybe we could shutdown and destroy the SWP
> between each test fixture?

I'm okay with shutting down and destroying the SWP between each tests.

[akalin, 2013-01-04 01:20:01]

On 2013/01/02 22:30:07, michaeln wrote:
> > Maybe we could shutdown and destroy the SWP between each test fixture?
> 
> I think this might be an avenue to pursue in getting SWP tasks to be handled
in
> a more orderly fashion during testing.
> 
> FlushForTesting as currently composed works fine for SWP unittests, but is not
> so great for the larger universe of browser_tests, interactive_ui tests, and
> general chrome and content unittests many of which happen to indirectly
utilize
> content::BrowserThread's SWP.
> 
> So... a maybe different <<>>ForTesting method that completes all pending work
> (possibly including 'delayed' work by completing timeouts immediately) and
> results in all task closures being destructed prior to return.

Yeah, it looks like I came to the same conclusion in my reply to your last
e-mail. :)

But I'm on leave this month, so perhaps jar@ is a better person to consult about
this.

[michaeln, 2013-01-04 20:19:26]

> Yeah, it looks like I came to the same conclusion in my reply to your last
> e-mail. :)

Yup, should help with general test flakiness to clean house between tests and
allow for broader SWP adoption. To adopt further right now would probably add
more test flakiness noise.

> But I'm on leave this month, so perhaps jar@ is a better person to consult
about
> this.

Bummer ;)

[michaeln, 2013-01-04 22:31:28]

I've filed a separate bug about the SWP task cleanup specifically.
https://code.google.com/p/chromium/issues/detail?id=168415

[michaeln, 2013-01-10 22:17:38]

Trybots somewhat consistetly complaining about the same set of tests on linux,
win, and mac with this patched in, so I've got something to look at.

content_unittests unit_tests, device_unittests
failed to launch

interactive_ui_tests
TestCancel
TestClose
TestPositioning
TestQueueing
TestShow
TestVariableSize

browser_tests
DeleteSingleFile
StartAndShutdown
StartBrowserWithPolicy
StartBrowserWithoutPolicy
InstallExtensionEvent
NetworkBytesRead
TwoProfileUncleanExit

[michaeln, 2013-01-10 22:46:46]

some low hanging... fruit CleanupForTesting() should return immediately if no
tasks have ever been posted

[michaeln, 2013-01-10 23:25:30]

Ahh... snapshot 10 is looking not bad!

[michaeln, 2013-01-11 00:18:13]

> Ahh... snapshot 10 is looking not bad!

Overwhelmingly, tests are running as expected, and passing with removal of the
valgrind suppressions is a good thing.

PageCycler.FailProvisionalLoads crashing consistently across OSs, but looks
maybe only tangentially unrelated to my changes, there are use-after-free bugs
in there. The ASAN bot has the best data on it. Running (dbg) in isolation on my
system, this test is has passed everytime... trying with --gtest_repeat=1000 to
hopefully see it in action.


[----------] 4 tests from PageCyclerTest
[ RUN      ] PageCyclerTest.FailProvisionalLoads
=================================================================
==9042== ERROR: AddressSanitizer: heap-use-after-free on address 0x7f28ec03fd88
at pc 0x82e3276 bp 0x7ffff6b65750 sp 0x7ffff6b65748
WRITE of size 8 at 0x7f28ec03fd88 thread T0
    #0 0x82e3275 in
testing::internal::UntypedFunctionMockerBase::RegisterOwner(void const*) ???:0
    #1 0x2673e77 in PageCyclerTest_FailProvisionalLoads_Test::TestBody() ???:0
    #2 0x8970ffd in testing::Test::Run() ???:0
    #3 0x8972ee7 in testing::TestInfo::Run() ???:0
    #4 0x8973e56 in testing::TestCase::Run() ???:0
    #5 0x8980b08 in testing::internal::UnitTestImpl::RunAllTests() ???:0
    #6 0x8980053 in testing::UnitTest::Run() ???:0
    #7 0xdcd5910 in base::TestSuite::Run() ???:0
    #8 0xd310be4 in main ???:0
    #9 0x7f28f720fc4c in __libc_start_main
/build/buildd/eglibc-2.11.1/csu/libc-start.c:226
0x7f28ec03fd88 is located 328 bytes inside of 648-byte region
[0x7f28ec03fc40,0x7f28ec03fec8)
freed by thread T0 here:
    #0 0x5a0eb2 in operator delete(void*) ??:0
    #1 0x5d7ee3c in
base::internal::BindState<base::internal::RunnableAdapter<void
(PageCycler::*)()>, void ()(PageCycler*), void ()(PageCycler*)>::~BindState()
???:0
    #2 0xaf9cfc1 in base::PendingTask::~PendingTask() ???:0
    #3 0xaf71fb6 in MessageLoop::DoWork() ???:0
    #4 0xb03d607 in (anonymous namespace)::WorkSourceDispatch(_GSource*, int
(*)(void*), void*) ../../base/message_pump_glib.cc:0
    #5 0x7f28fbc748c1 in g_main_dispatch
/build/buildd/glib2.0-2.24.1/glib/gmain.c:1960
previously allocated by thread T0 here:
    #0 0x5a0cf2 in operator new(unsigned long) ??:0
    #1 0x26703d1 in PageCyclerTest_FailProvisionalLoads_Test::TestBody() ???:0
    #2 0x8970ffd in testing::Test::Run() ???:0
    #3 0x8972ee7 in testing::TestInfo::Run() ???:0
    #4 0x8973e56 in testing::TestCase::Run() ???:0
    #5 0x8980b08 in testing::internal::UnitTestImpl::RunAllTests() ???:0
    #6 0x8980053 in testing::UnitTest::Run() ???:0
    #7 0xdcd5910 in base::TestSuite::Run() ???:0
    #8 0xd310be4 in main ???:0

[michaeln, 2013-01-14 23:56:10]

I'm ignoring the SLHostStateTest leaks (see crbug/82974) but am taking a closer
look at the PageCyclerTest... somethings look fishy in there.

[michaeln, 2013-01-15 02:11:49]

Hi Jim, here's the CL i was talking about.

[jar (doing other things), 2013-01-15 22:59:59]

I just skimmed this (wasn't sure if you were proposing this as final).

https://codereview.chromium.org/11649032/diff/88001/base/test/task_runner_tes...
File base/test/task_runner_test_template.h (right):

https://codereview.chromium.org/11649032/diff/88001/base/test/task_runner_tes...
base/test/task_runner_test_template.h:85: void WaitForTasksToComplete(int n);
nit: avoid single letter variable names.  Suggest:
task_count

https://codereview.chromium.org/11649032/diff/88001/base/threading/sequenced_...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/11649032/diff/88001/base/threading/sequenced_...
base/threading/sequenced_worker_pool.cc:698: HandleCleanup();
It is surprising we call this in 3 of the 4 states.

Probably more readable to do all the state checking in the HandleCleanup().

https://codereview.chromium.org/11649032/diff/88001/base/threading/sequenced_...
base/threading/sequenced_worker_pool.cc:896: // Let's try running all delayed
tasks during cleanup?
What's up with the question mark?

...also... I thought you gave up on running them, and were going to delete them.

[michaeln, 2013-01-15 23:16:08]

Thnx for looking.

> I just skimmed this (wasn't sure if you were proposing this as final).

Not quite yet, I'm stilling looking into the PageCycler test failure. Its
forcing me to do something with my linux box. I'm replicating this patch there
now.

> base/test/task_runner_test_template.h:85: void WaitForTasksToComplete(int n);
> nit: avoid single letter variable names.  Suggest:
> task_count

Will do.

> Probably more readable to do all the state checking in the HandleCleanup().

I could hoist the entire method into this loop.


> ...also... I thought you gave up on running them, and were going to delete
them.

Just playing with . I propose to delete them per the block that adds them to the
delete_these collection.

[michaeln, 2013-01-16 01:03:30]

> Not quite yet, I'm stilling looking into the PageCycler test failure. Its
> forcing me to do something with my linux box. I'm replicating this patch there
> now.

That's annoying... the pagecycler test runs fine on my linux box.

[michaeln, 2013-01-18 21:20:03]

Idk wassup with the page cycler test  I'm trying a snapshot w/o my changes to
see if that test passes irrespective of my changes?

[michaeln, 2013-01-19 00:42:21]

Test passed when reverting.

Still don't get it but reduced the problem a little. The failure occurs w/o
invoking the new CleanupForTesting() method between tests. That method does get
invoked in the PageCycler test multiple times in the act of its running (since
I've replaced FlushForTesting() with this method too). Somehow the crash is
related to differences in behavior between the old method.

A difference between the old/new is that delayed tasks are deleted. There are no
delayed tasks involved in the PageCycler test specifically, but there could be
delayed tasks from prior tests involved. I'll try change that doesn't delete
delayed task just to see if that makes a difference?

Another difference between the old/new is that tasks are executed serially by
the new whereas parallelization could occur in the old. Dont see why that would
matter, but it is a difference.

Old try runs indicate that the main thread was where the crash occurred (not a
background worker thread). There's a memory corrupting bug happening on the bots
that frustratingly does not replicate when i run this locally.

[michaeln, 2013-01-19 02:21:01]

ah... maybe something to go on form this sea of red that happened to get
generated in one run...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_valg...

All kinds of redness around MockPageCycler's MOCK_METHODS. What does that voodoo
macro do anyway. The MockPageCycler is a thread-safe refcounted class, it's
using MOCK_METHODS which employ non-thread-safe link_ptr<>s under the covers in
some way. Yup, sounds like an impedance mismatch is possible in there.

What does this mean exactly and how can I do what its doing w/o the voodoo macro
mania, in plain spoken c++.
  EXPECT_CALL(*page_cycler(),
              DidFinishLoad(kFrameID, kAboutURL, kIsMainFrame, _))
      .WillOnce(Invoke(page_cycler(),
                       &MockPageCycler::PageCyclerDidFinishLoad));

17:27:57 memcheck_analyze.py [ERROR] Command: src/sconsbuild/Release/unit_tests
--gtest_print_time --gtest_filter=-
InvalidRead
Invalid read of size 8
  testing::internal::linked_ptr<testing::MatcherInterface<long> const>::get()
const (testing/gtest/include/gtest/internal/gtest-linked_ptr.h:172)
  void testing::internal::linked_ptr<testing::MatcherInterface<long>
const>::copy<testing::MatcherInterface<long>
const>(testing::internal::linked_ptr<testing::MatcherInterface<long> const>
const*) (testing/gtest/include/gtest/internal/gtest-linked_ptr.h:204)
  testing::internal::linked_ptr<testing::MatcherInterface<long>
const>::linked_ptr(testing::internal::linked_ptr<testing::MatcherInterface<long>
const> const&) (testing/gtest/include/gtest/internal/gtest-linked_ptr.h:149)
 
testing::internal::MatcherBase<long>::MatcherBase(testing::internal::MatcherBase<long>
const&) (testing/gmock/include/gmock/gmock-matchers.h:191)
  testing::Matcher<long>::Matcher(testing::Matcher<long> const&)
(testing/gmock/include/gmock/gmock-matchers.h:251)
  std::tr1::_Tuple_impl<0, testing::Matcher<long>, testing::Matcher<bool>,
testing::Matcher<GURL const&>, testing::Matcher<int>,
testing::Matcher<std::basic_string<unsigned short, base::string16_char_traits,
std::allocator<unsigned short> > const&>,
testing::Matcher<content::RenderViewHost*>
>::_Tuple_impl(std::tr1::_Tuple_impl<0, testing::Matcher<long>,
testing::Matcher<bool>, testing::Matcher<GURL const&>, testing::Matcher<int>,
testing::Matcher<std::basic_string<unsigned short, base::string16_char_traits,
std::allocator<unsigned short> > const&>,
testing::Matcher<content::RenderViewHost*> > const&)
(/usr/include/c++/4.4/tr1/tuple:107)
  std::tr1::tuple<testing::Matcher<long>, testing::Matcher<bool>,
testing::Matcher<GURL const&>, testing::Matcher<int>,
testing::Matcher<std::basic_string<unsigned short, base::string16_char_traits,
std::allocator<unsigned short> > const&>,
testing::Matcher<content::RenderViewHost*>
>::tuple(std::tr1::tuple<testing::Matcher<long>, testing::Matcher<bool>,
testing::Matcher<GURL const&>, testing::Matcher<int>,
testing::Matcher<std::basic_string<unsigned short, base::string16_char_traits,
std::allocator<unsigned short> > const&>,
testing::Matcher<content::RenderViewHost*> > const&)
(/usr/include/c++/4.4/tr1/tuple:144)
  testing::internal::TypedExpectation<void ()(long, bool, GURL const&, int,
std::basic_string<unsigned short, base::string16_char_traits,
std::allocator<unsigned short> > const&,
content::RenderViewHost*)>::TypedExpectation(testing::internal::FunctionMockerBase<void
()(long, bool, GURL const&, int, std::basic_string<unsigned short,
base::string16_char_traits, std::allocator<unsigned short> > const&,
content::RenderViewHost*)>*, char const*, int, std::basic_string<char,
std::char_traits<char>, std::allocator<char> > const&,
std::tr1::tuple<testing::Matcher<long>, testing::Matcher<bool>,
testing::Matcher<GURL const&>, testing::Matcher<int>,
testing::Matcher<std::basic_string<unsigned short, base::string16_char_traits,
std::allocator<unsigned short> > const&>,
testing::Matcher<content::RenderViewHost*> > const&)
(testing/gmock/include/gmock/gmock-spec-builders.h:886)
  testing::internal::FunctionMockerBase<void ()(long, bool, GURL const&, int,
std::basic_string<unsigned short, base::string16_char_traits,
std::allocator<unsigned short> > const&,
content::RenderViewHost*)>::AddNewExpectation(char const*, int,
std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&,
std::tr1::tuple<testing::Matcher<long>, testing::Matcher<bool>,
testing::Matcher<GURL const&>, testing::Matcher<int>,
testing::Matcher<std::basic_string<unsigned short, base::string16_char_traits,
std::allocator<unsigned short> > const&>,
testing::Matcher<content::RenderViewHost*> > const&)
(testing/gmock/include/gmock/gmock-spec-builders.h:1536)
  testing::internal::MockSpec<void ()(long, bool, GURL const&, int,
std::basic_string<unsigned short, base::string16_char_traits,
std::allocator<unsigned short> > const&,
content::RenderViewHost*)>::InternalExpectedAt(char const*, int, char const*,
char const*) (testing/gmock/include/gmock/gmock-spec-builders.h:1265)
  PageCyclerTest_FailProvisionalLoads_Test::TestBody()
(chrome/browser/page_cycler/page_cycler_unittest.cc:250)
Address 0x21b443f0 is 528 bytes inside a block of size 648 free'd
  operator delete(void*) (m_replacemalloc/vg_replace_malloc.c:1149)
  MockPageCycler::~MockPageCycler()
(chrome/browser/page_cycler/page_cycler_unittest.cc:97)
  base::RefCountedThreadSafe<PageCycler,
base::DefaultRefCountedThreadSafeTraits<PageCycler> >::DeleteInternal(PageCycler
const*) (./base/memory/ref_counted.h:151)
  base::DefaultRefCountedThreadSafeTraits<PageCycler>::Destruct(PageCycler
const*) (./base/memory/ref_counted.h:114)
  base::RefCountedThreadSafe<PageCycler,
base::DefaultRefCountedThreadSafeTraits<PageCycler> >::Release() const
(./base/memory/ref_counted.h:142)
  base::internal::MaybeRefcount<true, PageCycler*>::Release(PageCycler*)
(./base/bind_helpers.h:465)
  base::internal::BindState<base::internal::RunnableAdapter<void
(PageCycler::*)()>, void ()(PageCycler*), void ()(PageCycler*)>::~BindState()
(./base/bind_internal.h:2569)
  base::RefCountedThreadSafe<base::internal::BindStateBase,
base::DefaultRefCountedThreadSafeTraits<base::internal::BindStateBase>
>::DeleteInternal(base::internal::BindStateBase const*)
(./base/memory/ref_counted.h:151)
 
base::DefaultRefCountedThreadSafeTraits<base::internal::BindStateBase>::Destruct(base::internal::BindStateBase
const*) (./base/memory/ref_counted.h:114)
  base::RefCountedThreadSafe<base::internal::BindStateBase,
base::DefaultRefCountedThreadSafeTraits<base::internal::BindStateBase>
>::Release() const (./base/memory/ref_counted.h:142)
  scoped_refptr<base::internal::BindStateBase>::~scoped_refptr()
(./base/memory/ref_counted.h:250)
  base::internal::CallbackBase::~CallbackBase() (base/callback_internal.cc:35)
  base::Callback<void ()()>::~Callback() (./base/callback_forward.h:11)
  base::PendingTask::~PendingTask() (base/pending_task.cc:38)
  MessageLoop::DoWork() (base/message_loop.cc:670)
  base::MessagePumpGlib::HandleDispatch() (base/message_pump_glib.cc:268)
  (anonymous namespace)::WorkSourceDispatch(_GSource*, int (*)(void*), void*)
(base/message_pump_glib.cc:105)
  g_main_context_dispatch (/build/buildd/glib2.0-2.24.1/glib/gmain.c:1960)
  g_main_context_iterate (/build/buildd/glib2.0-2.24.1/glib/gmain.c:2591)
  g_main_context_iteration (/build/buildd/glib2.0-2.24.1/glib/gmain.c:2654)
  base::MessagePumpGlib::RunWithDispatcher(base::MessagePump::Delegate*,
base::MessagePumpDispatcher*) (base/message_pump_glib.cc:199)
  base::MessagePumpGlib::Run(base::MessagePump::Delegate*)
(base/message_pump_glib.cc:291)
  MessageLoop::RunInternal() (base/message_loop.cc:430)
  MessageLoop::RunHandler() (base/message_loop.cc:403)
  base::RunLoop::Run() (base/run_loop.cc:45)
  base::RunLoop::RunUntilIdle() (base/run_loop.cc:51)
  MessageLoop::RunUntilIdle() (base/message_loop.cc:315)
  PageCyclerTest::PumpLoop()
(chrome/browser/page_cycler/page_cycler_unittest.cc:151)
  PageCyclerTest::FailProvisionalLoad(int, std::basic_string<unsigned short,
base::string16_char_traits, std::allocator<unsigned short> >&)
(chrome/browser/page_cycler/page_cycler_unittest.cc:133)
  PageCyclerTest_FailProvisionalLoads_Test::TestBody()
(chrome/browser/page_cycler/page_cycler_unittest.cc:241)
Suppression (error hash=#6CB1699E6EDE1F42#):
  For more info on using suppressions see
http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/mem...
{
   <insert_a_suppression_name_here>
   Memcheck:Unaddressable
   fun:_ZNK7testing8internal10linked_ptrIKNS_16MatcherInterfaceIlEEE3getEv
  
fun:_ZN7testing8internal10linked_ptrIKNS_16MatcherInterfaceIlEEE4copyIS4_EEvPKNS1_IT_EE
   fun:_ZN7testing8internal10linked_ptrIKNS_16MatcherInterfaceIlEEEC1ERKS5_
   fun:_ZN7testing8internal11MatcherBaseIlEC2ERKS2_
   fun:_ZN7testing7MatcherIlEC1ERKS1_
  
fun:_ZNSt3tr111_Tuple_implILi0EIN7testing7MatcherIlEENS2_IbEENS2_IRK4GURLEENS2_IiEENS2_IRKSbItN4base20string16_char_traitsESaItEEEENS2_IPN7content14RenderViewHostEEEEEC2ERKSL_
  
fun:_ZNSt3tr15tupleIIN7testing7MatcherIlEENS2_IbEENS2_IRK4GURLEENS2_IiEENS2_IRKSbItN4base20string16_char_traitsESaItEEEENS2_IPN7content14RenderViewHostEEEEEC1ERKSL_
  
fun:_ZN7testing8internal16TypedExpectationIFvlbRK4GURLiRKSbItN4base20string16_char_traitsESaItEEPN7content14RenderViewHostEEEC1EPNS0_18FunctionMockerBaseISE_EEPKciRKSsRKNSt3tr15tupleIINS_7MatcherIlEENSP_IbEENSP_IS4_EENSP_IiEENSP_ISA_EENSP_ISD_EEEEE
  
fun:_ZN7testing8internal18FunctionMockerBaseIFvlbRK4GURLiRKSbItN4base20string16_char_traitsESaItEEPN7content14RenderViewHostEEE17AddNewExpectationEPKciRKSsRKNSt3tr15tupleIINS_7MatcherIlEENSM_IbEENSM_IS4_EENSM_IiEENSM_ISA_EENSM_ISD_EEEEE
  
fun:_ZN7testing8internal8MockSpecIFvlbRK4GURLiRKSbItN4base20string16_char_traitsESaItEEPN7content14RenderViewHostEEE18InternalExpectedAtEPKciSH_SH_
   fun:_ZN40PageCyclerTest_FailProvisionalLoads_Test8TestBodyEv
}

[michaeln, 2013-01-23 00:16:07]

Found bugs and fixed them, ptal.

https://codereview.chromium.org/11649032/diff/135033/base/threading/sequenced...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/11649032/diff/135033/base/threading/sequenced...
base/threading/sequenced_worker_pool.cc:794: void
SequencedWorkerPool::Inner::HandleCleanup() {
The synchronization logic is mostly in here. This method arranges for one worker
to serially cleanup pending tasks. That worker waits for the others to enter an
idle state before proceeding. Then this method waits for all workers to return
from idling prior to finally waking the caller on line 828.

In some sense serializing the cleanup work is a simplification, but this logic
to accomplish that is not so simple.

[michaeln, 2013-01-23 00:22:19]

Also keep in mind that in production code cleanup_state_ is always DONE.

[michaeln, 2013-01-25 21:40:44]

Looks like this new function is functioning as intended and existing tests pass
with this function replacing the old Flush function and being automatically
called between each test by the test harness. But this does not resolve some odd
leaks being reported by valgrind. I don't have an explantion for the leaks, but
I think this cleanup between tests is a pretty good improvement.

<<<<<< A heap allocation done by base::Bind is reported as leaked.

void DOMStorageContextImpl::DeleteLocalStorage(const GURL& origin) {
  DCHECK(context_);
  context_->task_runner()->PostShutdownBlockingTask(
      FROM_HERE,
      DomStorageTaskRunner::PRIMARY_SEQUENCE,
      base::Bind(&DomStorageContext::DeleteLocalStorage, context_, origin));
<<<<<< 
}

<<<<<< A heap allocation by FilePath is reported as leaked.

bool CreateDirectory(const FilePath& full_path) {
 ....
  std::vector<FilePath> subpaths;
  // Collect a list of all parent directories.
  FilePath last_path = full_path;
  subpaths.push_back(full_path);
  for (FilePath path = full_path.DirName();
       path.value() != last_path.value(); path = path.DirName()) {
    subpaths.push_back(path);   <<<<<< 
    last_path = path;
  }
 ...
}

[michaeln, 2013-01-28 20:31:43]

ping... I've removed the removal of the memcheck suppressions from this CL.

[jar (doing other things), 2013-01-28 20:45:14]

"Best practice" is to include the name(s) of the target of your pings.

T'was it me??


On Mon, Jan 28, 2013 at 12:31 PM, <michaeln@chromium.org> wrote:

> ping... I've removed the removal of the memcheck suppressions from this CL.
>
>
https://codereview.chromium.**org/11649032/<https://codereview.chromium.org/1...
>

[michaeln, 2013-01-28 20:58:26]

ok... ping jar

[michaeln, 2013-01-31 21:42:41]

ping, @jar @akalin @somebody :)

[michaeln, 2013-02-12 02:03:51]

hey guys, this CL is going to bit rot unless you take a look. i think that would
be a shame since it does something pretty useful around getting a handle on
pending tasks between test fixtures. if you have some feedback/comments that
would be awesome.

or if you would rather i drop this CL... lmk so i can delete it

[jar (doing other things), 2013-02-12 02:26:01]

I apologize for my lack of responsiveness.  I keep getting pulled every which
way.  I'll try very hard to get to this shortly.

[michaeln, 2013-02-21 22:16:27]

patch is still in my local copy, but if/when i get a merge conflict i'll be
deleting this CL from my local copy rather than resolving conflicts

[jar (doing other things), 2013-02-22 23:39:47]

akalin: Can you answer the one question for you listed below?

Assuming a good answer, and nits fixed, LGTM.

https://codereview.chromium.org/11649032/diff/145001/base/test/sequenced_task...
File base/test/sequenced_task_runner_test_template.cc (right):

https://codereview.chromium.org/11649032/diff/145001/base/test/sequenced_task...
base/test/sequenced_task_runner_test_template.cc:98: while (task_end_count_ !=
count)
nit: per your suggestion, perhaps we should use >= and
WaitForTaskCompletionCountOfAtLeast()
WaitForCountTasksToComplete()

https://codereview.chromium.org/11649032/diff/145001/base/test/sequenced_task...
File base/test/sequenced_task_runner_test_template.h (right):

https://codereview.chromium.org/11649032/diff/145001/base/test/sequenced_task...
base/test/sequenced_task_runner_test_template.h:122: : task_tracker_(new
internal::SequencedTaskTracker()) {}
nit: indent 4 more.

https://codereview.chromium.org/11649032/diff/145001/base/test/task_runner_te...
File base/test/task_runner_test_template.cc (right):

https://codereview.chromium.org/11649032/diff/145001/base/test/task_runner_te...
base/test/task_runner_test_template.cc:24: ++task_run_counts_[i];
aklin: Why do we increment count, when we *don't* run??

https://codereview.chromium.org/11649032/diff/145001/base/test/task_runner_te...
base/test/task_runner_test_template.cc:34: void
TaskTracker::WaitForTasksToComplete(int count) {
nit: rename, and use >=

https://codereview.chromium.org/11649032/diff/145001/base/test/task_runner_te...
File base/test/task_runner_test_template.h (right):

https://codereview.chromium.org/11649032/diff/145001/base/test/task_runner_te...
base/test/task_runner_test_template.h:85: void WaitForTasksToComplete(int
count);
nit: same rename as other template.

https://codereview.chromium.org/11649032/diff/145001/base/threading/sequenced...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/11649032/diff/145001/base/threading/sequenced...
base/threading/sequenced_worker_pool.cc:627: return;
What should the cleanup_state_ be when we return?

Perchance we should move line 628 in front of line 626.

...alternatively... if you feel these are mutually exclusive (you call shutdown,
or cleanup, but not both) you should say so.  (I'm assuming this per our chat...
but please add to comment).

[michaeln, 2013-02-23 01:10:16]

thnx!

https://codereview.chromium.org/11649032/diff/145001/base/test/sequenced_task...
File base/test/sequenced_task_runner_test_template.cc (right):

https://codereview.chromium.org/11649032/diff/145001/base/test/sequenced_task...
base/test/sequenced_task_runner_test_template.cc:98: while (task_end_count_ !=
count)
On 2013/02/22 23:39:47, jar wrote:
> nit: per your suggestion, perhaps we should use >= and
> WaitForTaskCompletionCountOfAtLeast()
> WaitForCountTasksToComplete()

Done, here and elsewhere.

https://codereview.chromium.org/11649032/diff/145001/base/test/sequenced_task...
File base/test/sequenced_task_runner_test_template.h (right):

https://codereview.chromium.org/11649032/diff/145001/base/test/sequenced_task...
base/test/sequenced_task_runner_test_template.h:122: : task_tracker_(new
internal::SequencedTaskTracker()) {}
On 2013/02/22 23:39:47, jar wrote:
> nit: indent 4 more.

Done.

https://codereview.chromium.org/11649032/diff/145001/base/threading/sequenced...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/11649032/diff/145001/base/threading/sequenced...
base/threading/sequenced_worker_pool.cc:627: return;
On 2013/02/22 23:39:47, jar wrote:
> What should the cleanup_state_ be when we return?

Should always be CLEANUP_DONE on entry and return. The CHECK asserts that the
method is not re-entrant. Added a comment about how Shutdown() and Cleanup()
should not be called concurrently. The early return when shutdown_called_ allows
Cleanup() to be invoked after Shutdown() has completed.

> Perchance we should move line 628 in front of line 626.

Done.

[michaeln, 2013-02-23 02:38:46]

https://codereview.chromium.org/11649032/diff/145001/base/test/task_runner_te...
File base/test/task_runner_test_template.cc (right):

https://codereview.chromium.org/11649032/diff/145001/base/test/task_runner_te...
base/test/task_runner_test_template.cc:24: ++task_run_counts_[i];
On 2013/02/22 23:39:47, jar wrote:
> aklin: Why do we increment count, when we *don't* run??

In an effort to speed the process... i'll take a stab at an answer.

  Closure empty;
  empty.Run(); // This will crash

For the sake of convenience some of the test fixtures post wrapped empty/null
closures. For example, in TYPED_TEST_P(TaskRunnerTest, Basic), a bunch of empty
wrapped tasks are posted.

  const Closure& ith_task = this->task_tracker_->WrapTask(Closure(), i);

What's being counted is how many tasks the SequencnedWorkerPool processes, SWP
is calling TaskTracker::RunTask, internally that method may or may not call the
wrapped 'task' depending on how it was composed.

[michaeln, 2013-02-25 21:40:21]

> aklin: Why do we increment count, when we *don't* run??
> Assuming a good answer, and nits fixed, LGTM.

Nits are fixed and i'm assuming my answer works (its pretty cut and dry why that
is as it is)... CQ'ing.

[commit-bot: I haz the power, 2013-02-25 21:41:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/11649032/163001

[commit-bot: I haz the power, 2013-02-25 21:41:19]

Presubmit check for 11649032-163001 failed and returned exit status 1.

INFO:root:Found 10 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/base/PRESUBMIT.py

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  base/threading/sequenced_worker_pool.cc:283
    void CleanupForTesting(); \
  base/threading/sequenced_worker_pool.cc:285
    void SignalHasWorkForTesting(); \
  base/threading/sequenced_worker_pool.cc:287
    int GetWorkSignalCountForTesting() const; \
  base/threading/sequenced_worker_pool.cc:1227
    inner_->CleanupForTesting(); \
  base/threading/sequenced_worker_pool.cc:1231
    inner_->SignalHasWorkForTesting(); \
  content/browser/browser_thread_impl.cc:94
    g_globals.Get().blocking_pool->FlushForTesting();

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    content/browser/browser_thread_impl.h
    content/public/test/content_test_suite_base.cc
    content/browser/browser_thread_impl.cc

Presubmit checks took 2.3s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[michaeln, 2013-02-25 22:01:49]

+sky for OWNERS

Missing LGTM from an OWNER for these files:
    content/browser/browser_thread_impl.h
    content/public/test/content_test_suite_base.cc
    content/browser/browser_thread_impl.cc

[sky, 2013-02-25 22:59:55]

I'm not a fan of the ForTesting style. I prefer a friend class. I'm leaving this
to John for his thoughts.

[michaeln, 2013-02-25 23:37:31]

Modulo stylisms wdyt? I half expect John to defer to you on the testing
infrastructure and so to cut to the chase... does the logic and behavior change
look good?

Worth pointing out, this does not affect the public content interface.

I'd be fine with declaring ContentTestSuiteListener as a friend (would prefer
that as well honestly). That class is in an annon namespace, not sure what
voodoo is needed to make that declaration, but so long as some voodoo works on
all our compilers... fine by me.

 31 namespace {
 32 class ContentTestSuiteListener : public testing::EmptyTestEventListener {
 33  public:
 34   ContentTestSuiteListener() {
 35   }
 36   virtual void OnTestEnd(const testing::TestInfo& test_info) OVERRIDE {
 37     BrowserThreadImpl::FlushThreadPoolForTesting();
 38   }
 39   DISALLOW_COPY_AND_ASSIGN(ContentTestSuiteListener);
 40 };
 41 }  // namespace

[akalin, 2013-02-26 00:12:22]

lgtm after nits

(I tried not to bikeshed this too much.)

https://codereview.chromium.org/11649032/diff/145001/base/test/task_runner_te...
File base/test/task_runner_test_template.cc (right):

https://codereview.chromium.org/11649032/diff/145001/base/test/task_runner_te...
base/test/task_runner_test_template.cc:24: ++task_run_counts_[i];
On 2013/02/23 02:38:46, michaeln wrote:
> On 2013/02/22 23:39:47, jar wrote:
> > aklin: Why do we increment count, when we *don't* run??
> 
> In an effort to speed the process... i'll take a stab at an answer.
> 
>   Closure empty;
>   empty.Run(); // This will crash
> 
> For the sake of convenience some of the test fixtures post wrapped empty/null
> closures. For example, in TYPED_TEST_P(TaskRunnerTest, Basic), a bunch of
empty
> wrapped tasks are posted.
> 
>   const Closure& ith_task = this->task_tracker_->WrapTask(Closure(), i);
> 
> What's being counted is how many tasks the SequencnedWorkerPool processes, SWP
> is calling TaskTracker::RunTask, internally that method may or may not call
the
> wrapped 'task' depending on how it was composed.

Yes, this is correct.  Perhaps add a comment explaining this?

https://codereview.chromium.org/11649032/diff/163001/base/test/sequenced_task...
File base/test/sequenced_task_runner_test_template.h (right):

https://codereview.chromium.org/11649032/diff/163001/base/test/sequenced_task...
base/test/sequenced_task_runner_test_template.h:65: void
WaitForCountTasksToComplete(int count);
There's a bit of an ambiguity here -- do you mean wait for a total of |count|
tasks to complete, or |count| additional tasks? I'm guessing the former, since
the latter seems racy (without another variable).

If so, perhaps a better name would be:

WaitForCompletedTasks(count);

or something that removes the ambiguity?

Also, should probably add a comment either way.

https://codereview.chromium.org/11649032/diff/163001/base/test/sequenced_task...
base/test/sequenced_task_runner_test_template.h:95: // The number of task ends
we've received.
task ends -> task end events

https://codereview.chromium.org/11649032/diff/163001/base/test/sequenced_task...
base/test/sequenced_task_runner_test_template.h:255:
this->task_tracker_->WaitForCountTasksToComplete(1);
use kTaskCount here, too

https://codereview.chromium.org/11649032/diff/163001/base/test/task_runner_te...
File base/test/task_runner_test_template.h (right):

https://codereview.chromium.org/11649032/diff/163001/base/test/task_runner_te...
base/test/task_runner_test_template.h:28: //     // this is called are run.
can you update this comment to mention the new behavior with delayed tasks?

https://codereview.chromium.org/11649032/diff/163001/base/test/task_runner_te...
base/test/task_runner_test_template.h:85: void WaitForCountTasksToComplete(int
count);
similar comments as in sequenced_task_runner_template.{h,cc}

https://codereview.chromium.org/11649032/diff/163001/base/threading/sequenced...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/11649032/diff/163001/base/threading/sequenced...
base/threading/sequenced_worker_pool.cc:284: void CleanupForTesting();
any reason for this rename? perhaps make it consistent with the outer method
name (in a follow up CL perhaps)

https://codereview.chromium.org/11649032/diff/163001/base/threading/sequenced...
base/threading/sequenced_worker_pool.cc:802: if (cleanup_state_ ==
CLEANUP_REQUESTED) {
sad that cleanup handling became this complex, but if it's needed it's needed.
:-(

https://codereview.chromium.org/11649032/diff/163001/base/threading/sequenced...
File base/threading/sequenced_worker_pool.h (right):

https://codereview.chromium.org/11649032/diff/163001/base/threading/sequenced...
base/threading/sequenced_worker_pool.h:291: // This will not flush delayed
tasks, delayed tasks get deleted.
semi-colon instead of comma

[michaeln, 2013-02-26 00:33:00]

https://codereview.chromium.org/11649032/diff/163001/base/threading/sequenced...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/11649032/diff/163001/base/threading/sequenced...
base/threading/sequenced_worker_pool.cc:802: if (cleanup_state_ ==
CLEANUP_REQUESTED) {
On 2013/02/26 00:12:23, akalin wrote:
> sad that cleanup handling became this complex, but if it's needed it's needed.
> :-(

I know... as mentioned a while ago... in some sense having this special cleanup
mode is an overall simplification (serialize cleanup and nuke it all), but the
details of entering and exiting that mode are not so simple.

[jam, 2013-02-26 02:22:41]

On 2013/02/25 22:59:55, sky wrote:
> I'm not a fan of the ForTesting style. I prefer a friend class. I'm leaving
this
> to John for his thoughts.

I don't feel strongly here. Given that g_globals is a static variable, using a
friend would mean moving it to the header right? seems preferable not to do
that?

[sky, 2013-02-26 03:24:42]

On Mon, Feb 25, 2013 at 6:22 PM,  <jam@chromium.org> wrote:
> On 2013/02/25 22:59:55, sky wrote:
>>
>> I'm not a fan of the ForTesting style. I prefer a friend class. I'm
>> leaving
>
> this
>>
>> to John for his thoughts.
>
>
> I don't feel strongly here. Given that g_globals is a static variable, using
> a
> friend would mean moving it to the header right? seems preferable not to do
> that?

Not necessarily. Could still expose a private method that does this.

  -Scott
>
> https://codereview.chromium.org/11649032/

[michaeln, 2013-02-26 20:37:59]

> Not necessarily. Could still expose a private method that does this.

I'll upload a new CL with a cut at friendifying the ForTesting method, akalin
spotted a couple of things to poke at too.

[michaeln, 2013-02-27 00:57:11]

ptal... friendified the ForTesting method and...

https://codereview.chromium.org/11649032/diff/163001/base/test/sequenced_task...
File base/test/sequenced_task_runner_test_template.h (right):

https://codereview.chromium.org/11649032/diff/163001/base/test/sequenced_task...
base/test/sequenced_task_runner_test_template.h:65: void
WaitForCountTasksToComplete(int count);
On 2013/02/26 00:12:23, akalin wrote:
> There's a bit of an ambiguity here -- do you mean wait for a total of |count|
> tasks to complete, or |count| additional tasks? I'm guessing the former, since
> the latter seems racy (without another variable).
> 
> If so, perhaps a better name would be:
> 
> WaitForCompletedTasks(count);
> 
> or something that removes the ambiguity?
> 
> Also, should probably add a comment either way.

Done.

https://codereview.chromium.org/11649032/diff/163001/base/test/sequenced_task...
base/test/sequenced_task_runner_test_template.h:95: // The number of task ends
we've received.
On 2013/02/26 00:12:23, akalin wrote:
> task ends -> task end events

Done.

https://codereview.chromium.org/11649032/diff/163001/base/test/sequenced_task...
base/test/sequenced_task_runner_test_template.h:255:
this->task_tracker_->WaitForCountTasksToComplete(1);
On 2013/02/26 00:12:23, akalin wrote:
> use kTaskCount here, too

Done.

https://codereview.chromium.org/11649032/diff/163001/base/test/task_runner_te...
File base/test/task_runner_test_template.h (right):

https://codereview.chromium.org/11649032/diff/163001/base/test/task_runner_te...
base/test/task_runner_test_template.h:28: //     // this is called are run.
On 2013/02/26 00:12:23, akalin wrote:
> can you update this comment to mention the new behavior with delayed tasks?

Done.

https://codereview.chromium.org/11649032/diff/163001/base/test/task_runner_te...
base/test/task_runner_test_template.h:85: void WaitForCountTasksToComplete(int
count);
On 2013/02/26 00:12:23, akalin wrote:
> similar comments as in sequenced_task_runner_template.{h,cc}

Done.

https://codereview.chromium.org/11649032/diff/163001/base/threading/sequenced...
File base/threading/sequenced_worker_pool.h (right):

https://codereview.chromium.org/11649032/diff/163001/base/threading/sequenced...
base/threading/sequenced_worker_pool.h:291: // This will not flush delayed
tasks, delayed tasks get deleted.
On 2013/02/26 00:12:23, akalin wrote:
> semi-colon instead of comma

Done.

[michaeln, 2013-02-27 22:52:12]

@sky or jam, ping for OWNERS

[michaeln, 2013-03-01 22:03:36]

ok... ping, with a difference... committing on monday unless somebody
articulates good reason not to

[michaeln, 2013-03-07 01:35:41]

Committed patchset #30 manually as r186578.