Fix an AppCacheStorageImpl leak in SiteInstanceTest.SiteInstanceDestructor by running the message l…

Fix an AppCacheStorageImpl leak in SiteInstanceTest.SiteInstanceDestructor by running the message loops some more.

BUG=143565
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=155233

[Lei Zhang, 2012-08-31 00:15:48]

I think some of the scoped variables don't get destroyed until the end of the
test after the existing call to DrainMessageLoops(). Thus to clean up the tasks
posted during their destruction, there needs to be an addition call to
DrainMessageLoops() in TearDown().

[awong, 2012-09-05 21:30:08]

https://chromiumcodereview.appspot.com/10908027/diff/1/content/browser/site_i...
File content/browser/site_instance_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/10908027/diff/1/content/browser/site_i...
content/browser/site_instance_impl_unittest.cc:141: DrainMessageLoops();
This previously wasn't in TearDown() because the Browser itself was dead by this
point which meant we'd likely have a use-after-free.  Is there a particular test
that appcache is still active for?

[Lei Zhang, 2012-09-06 00:29:43]

https://chromiumcodereview.appspot.com/10908027/diff/1/content/browser/site_i...
File content/browser/site_instance_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/10908027/diff/1/content/browser/site_i...
content/browser/site_instance_impl_unittest.cc:141: DrainMessageLoops();
On 2012/09/05 21:30:09, awong wrote:
> This previously wasn't in TearDown() because the Browser itself was dead by
this
> point which meant we'd likely have a use-after-free.  Is there a particular
test
> that appcache is still active for?

I can tell you with this cl, there are no valgrind errors in these tests. I will
figure out which SiteInstanceTest triggers this leak.

[Lei Zhang, 2012-09-06 00:57:15]

On 2012/09/06 00:29:43, Lei Zhang wrote:
>
https://chromiumcodereview.appspot.com/10908027/diff/1/content/browser/site_i...
> File content/browser/site_instance_impl_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10908027/diff/1/content/browser/site_i...
> content/browser/site_instance_impl_unittest.cc:141: DrainMessageLoops();
> On 2012/09/05 21:30:09, awong wrote:
> > This previously wasn't in TearDown() because the Browser itself was dead by
> this
> > point which meant we'd likely have a use-after-free.  Is there a particular
> test
> > that appcache is still active for?
> 
> I can tell you with this cl, there are no valgrind errors in these tests. I
will
> figure out which SiteInstanceTest triggers this leak.

The test cases it fails in are GetProcess, HasWrongProcessForURL, 
OneSiteInstancePerSite, OneSiteInstancePerSiteInBrowserContext, and
SiteInstanceDestructor.

So it looks like if you call DrainMessageLoop() at the end of the test, calling
it again in TearDown() doesn't hurt.

[awong, 2012-09-06 00:59:11]

LGTM w/ nit

On 2012/09/06 00:57:15, Lei Zhang wrote:
> On 2012/09/06 00:29:43, Lei Zhang wrote:
> >
>
https://chromiumcodereview.appspot.com/10908027/diff/1/content/browser/site_i...
> > File content/browser/site_instance_impl_unittest.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10908027/diff/1/content/browser/site_i...
> > content/browser/site_instance_impl_unittest.cc:141: DrainMessageLoops();
> > On 2012/09/05 21:30:09, awong wrote:
> > > This previously wasn't in TearDown() because the Browser itself was dead
by
> > this
> > > point which meant we'd likely have a use-after-free.  Is there a
particular
> > test
> > > that appcache is still active for?
> > 
> > I can tell you with this cl, there are no valgrind errors in these tests. I
> will
> > figure out which SiteInstanceTest triggers this leak.
> 
> The test cases it fails in are GetProcess, HasWrongProcessForURL, 
> OneSiteInstancePerSite, OneSiteInstancePerSiteInBrowserContext, and
> SiteInstanceDestructor.
> 
> So it looks like if you call DrainMessageLoop() at the end of the test,
calling
> it again in TearDown() doesn't hurt.


That makes no sense to me, but okay.  Can you add a TODO about the call in the
teardown with this bug number and then assign it to me after landing this patch?
 I want to figure out why this is happening.

[Lei Zhang, 2012-09-06 01:17:47]

On 2012/09/06 00:59:11, awong wrote:
> That makes no sense to me, but okay.  Can you add a TODO about the call in the
> teardown with this bug number and then assign it to me after landing this
patch?
>  I want to figure out why this is happening.

So take SiteInstanceDestructor as an example. It creates a WebContents, and that
eventually calls into StoragePartitionImplMap::PostCreateInitialization(), which
posts a task to the IO thread since the test has one. However, since the message
loop is not running, when you call DrainMessageLoop(), that's when it actually
runs and creates a AppCacheDatabase. Next, the AppCacheDatabase doesn't get
deleted until the next time you run the message loop, which I do in TearDown().

[awong, 2012-09-06 01:22:29]

On 2012/09/06 01:17:47, Lei Zhang wrote:
> On 2012/09/06 00:59:11, awong wrote:
> > That makes no sense to me, but okay.  Can you add a TODO about the call in
the
> > teardown with this bug number and then assign it to me after landing this
> patch?
> >  I want to figure out why this is happening.
> 
> So take SiteInstanceDestructor as an example. It creates a WebContents, and
that
> eventually calls into StoragePartitionImplMap::PostCreateInitialization(),
which
> posts a task to the IO thread since the test has one. However, since the
message
> loop is not running, when you call DrainMessageLoop(), that's when it actually
> runs and creates a AppCacheDatabase. Next, the AppCacheDatabase doesn't get
> deleted until the next time you run the message loop, which I do in
TearDown().

I see.  That's really annoying. :(  Thanks for explaining. Can you put that
explanation into a comment inside Teardown?

[Lei Zhang, 2012-09-06 01:31:05]

On 2012/09/06 01:22:29, awong wrote:
> I see.  That's really annoying. :(  Thanks for explaining. Can you put that
> explanation into a comment inside Teardown?

Done, see patch set 2.

[awong, 2012-09-06 01:33:00]

LGTM

Thanks for fixing!

[commit-bot: I haz the power, 2012-09-06 02:56:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/10908027/10001

[commit-bot: I haz the power, 2012-09-06 02:56:53]

Presubmit check for 10908027-10001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser

[Lei Zhang, 2012-09-06 03:04:51]

+jam for OWNERS.

[jam, 2012-09-06 16:58:03]

lgtm

https://codereview.chromium.org/10908027/diff/10001/content/browser/site_inst...
File content/browser/site_instance_impl_unittest.cc (right):

https://codereview.chromium.org/10908027/diff/10001/content/browser/site_inst...
content/browser/site_instance_impl_unittest.cc:5: #include <string>
including string is quite unnecessary.. i would say the same for vector. i
understand about iwyu, but there are some very basic headers that are assumed to
be included (i.e. just like basictypes.h or build_config.h, which aren't
included here)

[Lei Zhang, 2012-09-06 18:16:43]

https://codereview.chromium.org/10908027/diff/10001/content/browser/site_inst...
File content/browser/site_instance_impl_unittest.cc (right):

https://codereview.chromium.org/10908027/diff/10001/content/browser/site_inst...
content/browser/site_instance_impl_unittest.cc:5: #include <string>
On 2012/09/06 16:58:03, John Abd-El-Malek wrote:
> including string is quite unnecessary.. i would say the same for vector. i
> understand about iwyu, but there are some very basic headers that are assumed
to
> be included (i.e. just like basictypes.h or build_config.h, which aren't
> included here)

Ok, maybe we should take it out of the linter...

[commit-bot: I haz the power, 2012-09-06 18:17:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/10908027/9002

[commit-bot: I haz the power, 2012-09-06 20:27:48]

Try job failure for 10908027-9002 (retry) on win_rel for step "runhooks".
It's a second try, previously, step "sync_integration_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...