Implement timeouts for IQ requests.

remoting/jingle_glue/iq_sender.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "third_party/libjingle/source/talk/xmpp/constants.h"
 
 namespace remoting {
 
 // static
 scoped_ptr<buzz::XmlElement> IqSender::MakeIqStanza(
     const std::string& type,
@@ skipping 11 matching lines @@
     : signal_strategy_(signal_strategy) {
   signal_strategy_->AddListener(this);
 }
 
 IqSender::~IqSender() {
   signal_strategy_->RemoveListener(this);
 }
 
 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();
 }
 
 scoped_ptr<IqRequest> IqSender::SendIq(const std::string& type,
                                        const std::string& addressee,
                                        scoped_ptr<buzz::XmlElement> iq_body,
                                        const ReplyCallback& callback) {
   return SendIq(MakeIqStanza(type, addressee, iq_body.Pass()), callback);
@@ skipping 29 matching lines @@
   if (type != "result" && type != "error") {
     return false;
   }
 
   const std::string& id = stanza->Attr(buzz::QN_ID);
   if (id.empty()) {
     LOG(WARNING) << "IQ packet missing id " << stanza->Str();
     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;
+
+  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);
 
-  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