Plug some of the leaks in dbus_unittests.

Plug some of the leaks in dbus_unittests.

Also, remove the deprecated base::MessageLoop::Run() calls
everywhere in dbus unittests.

BUG=365669
R=armansito@chromium.org

Committed: https://crrev.com/9562caf1ec60d2e6f1c4c64b84ffddcd5a0ca029
Cr-Commit-Position: refs/heads/master@{#293113}

[armansito, 2014-08-29 19:59:21]

armansito@chromium.org changed reviewers:
+ keybuk@chromium.org, satorux@chromium.org

[armansito, 2014-08-29 19:59:21]

Might as well get rid of other uses of MessageLoop::Run in the rest of the dbus
unittests as well.

[earthdok, 2014-08-29 20:16:15]

On 2014/08/29 19:59:21, armansito wrote:
> Might as well get rid of other uses of MessageLoop::Run in the rest of the
dbus
> unittests as well.

Oops! I've replaced Run() calls with RunUntilIdle() calls. This is not correct.
Fix on the way.

[earthdok, 2014-08-29 20:22:27]

On 2014/08/29 19:59:21, armansito wrote:
> Might as well get rid of other uses of MessageLoop::Run in the rest of the
dbus
> unittests as well.

Done

[armansito, 2014-08-29 20:32:38]

On 2014/08/29 20:22:27, earthdok wrote:
> On 2014/08/29 19:59:21, armansito wrote:
> > Might as well get rid of other uses of MessageLoop::Run in the rest of the
> dbus
> > unittests as well.
> 
> Done

The pattern that the tests follow here is to call Run and then Quit when an
async operation has completed. Should calls to MessageLoop::Quit also be changed
to the RunLoop equivalent?

[earthdok, 2014-09-01 13:39:21]

On 2014/08/29 20:32:38, armansito wrote:
> On 2014/08/29 20:22:27, earthdok wrote:
> > On 2014/08/29 19:59:21, armansito wrote:
> > > Might as well get rid of other uses of MessageLoop::Run in the rest of the
> > dbus
> > > unittests as well.
> > 
> > Done
> 
> The pattern that the tests follow here is to call Run and then Quit when an
> async operation has completed. Should calls to MessageLoop::Quit also be
changed
> to the RunLoop equivalent?

They probably should, but I'm not sure what the RunLoop equivalent is. It's not
as simple as calling base::RunLoop().Quit(), and I don't really have a solid
understanding of that stuff. I think we need to have a top-level RunLoop in the
test fixture, but also create nested RunLoops in some places. Maybe someone who
understands the code better should take a look?

[armansito, 2014-09-01 13:58:00]

On 2014/09/01 13:39:21, earthdok wrote:
> On 2014/08/29 20:32:38, armansito wrote:
> > On 2014/08/29 20:22:27, earthdok wrote:
> > > On 2014/08/29 19:59:21, armansito wrote:
> > > > Might as well get rid of other uses of MessageLoop::Run in the rest of
the
> > > dbus
> > > > unittests as well.
> > > 
> > > Done
> > 
> > The pattern that the tests follow here is to call Run and then Quit when an
> > async operation has completed. Should calls to MessageLoop::Quit also be
> changed
> > to the RunLoop equivalent?
> 
> They probably should, but I'm not sure what the RunLoop equivalent is. It's
not
> as simple as calling base::RunLoop().Quit(), and I don't really have a solid
> understanding of that stuff. I think we need to have a top-level RunLoop in
the
> test fixture, but also create nested RunLoops in some places. Maybe someone
who
> understands the code better should take a look?

I think base::RunLoop just calls MessageLoop::Run and MessageLoop::Stop on the
current message loop (at the time it gets initialized), though I'm not very
familiar with it either. (I assumed you were, since you were the one who went
and replaced some of the deprecated usage in the first place :) )

Anyway, let's just fix the leak and not bother with the RunLoop stuff in this CL
then, which is what this CL was initially for. Just call MessageLoop::Run
directly for now, we can go about removing deprecated code later if that ever
becomes a priority.

Also, the dbus OWNERS should also take a look.

[satorux1, 2014-09-02 03:26:59]

satorux@chromium.org changed reviewers:
+ hashimoto@chromium.org

[satorux1, 2014-09-02 03:27:00]

+hashimoto@ could you take a look? I thought you had good understanding of the
RunLoop stuff.

[hashimoto, 2014-09-02 03:34:02]

https://codereview.chromium.org/523623003/diff/40001/dbus/end_to_end_async_un...
File dbus/end_to_end_async_unittest.cc (right):

https://codereview.chromium.org/523623003/diff/40001/dbus/end_to_end_async_un...
dbus/end_to_end_async_unittest.cc:192: message_loop_.Quit();
Why don't you replace this MessageLoop::Quit call with RunLoop::Quit?
You can make base::RunLoop a member of the test fixture, or pass a QuitClosure
to this place to quit the run loop.

The same goes for other places in this file and other files.

[earthdok, 2014-09-02 13:45:19]

On 2014/09/02 03:34:02, hashimoto wrote:
>
https://codereview.chromium.org/523623003/diff/40001/dbus/end_to_end_async_un...
> File dbus/end_to_end_async_unittest.cc (right):
> 
>
https://codereview.chromium.org/523623003/diff/40001/dbus/end_to_end_async_un...
> dbus/end_to_end_async_unittest.cc:192: message_loop_.Quit();
> Why don't you replace this MessageLoop::Quit call with RunLoop::Quit?
> You can make base::RunLoop a member of the test fixture, or pass a QuitClosure
> to this place to quit the run loop.
> 
> The same goes for other places in this file and other files.

PTAL (PS#4)

Passing a QuitClosure as a parameter seems cleaner compared to using a "global"
RunLoop instance, but looks like it would require considerable refactoring,
especially with idioms such as WaitForTestSignal().

[hashimoto, 2014-09-03 05:27:07]

lgtm, thanks

satorux@, could you approve this change as an owner of dbus/?

[satorux1, 2014-09-03 05:32:09]

LGTM. Thank you for fixing the leaks!

[earthdok, 2014-09-03 09:34:28]

The CQ bit was checked by earthdok@chromium.org

[commit-bot: I haz the power, 2014-09-03 09:36:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/earthdok@chromium.org/523623003/80001

[commit-bot: I haz the power, 2014-09-03 10:33:41]

Committed patchset #4 (id:80001) as 1b697373391ba705e5393d58c222bf3b8ceff5bf

[commit-bot: I haz the power, 2014-09-10 03:24:56]

Patchset 4 (id:??) landed as
https://crrev.com/9562caf1ec60d2e6f1c4c64b84ffddcd5a0ca029
Cr-Commit-Position: refs/heads/master@{#293113}