Handle IQ responses asynchronously.

Previously IqRequest was calling response callback synchronously. The callback
is allowed to delete SignalStrategy object, but it may be unsafe to delete
XmppSignalStrategy when handling an incoming message. Fixed IqRequest to
invoke response callback asynchronously, so that the callback is called with
a clean stack.

BUG=124430

Review URL: https://chromiumcodereview.appspot.com/10161010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@133302 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 28 insertions, 5 deletions

diff --git a/remoting/jingle_glue/iq_sender.cc b/remoting/jingle_glue/iq_sender.cc
index 6bc7810..797db2a 100644
--- a/remoting/jingle_glue/iq_sender.cc
+++ b/remoting/jingle_glue/iq_sender.cc
@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/message_loop_proxy.h"
 #include "base/string_number_conversions.h"
 #include "base/time.h"
@@ -162,7 +163,16 @@ void IqRequest::OnTimeout() {
 }
 
 void IqRequest::OnResponse(const buzz::XmlElement* stanza) {
-  CallCallback(stanza);
+  // It's unsafe to delete signal strategy here, and the callback may
+  // want to do that, so we post task to invoke the callback later.
+  scoped_ptr<buzz::XmlElement> stanza_copy(new buzz::XmlElement(*stanza));
+  base::MessageLoopProxy::current()->PostTask(
+      FROM_HERE, base::Bind(&IqRequest::DeliverResponse, AsWeakPtr(),
+                            base::Passed(&stanza_copy)));
+}
+
+void IqRequest::DeliverResponse(scoped_ptr<buzz::XmlElement> stanza) {
+  CallCallback(stanza.get());
 }
 
 }  // namespace remoting
diff --git a/remoting/jingle_glue/iq_sender.h b/remoting/jingle_glue/iq_sender.h
index e90e766..ce333e5 100644
--- a/remoting/jingle_glue/iq_sender.h
+++ b/remoting/jingle_glue/iq_sender.h
@@ -99,6 +99,8 @@ class IqRequest : public  base::SupportsWeakPtr<IqRequest> {
   // Called by IqSender when a response is received.
   void OnResponse(const buzz::XmlElement* stanza);
 
+  void DeliverResponse(scoped_ptr<buzz::XmlElement> stanza);
+
   IqSender* sender_;
   IqSender::ReplyCallback callback_;
   std::string addressee_;
diff --git a/remoting/jingle_glue/iq_sender_unittest.cc b/remoting/jingle_glue/iq_sender_unittest.cc
index 06730f5..a5f02a5 100644
--- a/remoting/jingle_glue/iq_sender_unittest.cc
+++ b/remoting/jingle_glue/iq_sender_unittest.cc
@@ -39,6 +39,10 @@ class MockCallback {
   MOCK_METHOD2(OnReply, void(IqRequest* request, const XmlElement* reply));
 };
 
+MATCHER_P(XmlEq, expected, "") {
+  return arg->Str() == expected->Str();
+}
+
 }  // namespace
 
 class IqSenderTest : public testing::Test {
@@ -95,8 +99,10 @@ TEST_F(IqSenderTest, SendIq) {
       QName("test:namespace", "response-body"));
   response->AddElement(result);
 
-  EXPECT_CALL(callback_, OnReply(request_.get(), response.get()));
   EXPECT_TRUE(sender_->OnSignalStrategyIncomingStanza(response.get()));
+
+  EXPECT_CALL(callback_, OnReply(request_.get(), XmlEq(response.get())));
+  message_loop_.RunAllPending();
 }
 
 TEST_F(IqSenderTest, Timeout) {
@@ -125,9 +131,10 @@ TEST_F(IqSenderTest, InvalidFrom) {
       QName("test:namespace", "response-body"));
   response->AddElement(result);
 
-  EXPECT_CALL(callback_, OnReply(request_.get(), response.get()))
+  EXPECT_CALL(callback_, OnReply(_, _))
       .Times(0);
   EXPECT_FALSE(sender_->OnSignalStrategyIncomingStanza(response.get()));
+  message_loop_.RunAllPending();
 }
 
 TEST_F(IqSenderTest, IdMatchingHack) {
@@ -144,8 +151,10 @@ TEST_F(IqSenderTest, IdMatchingHack) {
       QName("test:namespace", "response-body"));
   response->AddElement(result);
 
-  EXPECT_CALL(callback_, OnReply(request_.get(), response.get()));
   EXPECT_TRUE(sender_->OnSignalStrategyIncomingStanza(response.get()));
+
+  EXPECT_CALL(callback_, OnReply(request_.get(), XmlEq(response.get())));
+  message_loop_.RunAllPending();
 }
 
 }  // namespace remoting
diff --git a/remoting/jingle_glue/signal_strategy.h b/remoting/jingle_glue/signal_strategy.h
index 0864cee..8971267 100644
--- a/remoting/jingle_glue/signal_strategy.h
+++ b/remoting/jingle_glue/signal_strategy.h
@@ -40,7 +40,9 @@ class SignalStrategy {
     // Called after state of the connection has changed.
     virtual void OnSignalStrategyStateChange(State state) {}
 
-    // Must return true if the stanza was handled, false otherwise.
+    // Must return true if the stanza was handled, false
+    // otherwise. The signal strategy must not be deleted from a
+    // handler of this message.
     virtual bool OnSignalStrategyIncomingStanza(
         const buzz::XmlElement* stanza) { return false; }
   };