Implement timeouts for IQ requests.

remoting/jingle_glue/iq_sender.cc

Index: remoting/jingle_glue/iq_sender.cc
diff --git a/remoting/jingle_glue/iq_sender.cc b/remoting/jingle_glue/iq_sender.cc
index eb8f17ab133846112cb5c91ccf8e82898d856d59..ca0b3f6960c1d54ba4c08ab282885df4edbffab7 100644
--- a/remoting/jingle_glue/iq_sender.cc
+++ b/remoting/jingle_glue/iq_sender.cc
@@ -4,7 +4,10 @@
 
 #include "remoting/jingle_glue/iq_sender.h"
 
+#include "base/bind.h"
+#include "base/location.h"
 #include "base/logging.h"
+#include "base/message_loop_proxy.h"
 #include "base/string_number_conversions.h"
 #include "remoting/jingle_glue/signal_strategy.h"
 #include "third_party/libjingle/source/talk/xmllite/xmlelement.h"
@@ -36,13 +39,14 @@ IqSender::~IqSender() {
 
 scoped_ptr<IqRequest> IqSender::SendIq(scoped_ptr<buzz::XmlElement> stanza,
                                        const ReplyCallback& callback) {
+  std::string addressee = stanza->Attr(buzz::QN_TO);
   std::string id = signal_strategy_->GetNextId();
   stanza->AddAttr(buzz::QN_ID, id);
   if (!signal_strategy_->SendStanza(stanza.Pass())) {
     return scoped_ptr<IqRequest>(NULL);
   }
   DCHECK(requests_.find(id) == requests_.end());
-  scoped_ptr<IqRequest> request(new IqRequest(this, callback));
+  scoped_ptr<IqRequest> request(new IqRequest(this, callback, addressee));
   if (!callback.is_null())
     requests_[id] = request.get();
   return request.Pass();
@@ -92,32 +96,72 @@ bool IqSender::OnSignalStrategyIncomingStanza(const buzz::XmlElement* stanza) {
     return false;
   }
 
+  std::string from = stanza->Attr(buzz::QN_FROM);
+
   IqRequestMap::iterator it = requests_.find(id);
+
+  // This is a hack to workaround the issue with the WCS changing IDs
+  // of IQ responses sent from client to host. Whenever we receive a
+  // stanza with an unknown ID we try to match it with an outstanding
+  // request sent to the same peer.
+  if (it == requests_.end()) {
+    for (it = requests_.begin(); it != requests_.end(); ++it) {
+      if (it->second->addressee_ == from) {
+        break;
+      }
+    }
+  }
+

[simonmorris, 2012/02/24 00:11:31]

This is fragile if the host can send multiple requests to the same client
without waiting for a response after each request: a response could then be
matched to the wrong request.

Could this ever happen? If so, do responses contain any attributes that could
link them to a specific request, and is it worth checking first for a strong
match, and then falling back to any weaker matches?

Even if not, I think this logic is worth unit testing.

[Sergey Ulanov, 2012/02/24 21:56:31]

I believe with the current code it will not happen because we generate IQ ids
sequentially and map<> use binary search trees (i.e. we iterate over the
requests in the same order in which they were sent), but it is obviously
fragile. In any case, the way we use responses it doesn't really matter if
response to request A is interpreted as if it was for B and response to request
B is interpreted as if it was for A. What matters is that number of response is
the same as number of requests.  Also we almost never should have more than one
outstanding request.

Normally result stanzas don't have any useful information inside. Error stanzas
do, but using that information properly would make this code significantly more
complicated and wouldn't solve the problem anyway.
Done

[simonmorris, 2012/02/24 22:32:51]

OK

   if (it == requests_.end()) {
     return false;
   }
 
   IqRequest* request = it->second;
-  requests_.erase(it);
 
+  if (request->addressee_ != from) {
+    LOG(ERROR) << "Received IQ response from from a invalid JID. Ignoring it."
+               << " Message received from: " << from
+               << " Original JID: " << request->addressee_;
+    return false;
+  }
+
+  requests_.erase(it);
   request->OnResponse(stanza);
+
   return true;
 }
 
-IqRequest::IqRequest(IqSender* sender, const IqSender::ReplyCallback& callback)
+IqRequest::IqRequest(IqSender* sender, const IqSender::ReplyCallback& callback,
+                     const std::string& addressee)
     : sender_(sender),
-      callback_(callback) {
+      callback_(callback),
+      addressee_(addressee) {
 }
 
 IqRequest::~IqRequest() {
   sender_->RemoveRequest(this);
 }
 
+void IqRequest::SetTimeout(base::TimeDelta timeout) {
+  base::MessageLoopProxy::current()->PostDelayedTask(
+      FROM_HERE, base::Bind(&IqRequest::OnTimeout, AsWeakPtr()),
+      timeout.InMilliseconds());
+}
+
+void IqRequest::CallCallback(const buzz::XmlElement* stanza) {
+  if (!callback_.is_null()) {
+    IqSender::ReplyCallback callback(callback_);
+    callback_.Reset();
+    callback.Run(this, stanza);
+  }
+}
+
+void IqRequest::OnTimeout() {
+  CallCallback(NULL);
+}
+
 void IqRequest::OnResponse(const buzz::XmlElement* stanza) {
-  DCHECK(!callback_.is_null());
-  IqSender::ReplyCallback callback(callback_);
-  callback_.Reset();
-  callback.Run(stanza);
+  CallCallback(stanza);
 }
 
 }  // namespace remoting