addresses akalin's comments on:

http://codereview.chromium.org/7273034

rolls cacheinvalidation forward to add message validation and better logging


Review URL: http://codereview.chromium.org/7324033

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

8 files changed, 87 insertions, 57 deletions

diff --git a/chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc b/chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc
index 4e62001..745eb48 100644
--- a/chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc
+++ b/chrome/browser/sync/notifier/cache_invalidation_packet_handler.cc
@@ -44,12 +44,13 @@ const buzz::QName kQnChannelContext("", "channelContext");
 class CacheInvalidationListenTask : public buzz::XmppTask {
  public:
   // Takes ownership of callback.
-  CacheInvalidationListenTask(Task* parent,
-                              std::string* channel_context,
-                              Callback1<const std::string&>::Type* callback)
+  CacheInvalidationListenTask(
+      Task* parent,
+      Callback1<const std::string&>::Type* callback,
+      Callback1<const std::string&>::Type* context_change_callback)
       : XmppTask(parent, buzz::XmppEngine::HL_TYPE),
-        channel_context_(channel_context),
-        callback_(callback) {}
+        callback_(callback),
+        context_change_callback_(context_change_callback) {}
   virtual ~CacheInvalidationListenTask() {}
 
   virtual int ProcessStart() {
@@ -79,10 +80,7 @@ class CacheInvalidationListenTask : public buzz::XmppTask {
 
   virtual bool HandleStanza(const buzz::XmlElement* stanza) {
     VLOG(1) << "Stanza received: "
-              << notifier::XmlElementToString(*stanza);
-    if (stanza->HasAttr(kQnChannelContext)) {
-      *channel_context_ = stanza->Attr(kQnChannelContext);
-    }
+            << notifier::XmlElementToString(*stanza);
     if (IsValidCacheInvalidationIqPacket(stanza)) {
       VLOG(2) << "Queueing stanza";
       QueueStanza(stanza);
@@ -100,7 +98,7 @@ class CacheInvalidationListenTask : public buzz::XmppTask {
   }
 
   bool GetCacheInvalidationIqPacketData(const buzz::XmlElement* stanza,
-                            std::string* data) {
+                                        std::string* data) {
     DCHECK(IsValidCacheInvalidationIqPacket(stanza));
     const buzz::XmlElement* cache_invalidation_iq_packet =
         stanza->FirstNamed(kQnData);
@@ -108,12 +106,19 @@ class CacheInvalidationListenTask : public buzz::XmppTask {
       LOG(ERROR) << "Could not find cache invalidation IQ packet element";
       return false;
     }
+    // Look for a channelContext attribute in the content of the stanza.  If
+    // present, remember it so it can be echoed back.
+    if (cache_invalidation_iq_packet->HasAttr(kQnChannelContext)) {
+      context_change_callback_->Run(
+          cache_invalidation_iq_packet->Attr(kQnChannelContext));
+    }
     *data = cache_invalidation_iq_packet->BodyText();
     return true;
   }
 
   std::string* channel_context_;
   scoped_ptr<Callback1<const std::string&>::Type> callback_;
+  scoped_ptr<Callback1<const std::string&>::Type> context_change_callback_;
   DISALLOW_COPY_AND_ASSIGN(CacheInvalidationListenTask);
 };
 
@@ -142,7 +147,7 @@ class CacheInvalidationSendMessageTask : public buzz::XmppTask {
         MakeCacheInvalidationIqPacket(to_jid_, task_id(), msg_,
                                       seq_, sid_, channel_context_));
     VLOG(1) << "Sending message: "
-              << notifier::XmlElementToString(*stanza.get());
+            << notifier::XmlElementToString(*stanza.get());
     if (SendStanza(stanza.get()) != buzz::XMPP_RETURN_OK) {
       VLOG(2) << "Error when sending message";
       return STATE_ERROR;
@@ -157,14 +162,14 @@ class CacheInvalidationSendMessageTask : public buzz::XmppTask {
       return STATE_BLOCKED;
     }
     VLOG(2) << "CacheInvalidationSendMessageTask response received: "
-              << notifier::XmlElementToString(*stanza);
+            << notifier::XmlElementToString(*stanza);
     // TODO(akalin): Handle errors here.
     return STATE_DONE;
   }
 
   virtual bool HandleStanza(const buzz::XmlElement* stanza) {
     VLOG(1) << "Stanza received: "
-              << notifier::XmlElementToString(*stanza);
+            << notifier::XmlElementToString(*stanza);
     if (!MatchResponseIq(stanza, to_jid_, task_id())) {
       VLOG(2) << "Stanza skipped";
       return false;
@@ -179,7 +184,7 @@ class CacheInvalidationSendMessageTask : public buzz::XmppTask {
       const buzz::Jid& to_jid,
       const std::string& task_id,
       const std::string& msg,
-      int seq, const std::string& sid, const std::string channel_context) {
+      int seq, const std::string& sid, const std::string& channel_context) {
     buzz::XmlElement* iq = MakeIq(buzz::STR_SET, to_jid, task_id);
     buzz::XmlElement* cache_invalidation_iq_packet =
         new buzz::XmlElement(kQnData, true);
@@ -201,7 +206,7 @@ class CacheInvalidationSendMessageTask : public buzz::XmppTask {
   std::string msg_;
   int seq_;
   std::string sid_;
-  std::string channel_context_;
+  const std::string channel_context_;
 
   DISALLOW_COPY_AND_ASSIGN(CacheInvalidationSendMessageTask);
 };
@@ -223,8 +228,10 @@ CacheInvalidationPacketHandler::CacheInvalidationPacketHandler(
   // Owned by base_task.  Takes ownership of the callback.
   CacheInvalidationListenTask* listen_task =
       new CacheInvalidationListenTask(
-          base_task_, &channel_context_, scoped_callback_factory_.NewCallback(
-              &CacheInvalidationPacketHandler::HandleInboundPacket));
+          base_task_, scoped_callback_factory_.NewCallback(
+              &CacheInvalidationPacketHandler::HandleInboundPacket),
+          scoped_callback_factory_.NewCallback(
+              &CacheInvalidationPacketHandler::HandleChannelContextChange));
   listen_task->Start();
 }
 
@@ -271,4 +278,10 @@ void CacheInvalidationPacketHandler::HandleInboundPacket(
   incoming_receiver_->Run(decoded_message);
 }
 
+void CacheInvalidationPacketHandler::HandleChannelContextChange(
+    const std::string& context) {
+  DCHECK(non_thread_safe_.CalledOnValidThread());
+  channel_context_ = context;
+}
+
 }  // namespace sync_notifier
diff --git a/chrome/browser/sync/notifier/cache_invalidation_packet_handler.h b/chrome/browser/sync/notifier/cache_invalidation_packet_handler.h
index e44fd0b..d85cfbd 100644
--- a/chrome/browser/sync/notifier/cache_invalidation_packet_handler.h
+++ b/chrome/browser/sync/notifier/cache_invalidation_packet_handler.h
@@ -49,6 +49,7 @@ class CacheInvalidationPacketHandler {
   FRIEND_TEST(CacheInvalidationPacketHandlerTest, Basic);
 
   void HandleInboundPacket(const std::string& packet);
+  void HandleChannelContextChange(const std::string& context);
 
   base::NonThreadSafe non_thread_safe_;
   base::ScopedCallbackFactory<CacheInvalidationPacketHandler>
diff --git a/chrome/browser/sync/notifier/chrome_system_resources.cc b/chrome/browser/sync/notifier/chrome_system_resources.cc
index 9d64f0d..0965c09 100644
--- a/chrome/browser/sync/notifier/chrome_system_resources.cc
+++ b/chrome/browser/sync/notifier/chrome_system_resources.cc
@@ -42,7 +42,7 @@ void ChromeLogger::Log(LogLevel level, const char* file, int line,
   // We treat LOG(INFO) as VLOG(1).
   if ((log_severity >= logging::GetMinLogLevel()) &&
       ((log_severity != logging::LOG_INFO) ||
-       (1 <= logging::GetVlogLevelHelper(file, ::strlen(file))))) {
+      (1 <= logging::GetVlogLevelHelper(file, ::strlen(file))))) {
     va_list ap;
     va_start(ap, format);
     std::string result;
@@ -94,11 +94,11 @@ void ChromeScheduler::Schedule(
       FROM_HERE, task_to_post, delay.InMillisecondsRoundedUp());
 }
 
-bool ChromeScheduler::IsRunningOnThread() {
+bool ChromeScheduler::IsRunningOnThread() const {
   return created_on_loop_ == MessageLoop::current();
 }
 
-invalidation::Time ChromeScheduler::GetCurrentTime() {
+invalidation::Time ChromeScheduler::GetCurrentTime() const {
   CHECK_EQ(created_on_loop_, MessageLoop::current());
   return base::Time::Now();
 }
@@ -187,7 +187,9 @@ void ChromeStorage::RunAndDeleteReadKeyCallback(
   delete callback;
 }
 
-ChromeNetwork::ChromeNetwork() : packet_handler_(NULL) {}
+ChromeNetwork::ChromeNetwork()
+    : packet_handler_(NULL),
+      scoped_callback_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {}
 
 ChromeNetwork::~ChromeNetwork() {
   STLDeleteElements(&network_status_receivers_);
@@ -214,8 +216,8 @@ void ChromeNetwork::UpdatePacketHandler(
   packet_handler_ = packet_handler;
   if (packet_handler_ != NULL) {
     packet_handler_->SetMessageReceiver(
-        invalidation::NewPermanentCallback(
-            this, &ChromeNetwork::HandleInboundMessage));
+        scoped_callback_factory_.NewCallback(
+            &ChromeNetwork::HandleInboundMessage));
   }
 }
 
@@ -248,7 +250,7 @@ void ChromeSystemResources::Stop() {
   listener_scheduler_->Stop();
 }
 
-bool ChromeSystemResources::IsStarted() {
+bool ChromeSystemResources::IsStarted() const {
   return is_started_;
 }
 
@@ -256,7 +258,7 @@ void ChromeSystemResources::set_platform(const std::string& platform) {
   platform_ = platform;
 }
 
-std::string ChromeSystemResources::platform() {
+std::string ChromeSystemResources::platform() const {
   return platform_;
 }
 
diff --git a/chrome/browser/sync/notifier/chrome_system_resources.h b/chrome/browser/sync/notifier/chrome_system_resources.h
index f034591..ca45cca 100644
--- a/chrome/browser/sync/notifier/chrome_system_resources.h
+++ b/chrome/browser/sync/notifier/chrome_system_resources.h
@@ -14,6 +14,8 @@
 #include <string>
 #include <vector>
 
+#include "base/compiler_specific.h"
+#include "base/memory/scoped_callback_factory.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
 #include "base/task.h"
@@ -31,8 +33,9 @@ class ChromeLogger : public invalidation::Logger {
 
   virtual ~ChromeLogger();
 
+  // invalidation::Logger implementation.
   virtual void Log(LogLevel level, const char* file, int line,
-                   const char* format, ...);
+                   const char* format, ...) OVERRIDE;
 };
 
 class ChromeScheduler : public invalidation::Scheduler {
@@ -42,16 +45,16 @@ class ChromeScheduler : public invalidation::Scheduler {
   virtual ~ChromeScheduler();
 
   // Start and stop the scheduler.
-  virtual void Start();
-  virtual void Stop();
+  void Start();
+  void Stop();
 
-  // Overrides.
+  // invalidation::Scheduler implementation.
   virtual void Schedule(invalidation::TimeDelta delay,
-                        invalidation::Closure* task);
+                        invalidation::Closure* task) OVERRIDE;
 
-  virtual bool IsRunningOnThread();
+  virtual bool IsRunningOnThread() const OVERRIDE;
 
-  virtual invalidation::Time GetCurrentTime();
+  virtual invalidation::Time GetCurrentTime() const OVERRIDE;
 
  private:
   scoped_ptr<ScopedRunnableMethodFactory<ChromeScheduler> >
@@ -59,7 +62,6 @@ class ChromeScheduler : public invalidation::Scheduler {
   // Holds all posted tasks that have not yet been run.
   std::set<invalidation::Closure*> posted_tasks_;
 
-  // TODO(tim): Trying to debug bug crbug.com/64652.
   const MessageLoop* created_on_loop_;
   bool is_started_;
   bool is_stopped_;
@@ -83,17 +85,18 @@ class ChromeStorage : public invalidation::Storage {
     cached_state_ = value;
   }
 
-  // invalidation::Storage overrides.
+  // invalidation::Storage implementation.
   virtual void WriteKey(const std::string& key, const std::string& value,
-                        invalidation::WriteKeyCallback* done);
+                        invalidation::WriteKeyCallback* done) OVERRIDE;
 
   virtual void ReadKey(const std::string& key,
-                       invalidation::ReadKeyCallback* done);
+                       invalidation::ReadKeyCallback* done) OVERRIDE;
 
   virtual void DeleteKey(const std::string& key,
-                         invalidation::DeleteKeyCallback* done);
+                         invalidation::DeleteKeyCallback* done) OVERRIDE;
 
-  virtual void ReadAllKeys(invalidation::ReadAllKeysCallback* key_callback);
+  virtual void ReadAllKeys(
+      invalidation::ReadAllKeysCallback* key_callback) OVERRIDE;
 
  private:
   // Runs the given storage callback with SUCCESS status and deletes it.
@@ -117,14 +120,14 @@ class ChromeNetwork : public invalidation::NetworkChannel {
 
   void UpdatePacketHandler(CacheInvalidationPacketHandler* packet_handler);
 
-  // Overrides.
-  virtual void SendMessage(const std::string& outgoing_message);
+  // invalidation::NetworkChannel implementation.
+  virtual void SendMessage(const std::string& outgoing_message) OVERRIDE;
 
   virtual void SetMessageReceiver(
-      invalidation::MessageCallback* incoming_receiver);
+      invalidation::MessageCallback* incoming_receiver) OVERRIDE;
 
   virtual void AddNetworkStatusReceiver(
-      invalidation::NetworkStatusCallback* network_status_receiver);
+      invalidation::NetworkStatusCallback* network_status_receiver) OVERRIDE;
 
  private:
   void HandleInboundMessage(const std::string& incoming_message);
@@ -132,6 +135,7 @@ class ChromeNetwork : public invalidation::NetworkChannel {
   CacheInvalidationPacketHandler* packet_handler_;
   scoped_ptr<invalidation::MessageCallback> incoming_receiver_;
   std::vector<invalidation::NetworkStatusCallback*> network_status_receivers_;
+  base::ScopedCallbackFactory<ChromeNetwork> scoped_callback_factory_;
 };
 
 class ChromeSystemResources : public invalidation::SystemResources {
@@ -141,16 +145,16 @@ class ChromeSystemResources : public invalidation::SystemResources {
   virtual ~ChromeSystemResources();
 
   // invalidation::SystemResources implementation.
-  virtual void Start();
-  virtual void Stop();
-  virtual bool IsStarted();
+  virtual void Start() OVERRIDE;
+  virtual void Stop() OVERRIDE;
+  virtual bool IsStarted() const OVERRIDE;
   virtual void set_platform(const std::string& platform);
-  virtual std::string platform();
-  virtual ChromeLogger* logger();
-  virtual ChromeStorage* storage();
-  virtual ChromeNetwork* network();
-  virtual ChromeScheduler* internal_scheduler();
-  virtual ChromeScheduler* listener_scheduler();
+  virtual std::string platform() const OVERRIDE;
+  virtual ChromeLogger* logger() OVERRIDE;
+  virtual ChromeStorage* storage() OVERRIDE;
+  virtual ChromeNetwork* network() OVERRIDE;
+  virtual ChromeScheduler* internal_scheduler() OVERRIDE;
+  virtual ChromeScheduler* listener_scheduler() OVERRIDE;
 
  private:
   bool is_started_;
diff --git a/chrome/browser/sync/notifier/invalidation_util.cc b/chrome/browser/sync/notifier/invalidation_util.cc
index 360d153..f707b54 100644
--- a/chrome/browser/sync/notifier/invalidation_util.cc
+++ b/chrome/browser/sync/notifier/invalidation_util.cc
@@ -3,10 +3,12 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/notifier/invalidation_util.h"
-#include "google/cacheinvalidation/v2/types.h"
 
 #include <sstream>
 
+#include "google/cacheinvalidation/v2/types.h"
+#include "google/cacheinvalidation/v2/types.pb.h"
+
 namespace sync_notifier {
 
 void RunAndDeleteClosure(invalidation::Closure* task) {
diff --git a/chrome/browser/sync/notifier/invalidation_util.h b/chrome/browser/sync/notifier/invalidation_util.h
index 22fc05b..a61c89b 100644
--- a/chrome/browser/sync/notifier/invalidation_util.h
+++ b/chrome/browser/sync/notifier/invalidation_util.h
@@ -12,8 +12,13 @@
 
 #include "chrome/browser/sync/syncable/model_type.h"
 #include "google/cacheinvalidation/callback.h"
-#include "google/cacheinvalidation/v2/invalidation-client.h"
-#include "google/cacheinvalidation/v2/types.pb.h"
+
+namespace invalidation {
+
+class Invalidation;
+class ObjectId;
+
+}  // namespace invalidation
 
 namespace sync_notifier {
 
diff --git a/chrome/browser/sync/notifier/registration_manager.cc b/chrome/browser/sync/notifier/registration_manager.cc
index d9d4575..0308dbe 100644
--- a/chrome/browser/sync/notifier/registration_manager.cc
+++ b/chrome/browser/sync/notifier/registration_manager.cc
@@ -11,6 +11,7 @@
 #include "base/rand_util.h"
 #include "chrome/browser/sync/notifier/invalidation_util.h"
 #include "chrome/browser/sync/syncable/model_type.h"
+#include "google/cacheinvalidation/v2/invalidation-client.h"
 
 namespace sync_notifier {
 
@@ -81,9 +82,10 @@ void RegistrationManager::SetRegisteredTypes(
 void RegistrationManager::MarkRegistrationLost(
     syncable::ModelType model_type) {
   DCHECK(non_thread_safe_.CalledOnValidThread());
-  registration_statuses_[model_type].state =
-      invalidation::InvalidationListener::UNREGISTERED;
-  TryRegisterType(model_type, true /* is_retry */);
+  RegistrationStatus* status = &registration_statuses_[model_type];
+  status->state = invalidation::InvalidationListener::UNREGISTERED;
+  bool is_retry = !status->last_registration_request.is_null();
+  TryRegisterType(model_type, is_retry);
 }
 
 void RegistrationManager::MarkAllRegistrationsLost() {
diff --git a/chrome/browser/sync/notifier/registration_manager_unittest.cc b/chrome/browser/sync/notifier/registration_manager_unittest.cc
index 2e92304..8a21e1f 100644
--- a/chrome/browser/sync/notifier/registration_manager_unittest.cc
+++ b/chrome/browser/sync/notifier/registration_manager_unittest.cc
@@ -14,6 +14,7 @@
 #include "base/message_loop.h"
 #include "chrome/browser/sync/notifier/invalidation_util.h"
 #include "chrome/browser/sync/syncable/model_type.h"
+#include "google/cacheinvalidation/v2/invalidation-client.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace sync_notifier {