Plug the leak in SingleLoginAttemptTest.* (jingle_unittests).

Plug the leak in SingleLoginAttemptTest.* (jingle_unittests).

Flushing the message loop after destroying the SingleLoginAttempt makes the
test shut down cleanly. Don't know why but it fixes the leak.

BUG=259350
R=ajwong@chromium.org

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

[earthdok, 2013-07-29 18:50:28]

please take a look
picking a random owner since Fred looks busy

[awong, 2013-07-29 20:59:59]

Can you add a little more info into the description for why this plugs the hole?
 Just 1-2 sentences is probably enough.

https://codereview.chromium.org/21121003/diff/1/jingle/notifier/communicator/...
File jingle/notifier/communicator/single_login_attempt_unittest.cc (right):

https://codereview.chromium.org/21121003/diff/1/jingle/notifier/communicator/...
jingle/notifier/communicator/single_login_attempt_unittest.cc:109:
attempt_.reset(NULL);
Don't need the explicit NULL.

[earthdok, 2013-07-29 23:59:32]

On 2013/07/29 20:59:59, awong wrote:
> Can you add a little more info into the description for why this plugs the
hole?
>  Just 1-2 sentences is probably enough.

I don't really know. It just looked like something that could work so I tried
it. I haven't looked deeper into the code.

[earthdok, 2013-07-30 00:02:36]

On 2013/07/29 23:59:32, earthdok wrote:
> On 2013/07/29 20:59:59, awong wrote:
> > Can you add a little more info into the description for why this plugs the
> hole?
> >  Just 1-2 sentences is probably enough.
> 
> I don't really know. It just looked like something that could work so I tried
> it. I haven't looked deeper into the code.

Will this do?

[awong, 2013-07-30 00:04:44]

On 2013/07/30 00:02:36, earthdok wrote:
> On 2013/07/29 23:59:32, earthdok wrote:
> > On 2013/07/29 20:59:59, awong wrote:
> > > Can you add a little more info into the description for why this plugs the
> > hole?
> > >  Just 1-2 sentences is probably enough.
> > 
> > I don't really know. It just looked like something that could work so I
tried
> > it. I haven't looked deeper into the code.
> 
> Will this do?

Sure, though I would phrase it with less certainty.  Something like

"Flushing the message loop after destroying the SingleLoginAttempt makes the
test shut down cleanly. Don't know why but it fixes the leak"

The main goal is to indicate to someone digging through revision history that we
don't quite know why this change worked.  I also had one comment about removing
the NULL in the reset().

[earthdok, 2013-07-30 00:08:12]

On 2013/07/30 00:04:44, awong wrote:
> On 2013/07/30 00:02:36, earthdok wrote:
> > On 2013/07/29 23:59:32, earthdok wrote:
> > > On 2013/07/29 20:59:59, awong wrote:
> > > > Can you add a little more info into the description for why this plugs
the
> > > hole?
> > > >  Just 1-2 sentences is probably enough.
> > > 
> > > I don't really know. It just looked like something that could work so I
> tried
> > > it. I haven't looked deeper into the code.
> > 
> > Will this do?
> 
> Sure, though I would phrase it with less certainty.  Something like
> 
> "Flushing the message loop after destroying the SingleLoginAttempt makes the
> test shut down cleanly. Don't know why but it fixes the leak"
> 
> The main goal is to indicate to someone digging through revision history that
we
> don't quite know why this change worked.  I also had one comment about
removing
> the NULL in the reset().

ok, addressed both comments

[awong, 2013-07-30 00:10:04]

LGTM

[commit-bot: I haz the power, 2013-07-30 08:52:23]

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

[earthdok, 2013-07-30 16:51:14]

Committed patchset #4 manually as r214368 (presubmit successful).

[commit-bot: I haz the power, 2013-07-30 19:27:41]

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

[commit-bot: I haz the power, 2013-07-30 19:27:45]

Failed to apply patch for
jingle/notifier/communicator/single_login_attempt_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file jingle/notifier/communicator/single_login_attempt_unittest.cc
  Hunk #1 FAILED at 95.
  Hunk #2 FAILED at 110.
  Hunk #3 FAILED at 118.
  Hunk #4 FAILED at 129.
  Hunk #5 FAILED at 137.
  Hunk #6 FAILED at 245.
  6 out of 6 hunks FAILED -- saving rejects to file
jingle/notifier/communicator/single_login_attempt_unittest.cc.rej

Patch:       jingle/notifier/communicator/single_login_attempt_unittest.cc
Index: jingle/notifier/communicator/single_login_attempt_unittest.cc
diff --git a/jingle/notifier/communicator/single_login_attempt_unittest.cc
b/jingle/notifier/communicator/single_login_attempt_unittest.cc
index
c00df3d3a4e96b048f32eac0ec3d018d0791426c..0e80fd0dfe38723e294230fc5b7c00ea004867fb
100644
--- a/jingle/notifier/communicator/single_login_attempt_unittest.cc
+++ b/jingle/notifier/communicator/single_login_attempt_unittest.cc
@@ -95,14 +95,19 @@ class SingleLoginAttemptTest : public ::testing::Test {
                   net::HostPortPair("example.com", 100), SUPPORTS_SSLTCP)),
           false /* try_ssltcp_first */,
           "auth_mechanism"),
-        attempt_(login_settings_, &fake_delegate_) {}
+        attempt_(new SingleLoginAttempt(login_settings_, &fake_delegate_)) {}
 
   virtual void TearDown() OVERRIDE {
     message_loop_.RunUntilIdle();
   }
 
   void FireRedirect(buzz::XmlElement* redirect_error) {
-    attempt_.OnError(buzz::XmppEngine::ERROR_STREAM, 0, redirect_error);
+    attempt_->OnError(buzz::XmppEngine::ERROR_STREAM, 0, redirect_error);
+  }
+
+  ~SingleLoginAttemptTest() {
+    attempt_.reset();
+    message_loop_.RunUntilIdle();
   }
 
  private:
@@ -110,7 +115,7 @@ class SingleLoginAttemptTest : public ::testing::Test {
   const LoginSettings login_settings_;
 
  protected:
-  SingleLoginAttempt attempt_;
+  scoped_ptr<SingleLoginAttempt> attempt_;
   FakeDelegate fake_delegate_;
   FakeBaseTask fake_base_task_;
 };
@@ -118,7 +123,7 @@ class SingleLoginAttemptTest : public ::testing::Test {
 // Fire OnConnect and make sure the base task gets passed to the
 // delegate properly.
 TEST_F(SingleLoginAttemptTest, Basic) {
-  attempt_.OnConnect(fake_base_task_.AsWeakPtr());
+  attempt_->OnConnect(fake_base_task_.AsWeakPtr());
   EXPECT_EQ(CONNECTED, fake_delegate_.state());
   EXPECT_EQ(fake_base_task_.AsWeakPtr().get(),
             fake_delegate_.base_task().get());
@@ -129,7 +134,7 @@ TEST_F(SingleLoginAttemptTest, Basic) {
 TEST_F(SingleLoginAttemptTest, Error) {
   for (int i = 0; i < 2; ++i) {
     EXPECT_EQ(IDLE, fake_delegate_.state());
-    attempt_.OnError(buzz::XmppEngine::ERROR_NONE, 0, NULL);
+    attempt_->OnError(buzz::XmppEngine::ERROR_NONE, 0, NULL);
   }
   EXPECT_EQ(SETTINGS_EXHAUSTED, fake_delegate_.state());
 }
@@ -137,8 +142,8 @@ TEST_F(SingleLoginAttemptTest, Error) {
 // Fire OnErrors but replace the last one with OnConnect, and make
 // sure the delegate still gets the OnConnect message.
 TEST_F(SingleLoginAttemptTest, ErrorThenSuccess) {
-  attempt_.OnError(buzz::XmppEngine::ERROR_NONE, 0, NULL);
-  attempt_.OnConnect(fake_base_task_.AsWeakPtr());
+  attempt_->OnError(buzz::XmppEngine::ERROR_NONE, 0, NULL);
+  attempt_->OnConnect(fake_base_task_.AsWeakPtr());
   EXPECT_EQ(CONNECTED, fake_delegate_.state());
   EXPECT_EQ(fake_base_task_.AsWeakPtr().get(),
             fake_delegate_.base_task().get());
@@ -245,7 +250,7 @@ TEST_F(SingleLoginAttemptTest, RedirectMissingSeeOtherHost)
{
 // Fire 'Unauthorized' errors and make sure the delegate gets the
 // OnCredentialsRejected() event.
 TEST_F(SingleLoginAttemptTest, CredentialsRejected) {
-  attempt_.OnError(buzz::XmppEngine::ERROR_UNAUTHORIZED, 0, NULL);
+  attempt_->OnError(buzz::XmppEngine::ERROR_UNAUTHORIZED, 0, NULL);
   EXPECT_EQ(CREDENTIALS_REJECTED, fake_delegate_.state());
 }