diff options
author | akalin@chromium.org <akalin@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-09-16 09:23:27 +0000 |
---|---|---|
committer | akalin@chromium.org <akalin@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-09-16 09:23:27 +0000 |
commit | 01f91e825511800dde18826d979cbc4d39c5862d (patch) | |
tree | 7f9734e9e108216a11dd52748dafdf2f306f297e /jingle | |
parent | 2b82c0d249fe3753e198567e505db9baade30136 (diff) | |
download | chromium_src-01f91e825511800dde18826d979cbc4d39c5862d.zip chromium_src-01f91e825511800dde18826d979cbc4d39c5862d.tar.gz chromium_src-01f91e825511800dde18826d979cbc4d39c5862d.tar.bz2 |
Refactored handling of XmppClient.
Added new classes WeakXmppClient and XmppConnection, which let me
simplify MediatorThreadImpl et al.
BUG=55302
TEST=new unit tests, manually
Review URL: http://codereview.chromium.org/3290025
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@59629 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'jingle')
25 files changed, 828 insertions, 859 deletions
diff --git a/jingle/jingle.gyp b/jingle/jingle.gyp index 79ee350..b3c867e 100644 --- a/jingle/jingle.gyp +++ b/jingle/jingle.gyp @@ -24,8 +24,12 @@ 'notifier/base/notifier_options.h', 'notifier/base/task_pump.cc', 'notifier/base/task_pump.h', + 'notifier/base/weak_xmpp_client.cc', + 'notifier/base/weak_xmpp_client.h', 'notifier/base/xmpp_client_socket_factory.cc', 'notifier/base/xmpp_client_socket_factory.h', + 'notifier/base/xmpp_connection.cc', + 'notifier/base/xmpp_connection.h', 'notifier/communicator/connection_options.cc', 'notifier/communicator/connection_options.h', 'notifier/communicator/connection_settings.cc', @@ -35,9 +39,6 @@ 'notifier/communicator/gaia_token_pre_xmpp_auth.h', 'notifier/communicator/login.cc', 'notifier/communicator/login.h', - 'notifier/communicator/login_connection_state.h', - 'notifier/communicator/login_failure.cc', - 'notifier/communicator/login_failure.h', 'notifier/communicator/login_settings.cc', 'notifier/communicator/login_settings.h', 'notifier/communicator/single_login_attempt.cc', @@ -55,7 +56,6 @@ 'notifier/listener/notification_defines.h', 'notifier/listener/send_update_task.cc', 'notifier/listener/send_update_task.h', - 'notifier/base/sigslotrepeater.h', 'notifier/listener/subscribe_task.cc', 'notifier/listener/subscribe_task.h', 'notifier/listener/talk_mediator.h', @@ -93,6 +93,8 @@ '../base/test/run_all_unittests.cc', 'notifier/base/chrome_async_socket_unittest.cc', 'notifier/base/fake_ssl_client_socket_unittest.cc', + 'notifier/base/xmpp_connection_unittest.cc', + 'notifier/base/weak_xmpp_client_unittest.cc', 'notifier/listener/talk_mediator_unittest.cc', 'notifier/listener/send_update_task_unittest.cc', 'notifier/listener/subscribe_task_unittest.cc', diff --git a/jingle/notifier/base/sigslotrepeater.h b/jingle/notifier/base/sigslotrepeater.h deleted file mode 100644 index cafb491..0000000 --- a/jingle/notifier/base/sigslotrepeater.h +++ /dev/null @@ -1,83 +0,0 @@ -// Copyright (c) 2009 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. - -#ifndef JINGLE_NOTIFIER_BASE_SIGSLOTREPEATER_H_ -#define JINGLE_NOTIFIER_BASE_SIGSLOTREPEATER_H_ - -// Repeaters are both signals and slots, which are designed as intermediate -// pass-throughs for signals and slots which don't know about each other (for -// modularity or encapsulation). This eliminates the need to declare a signal -// handler whose sole purpose is to fire another signal. The repeater connects -// to the originating signal using the 'repeat' method. When the repeated -// signal fires, the repeater will also fire. - -#include "talk/base/sigslot.h" - -namespace sigslot { - -template<class mt_policy = SIGSLOT_DEFAULT_MT_POLICY> -class repeater0 : public signal0<mt_policy>, - public has_slots<mt_policy> { - public: - typedef signal0<mt_policy> base_type; - typedef repeater0<mt_policy> this_type; - - repeater0() { } - explicit repeater0(const this_type& s) : base_type(s) { } - - void reemit() { signal0<mt_policy>::emit(); } - void repeat(base_type &s) { s.connect(this, &this_type::reemit); } -}; - -template<class arg1_type, class mt_policy = SIGSLOT_DEFAULT_MT_POLICY> -class repeater1 : public signal1<arg1_type, mt_policy>, - public has_slots<mt_policy> { - public: - typedef signal1<arg1_type, mt_policy> base_type; - typedef repeater1<arg1_type, mt_policy> this_type; - - repeater1() { } - repeater1(const this_type& s) : base_type(s) { } - - void reemit(arg1_type a1) { signal1<arg1_type, mt_policy>::emit(a1); } - void repeat(base_type& s) { s.connect(this, &this_type::reemit); } -}; - -template<class arg1_type, class arg2_type, - class mt_policy = SIGSLOT_DEFAULT_MT_POLICY> -class repeater2 : public signal2<arg1_type, arg2_type, mt_policy>, - public has_slots<mt_policy> { - public: - typedef signal2<arg1_type, arg2_type, mt_policy> base_type; - typedef repeater2<arg1_type, arg2_type, mt_policy> this_type; - - repeater2() { } - repeater2(const this_type& s) : base_type(s) { } - - void reemit(arg1_type a1, arg2_type a2) { - signal2<arg1_type, arg2_type, mt_policy>::emit(a1, a2); - } - void repeat(base_type& s) { s.connect(this, &this_type::reemit); } -}; - -template<class arg1_type, class arg2_type, class arg3_type, - class mt_policy = SIGSLOT_DEFAULT_MT_POLICY> -class repeater3 : public signal3<arg1_type, arg2_type, arg3_type, mt_policy>, - public has_slots<mt_policy> { - public: - typedef signal3<arg1_type, arg2_type, arg3_type, mt_policy> base_type; - typedef repeater3<arg1_type, arg2_type, arg3_type, mt_policy> this_type; - - repeater3() { } - repeater3(const this_type& s) : base_type(s) { } - - void reemit(arg1_type a1, arg2_type a2, arg3_type a3) { - signal3<arg1_type, arg2_type, arg3_type, mt_policy>::emit(a1, a2, a3); - } - void repeat(base_type& s) { s.connect(this, &this_type::reemit); } -}; - -} // namespace sigslot - -#endif // JINGLE_NOTIFIER_BASE_SIGSLOTREPEATER_H_ diff --git a/jingle/notifier/base/task_pump.cc b/jingle/notifier/base/task_pump.cc index 52ffd26..9423ffa 100644 --- a/jingle/notifier/base/task_pump.cc +++ b/jingle/notifier/base/task_pump.cc @@ -12,12 +12,17 @@ TaskPump::TaskPump() ALLOW_THIS_IN_INITIALIZER_LIST(this)), posted_wake_(false) {} -TaskPump::~TaskPump() {} +TaskPump::~TaskPump() { + DCHECK(non_thread_safe_.CalledOnValidThread()); +} void TaskPump::WakeTasks() { + DCHECK(non_thread_safe_.CalledOnValidThread()); if (!posted_wake_) { + MessageLoop* current_message_loop = MessageLoop::current(); + CHECK(current_message_loop); // Do the requested wake up. - MessageLoop::current()->PostTask( + current_message_loop->PostTask( FROM_HERE, scoped_runnable_method_factory_.NewRunnableMethod( &TaskPump::CheckAndRunTasks)); @@ -26,12 +31,14 @@ void TaskPump::WakeTasks() { } int64 TaskPump::CurrentTime() { + DCHECK(non_thread_safe_.CalledOnValidThread()); // Only timeout tasks rely on this function. Since we're not using // libjingle tasks for timeout, it's safe to return 0 here. return 0; } void TaskPump::CheckAndRunTasks() { + DCHECK(non_thread_safe_.CalledOnValidThread()); posted_wake_ = false; // We shouldn't be using libjingle for timeout tasks, so we should // have no timeout tasks at all. diff --git a/jingle/notifier/base/task_pump.h b/jingle/notifier/base/task_pump.h index 806fed2..ff8411a 100644 --- a/jingle/notifier/base/task_pump.h +++ b/jingle/notifier/base/task_pump.h @@ -5,6 +5,7 @@ #ifndef JINGLE_NOTIFIER_BASE_TASK_PUMP_H_ #define JINGLE_NOTIFIER_BASE_TASK_PUMP_H_ +#include "base/non_thread_safe.h" #include "base/task.h" #include "talk/base/taskrunner.h" @@ -23,6 +24,7 @@ class TaskPump : public talk_base::TaskRunner { private: void CheckAndRunTasks(); + NonThreadSafe non_thread_safe_; ScopedRunnableMethodFactory<TaskPump> scoped_runnable_method_factory_; bool posted_wake_; diff --git a/jingle/notifier/base/weak_xmpp_client.cc b/jingle/notifier/base/weak_xmpp_client.cc new file mode 100644 index 0000000..4eedd9f --- /dev/null +++ b/jingle/notifier/base/weak_xmpp_client.cc @@ -0,0 +1,41 @@ +// Copyright (c) 2010 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 "jingle/notifier/base/weak_xmpp_client.h" + +#include "base/compiler_specific.h" + +namespace notifier { + +WeakXmppClient::WeakXmppClient(talk_base::TaskParent* parent) + : buzz::XmppClient(parent), + weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {} + +WeakXmppClient::~WeakXmppClient() { + DCHECK(non_thread_safe_.CalledOnValidThread()); + Invalidate(); +} + +void WeakXmppClient::Invalidate() { + DCHECK(non_thread_safe_.CalledOnValidThread()); + // We don't want XmppClient raising any signals once its invalidated. + SignalStateChange.disconnect_all(); + SignalLogInput.disconnect_all(); + SignalLogOutput.disconnect_all(); + weak_ptr_factory_.InvalidateWeakPtrs(); +} + +base::WeakPtr<WeakXmppClient> WeakXmppClient::AsWeakPtr() { + DCHECK(non_thread_safe_.CalledOnValidThread()); + return weak_ptr_factory_.GetWeakPtr(); +} + +void WeakXmppClient::Stop() { + DCHECK(non_thread_safe_.CalledOnValidThread()); + // We don't want XmppClient used after it has been stopped. + Invalidate(); + buzz::XmppClient::Stop(); +} + +} // namespace notifier diff --git a/jingle/notifier/base/weak_xmpp_client.h b/jingle/notifier/base/weak_xmpp_client.h new file mode 100644 index 0000000..adda962 --- /dev/null +++ b/jingle/notifier/base/weak_xmpp_client.h @@ -0,0 +1,57 @@ +// Copyright (c) 2010 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. +// +// A thin wrapper around buzz::XmppClient that exposes weak pointers +// so that users know when the buzz::XmppClient becomes invalid to use +// (not necessarily only at destruction time). + +#ifndef JINGLE_NOTIFIER_BASE_WEAK_XMPP_CLIENT_H_ +#define JINGLE_NOTIFIER_BASE_WEAK_XMPP_CLIENT_H_ +#pragma once + +#include "base/basictypes.h" +#include "base/non_thread_safe.h" +#include "base/weak_ptr.h" +#include "talk/xmpp/xmppclient.h" + +namespace talk_base { +class TaskParent; +} // namespace + +namespace notifier { + +// buzz::XmppClient's destructor isn't marked virtual, but it inherits +// from talk_base::Task, whose destructor *is* marked virtual, so we +// can safely inherit from it. +class WeakXmppClient : public buzz::XmppClient { + public: + explicit WeakXmppClient(talk_base::TaskParent* parent); + + virtual ~WeakXmppClient(); + + // Returns a weak pointer that is invalidated when the XmppClient + // becomes invalid to use. + base::WeakPtr<WeakXmppClient> AsWeakPtr(); + + // Invalidates all weak pointers to this object. (This method is + // necessary as calling Abort() does not always lead to Stop() being + // called, so it's not a reliable way to cause an invalidation.) + void Invalidate(); + + protected: + virtual void Stop(); + + private: + NonThreadSafe non_thread_safe_; + // We use our own WeakPtrFactory instead of inheriting from + // SupportsWeakPtr since we want to invalidate in other places + // besides the destructor. + base::WeakPtrFactory<WeakXmppClient> weak_ptr_factory_; + + DISALLOW_COPY_AND_ASSIGN(WeakXmppClient); +}; + +} // namespace notifier + +#endif // JINGLE_NOTIFIER_BASE_WEAK_XMPP_CLIENT_H_ diff --git a/jingle/notifier/base/weak_xmpp_client_unittest.cc b/jingle/notifier/base/weak_xmpp_client_unittest.cc new file mode 100644 index 0000000..705bb35 --- /dev/null +++ b/jingle/notifier/base/weak_xmpp_client_unittest.cc @@ -0,0 +1,124 @@ +// Copyright (c) 2010 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 "jingle/notifier/base/weak_xmpp_client.h" + +#include "base/basictypes.h" +#include "base/message_loop.h" +#include "base/scoped_ptr.h" +#include "base/weak_ptr.h" +#include "jingle/notifier/base/task_pump.h" +#include "talk/base/sigslot.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace notifier { + +namespace { + +class MockXmppDelegate : public sigslot::has_slots<> { + public: + virtual ~MockXmppDelegate() {} + + MOCK_METHOD1(OnStateChange, void(buzz::XmppEngine::State)); + MOCK_METHOD2(OnInputLog, void(const char*, int)); + MOCK_METHOD2(OnOutputLog, void(const char*, int)); +}; + +const buzz::XmppEngine::State kState = buzz::XmppEngine::STATE_OPEN; +const char kInputLog[] = "input log"; +const char kOutputLog[] = "output log"; + +class WeakXmppClientTest : public testing::Test { + protected: + WeakXmppClientTest() : task_pump_(new TaskPump()) {} + + virtual ~WeakXmppClientTest() {} + + void ConnectSignals(buzz::XmppClient* xmpp_client) { + xmpp_client->SignalStateChange.connect( + &mock_xmpp_delegate_, &MockXmppDelegate::OnStateChange); + xmpp_client->SignalLogInput.connect( + &mock_xmpp_delegate_, &MockXmppDelegate::OnInputLog); + xmpp_client->SignalLogOutput.connect( + &mock_xmpp_delegate_, &MockXmppDelegate::OnOutputLog); + } + + void ExpectSignalCalls() { + EXPECT_CALL(mock_xmpp_delegate_, OnStateChange(kState)); + EXPECT_CALL(mock_xmpp_delegate_, + OnInputLog(kInputLog, arraysize(kInputLog))); + EXPECT_CALL(mock_xmpp_delegate_, + OnOutputLog(kOutputLog, arraysize(kOutputLog))); + } + + void RaiseSignals(buzz::XmppClient* xmpp_client) { + xmpp_client->SignalStateChange(kState); + xmpp_client->SignalLogInput(kInputLog, arraysize(kInputLog)); + xmpp_client->SignalLogOutput(kOutputLog, arraysize(kOutputLog)); + } + + // Needed by TaskPump. + MessageLoop message_loop_; + + scoped_ptr<TaskPump> task_pump_; + MockXmppDelegate mock_xmpp_delegate_; +}; + +TEST_F(WeakXmppClientTest, InvalidationViaInvalidate) { + ExpectSignalCalls(); + + WeakXmppClient* weak_xmpp_client = new WeakXmppClient(task_pump_.get()); + ConnectSignals(weak_xmpp_client); + + weak_xmpp_client->Start(); + base::WeakPtr<WeakXmppClient> weak_ptr = weak_xmpp_client->AsWeakPtr(); + EXPECT_TRUE(weak_ptr.get()); + RaiseSignals(weak_ptr.get()); + + weak_xmpp_client->Invalidate(); + EXPECT_FALSE(weak_ptr.get()); + // We know that |weak_xmpp_client| is still valid at this point, + // although it should be entirely disconnected. + RaiseSignals(weak_xmpp_client); +} + +TEST_F(WeakXmppClientTest, InvalidationViaStop) { + ExpectSignalCalls(); + + WeakXmppClient* weak_xmpp_client = new WeakXmppClient(task_pump_.get()); + ConnectSignals(weak_xmpp_client); + + weak_xmpp_client->Start(); + base::WeakPtr<WeakXmppClient> weak_ptr = weak_xmpp_client->AsWeakPtr(); + EXPECT_TRUE(weak_ptr.get()); + RaiseSignals(weak_ptr.get()); + + weak_xmpp_client->Abort(); + EXPECT_FALSE(weak_ptr.get()); + // We know that |weak_xmpp_client| is still valid at this point, + // although it should be entirely disconnected. + RaiseSignals(weak_xmpp_client); +} + +TEST_F(WeakXmppClientTest, InvalidationViaDestructor) { + ExpectSignalCalls(); + + WeakXmppClient* weak_xmpp_client = new WeakXmppClient(task_pump_.get()); + ConnectSignals(weak_xmpp_client); + + weak_xmpp_client->Start(); + base::WeakPtr<WeakXmppClient> weak_ptr = weak_xmpp_client->AsWeakPtr(); + EXPECT_TRUE(weak_ptr.get()); + RaiseSignals(weak_ptr.get()); + + task_pump_.reset(); + EXPECT_FALSE(weak_ptr.get()); + // |weak_xmpp_client| is truly invalid at this point so we can't + // RaiseSignals() with it. +} + +} // namespace + +} // namespace notifier diff --git a/jingle/notifier/base/xmpp_connection.cc b/jingle/notifier/base/xmpp_connection.cc new file mode 100644 index 0000000..c3597f9 --- /dev/null +++ b/jingle/notifier/base/xmpp_connection.cc @@ -0,0 +1,139 @@ +// Copyright (c) 2010 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 "jingle/notifier/base/xmpp_connection.h" + +#include "base/compiler_specific.h" +#include "base/logging.h" +#include "base/message_loop.h" +#include "base/string_piece.h" +#include "jingle/notifier/base/chrome_async_socket.h" +#include "jingle/notifier/base/task_pump.h" +#include "jingle/notifier/base/weak_xmpp_client.h" +#include "jingle/notifier/base/xmpp_client_socket_factory.h" +#include "net/base/ssl_config_service.h" +#include "talk/xmpp/xmppclientsettings.h" + +namespace notifier { + +namespace { + +buzz::AsyncSocket* CreateSocket( + const buzz::XmppClientSettings& xmpp_client_settings) { + bool use_fake_ssl_client_socket = + (xmpp_client_settings.protocol() == cricket::PROTO_SSLTCP); + net::ClientSocketFactory* const client_socket_factory = + new XmppClientSocketFactory( + net::ClientSocketFactory::GetDefaultFactory(), + use_fake_ssl_client_socket); + // The default SSLConfig is good enough for us for now. + const net::SSLConfig ssl_config; + // These numbers were taken from similar numbers in + // XmppSocketAdapter. + const size_t kReadBufSize = 64U * 1024U; + const size_t kWriteBufSize = 64U * 1024U; + // TODO(akalin): Use a real NetLog. + net::NetLog* const net_log = NULL; + return new ChromeAsyncSocket( + client_socket_factory, ssl_config, + kReadBufSize, kWriteBufSize, net_log); +} + +} // namespace + +XmppConnection::XmppConnection( + const buzz::XmppClientSettings& xmpp_client_settings, + Delegate* delegate, buzz::PreXmppAuth* pre_xmpp_auth) + : task_pump_(new TaskPump()), + on_connect_called_(false), + delegate_(delegate) { + DCHECK(delegate_); + // Owned by |task_pump_|, but is guaranteed to live at least as long + // as this function. + WeakXmppClient* weak_xmpp_client = new WeakXmppClient(task_pump_.get()); + weak_xmpp_client->SignalStateChange.connect( + this, &XmppConnection::OnStateChange); + weak_xmpp_client->SignalLogInput.connect( + this, &XmppConnection::OnInputLog); + weak_xmpp_client->SignalLogOutput.connect( + this, &XmppConnection::OnOutputLog); + const char kLanguage[] = "en"; + buzz::XmppReturnStatus connect_status = + weak_xmpp_client->Connect(xmpp_client_settings, kLanguage, + CreateSocket(xmpp_client_settings), + pre_xmpp_auth); + // buzz::XmppClient::Connect() should never fail. + DCHECK_EQ(connect_status, buzz::XMPP_RETURN_OK); + weak_xmpp_client->Start(); + weak_xmpp_client_ = weak_xmpp_client->AsWeakPtr(); +} + +XmppConnection::~XmppConnection() { + DCHECK(non_thread_safe_.CalledOnValidThread()); + ClearClient(); + MessageLoop* current_message_loop = MessageLoop::current(); + CHECK(current_message_loop); + // We do this because XmppConnection may get destroyed as a result + // of a signal from XmppClient. If we delete |task_pump_| here, bad + // things happen when the stack pops back up to the XmppClient's + // (which is deleted by |task_pump_|) function. + current_message_loop->DeleteSoon(FROM_HERE, task_pump_.release()); +} + +void XmppConnection::OnStateChange(buzz::XmppEngine::State state) { + DCHECK(non_thread_safe_.CalledOnValidThread()); + LOG(INFO) << "XmppClient state changed to " << state; + if (!weak_xmpp_client_.get()) { + LOG(DFATAL) << "weak_xmpp_client_ unexpectedly NULL"; + return; + } + if (!delegate_) { + LOG(DFATAL) << "delegate_ unexpectedly NULL"; + return; + } + switch (state) { + case buzz::XmppEngine::STATE_OPEN: + if (on_connect_called_) { + LOG(DFATAL) << "State changed to STATE_OPEN more than once"; + } else { + delegate_->OnConnect(weak_xmpp_client_); + on_connect_called_ = true; + } + break; + case buzz::XmppEngine::STATE_CLOSED: { + int subcode = 0; + buzz::XmppEngine::Error error = + weak_xmpp_client_->GetError(&subcode); + const buzz::XmlElement* stream_error = + weak_xmpp_client_->GetStreamError(); + ClearClient(); + Delegate* delegate = delegate_; + delegate_ = NULL; + delegate->OnError(error, subcode, stream_error); + break; + } + default: + // Do nothing. + break; + } +} + +void XmppConnection::OnInputLog(const char* data, int len) { + DCHECK(non_thread_safe_.CalledOnValidThread()); + LOG(INFO) << "XMPP Input: " << base::StringPiece(data, len); +} + +void XmppConnection::OnOutputLog(const char* data, int len) { + DCHECK(non_thread_safe_.CalledOnValidThread()); + LOG(INFO) << "XMPP Output: " << base::StringPiece(data, len); +} + +void XmppConnection::ClearClient() { + if (weak_xmpp_client_.get()) { + weak_xmpp_client_->Invalidate(); + DCHECK(!weak_xmpp_client_.get()); + } +} + +} // namespace notifier diff --git a/jingle/notifier/base/xmpp_connection.h b/jingle/notifier/base/xmpp_connection.h new file mode 100644 index 0000000..06d267b --- /dev/null +++ b/jingle/notifier/base/xmpp_connection.h @@ -0,0 +1,97 @@ +// Copyright (c) 2010 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. +// +// A class that manages a connection to an XMPP server. + +#ifndef JINGLE_NOTIFIER_BASE_XMPP_CONNECTION_H_ +#define JINGLE_NOTIFIER_BASE_XMPP_CONNECTION_H_ +#pragma once + +#include "base/basictypes.h" +#include "base/non_thread_safe.h" +#include "base/scoped_ptr.h" +#include "base/weak_ptr.h" +#include "talk/base/sigslot.h" +#include "talk/xmpp/xmppengine.h" +#include "testing/gtest/include/gtest/gtest_prod.h" + +namespace buzz { +class PreXmppAuth; +class XmlElement; +class XmppClientSettings; +} // namespace + +namespace talk_base { +class Task; +} // namespace + +namespace notifier { + +class TaskPump; +class WeakXmppClient; + +class XmppConnection : public sigslot::has_slots<> { + public: + class Delegate { + public: + virtual ~Delegate() {} + + // Called (at most once) when a connection has been established. + // |base_task| can be used by the client as the parent of any Task + // it creates as long as it is valid (i.e., non-NULL). + virtual void OnConnect(base::WeakPtr<talk_base::Task> base_task) = 0; + + // Called if an error has occurred (either before or after a call + // to OnConnect()). No calls to the delegate will be made after + // this call. Invalidates any weak pointers passed to the client + // by OnConnect(). + // + // |error| is the code for the raised error. |subcode| is an + // error-dependent subcode (0 if not applicable). |stream_error| + // is non-NULL iff |error| == ERROR_STREAM. |stream_error| is + // valid only for the lifetime of this function. + // + // Ideally, |error| would be set to something that is not + // ERROR_NONE, but due to inconsistent error-handling this doesn't + // always happen. + virtual void OnError(buzz::XmppEngine::Error error, int subcode, + const buzz::XmlElement* stream_error) = 0; + }; + + // Does not take ownership of |delegate|, which may not be NULL. + // Takes ownership of |pre_xmpp_auth|, which may be NULL. + // + // TODO(akalin): Avoid the need for |pre_xmpp_auth|. + XmppConnection(const buzz::XmppClientSettings& xmpp_client_settings, + Delegate* delegate, buzz::PreXmppAuth* pre_xmpp_auth); + + // Invalidates any weak pointers passed to the delegate by + // OnConnect(), but does not trigger a call to the delegate's + // OnError() function. + ~XmppConnection(); + + private: + void OnStateChange(buzz::XmppEngine::State state); + void OnInputLog(const char* data, int len); + void OnOutputLog(const char* data, int len); + + void ClearClient(); + + NonThreadSafe non_thread_safe_; + scoped_ptr<TaskPump> task_pump_; + base::WeakPtr<WeakXmppClient> weak_xmpp_client_; + bool on_connect_called_; + Delegate* delegate_; + + FRIEND_TEST(XmppConnectionTest, RaisedError); + FRIEND_TEST(XmppConnectionTest, Connect); + FRIEND_TEST(XmppConnectionTest, MultipleConnect); + FRIEND_TEST(XmppConnectionTest, ConnectThenError); + + DISALLOW_COPY_AND_ASSIGN(XmppConnection); +}; + +} // namespace notifier + +#endif // JINGLE_NOTIFIER_BASE_XMPP_CONNECTION_H_ diff --git a/jingle/notifier/base/xmpp_connection_unittest.cc b/jingle/notifier/base/xmpp_connection_unittest.cc new file mode 100644 index 0000000..edf30e3 --- /dev/null +++ b/jingle/notifier/base/xmpp_connection_unittest.cc @@ -0,0 +1,195 @@ +// Copyright (c) 2010 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 "jingle/notifier/base/xmpp_connection.h" + +#include <string> +#include <vector> + +#include "base/basictypes.h" +#include "base/message_loop.h" +#include "base/weak_ptr.h" +#include "jingle/notifier/base/weak_xmpp_client.h" +#include "talk/xmpp/prexmppauth.h" +#include "talk/xmpp/xmppclientsettings.h" +#include "testing/gmock/include/gmock/gmock.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace buzz { +class CaptchaChallenge; +class Jid; +} // namespace buzz + +namespace talk_base { +class CryptString; +class SocketAddress; +class Task; +} // namespace talk_base + +namespace notifier { + +using ::testing::_; +using ::testing::Return; +using ::testing::SaveArg; + +class MockPreXmppAuth : public buzz::PreXmppAuth { + public: + virtual ~MockPreXmppAuth() {} + + MOCK_METHOD2(ChooseBestSaslMechanism, + std::string(const std::vector<std::string>&, bool)); + MOCK_METHOD1(CreateSaslMechanism, + buzz::SaslMechanism*(const std::string&)); + MOCK_METHOD4(StartPreXmppAuth, + void(const buzz::Jid&, + const talk_base::SocketAddress&, + const talk_base::CryptString&, + const std::string&)); + MOCK_CONST_METHOD0(IsAuthDone, bool()); + MOCK_CONST_METHOD0(IsAuthorized, bool()); + MOCK_CONST_METHOD0(HadError, bool()); + MOCK_CONST_METHOD0(GetError, int()); + MOCK_CONST_METHOD0(GetCaptchaChallenge, buzz::CaptchaChallenge()); + MOCK_CONST_METHOD0(GetAuthCookie, std::string()); +}; + +class MockXmppConnectionDelegate : public XmppConnection::Delegate { + public: + virtual ~MockXmppConnectionDelegate() {} + + MOCK_METHOD1(OnConnect, void(base::WeakPtr<talk_base::Task>)); + MOCK_METHOD3(OnError, + void(buzz::XmppEngine::Error, int, const buzz::XmlElement*)); +}; + +class XmppConnectionTest : public testing::Test { + protected: + XmppConnectionTest() : mock_pre_xmpp_auth_(new MockPreXmppAuth()) {} + + virtual ~XmppConnectionTest() {} + + virtual void TearDown() { + // Clear out any messages posted by XmppConnections. + message_loop_.RunAllPending(); + } + + // Needed by XmppConnection. + MessageLoop message_loop_; + MockXmppConnectionDelegate mock_xmpp_connection_delegate_; + scoped_ptr<MockPreXmppAuth> mock_pre_xmpp_auth_; +}; + +TEST_F(XmppConnectionTest, CreateDestroy) { + XmppConnection xmpp_connection(buzz::XmppClientSettings(), + &mock_xmpp_connection_delegate_, NULL); +} + +TEST_F(XmppConnectionTest, ImmediateFailure) { + // ChromeAsyncSocket::Connect() will always return false since we're + // not setting a valid host, but this gets bubbled up as ERROR_NONE + // due to XmppClient's inconsistent error-handling. + EXPECT_CALL(mock_xmpp_connection_delegate_, + OnError(buzz::XmppEngine::ERROR_NONE, 0, NULL)); + + XmppConnection xmpp_connection(buzz::XmppClientSettings(), + &mock_xmpp_connection_delegate_, NULL); +} + +TEST_F(XmppConnectionTest, PreAuthFailure) { + EXPECT_CALL(*mock_pre_xmpp_auth_, StartPreXmppAuth(_, _, _, _)); + EXPECT_CALL(*mock_pre_xmpp_auth_, IsAuthDone()).WillOnce(Return(true)); + EXPECT_CALL(*mock_pre_xmpp_auth_, IsAuthorized()).WillOnce(Return(false)); + EXPECT_CALL(*mock_pre_xmpp_auth_, HadError()).WillOnce(Return(true)); + EXPECT_CALL(*mock_pre_xmpp_auth_, GetError()).WillOnce(Return(5)); + + EXPECT_CALL(mock_xmpp_connection_delegate_, + OnError(buzz::XmppEngine::ERROR_AUTH, 5, NULL)); + + XmppConnection xmpp_connection( + buzz::XmppClientSettings(), &mock_xmpp_connection_delegate_, + mock_pre_xmpp_auth_.release()); +} + +TEST_F(XmppConnectionTest, FailureAfterPreAuth) { + EXPECT_CALL(*mock_pre_xmpp_auth_, StartPreXmppAuth(_, _, _, _)); + EXPECT_CALL(*mock_pre_xmpp_auth_, IsAuthDone()).WillOnce(Return(true)); + EXPECT_CALL(*mock_pre_xmpp_auth_, IsAuthorized()).WillOnce(Return(true)); + EXPECT_CALL(*mock_pre_xmpp_auth_, GetAuthCookie()).WillOnce(Return("")); + + EXPECT_CALL(mock_xmpp_connection_delegate_, + OnError(buzz::XmppEngine::ERROR_NONE, 0, NULL)); + + XmppConnection xmpp_connection( + buzz::XmppClientSettings(), &mock_xmpp_connection_delegate_, + mock_pre_xmpp_auth_.release()); +} + +TEST_F(XmppConnectionTest, RaisedError) { + EXPECT_CALL(mock_xmpp_connection_delegate_, + OnError(buzz::XmppEngine::ERROR_NONE, 0, NULL)); + + XmppConnection xmpp_connection(buzz::XmppClientSettings(), + &mock_xmpp_connection_delegate_, NULL); + + xmpp_connection.weak_xmpp_client_-> + SignalStateChange(buzz::XmppEngine::STATE_CLOSED); +} + +TEST_F(XmppConnectionTest, Connect) { + base::WeakPtr<talk_base::Task> weak_ptr; + EXPECT_CALL(mock_xmpp_connection_delegate_, OnConnect(_)). + WillOnce(SaveArg<0>(&weak_ptr)); + + { + XmppConnection xmpp_connection(buzz::XmppClientSettings(), + &mock_xmpp_connection_delegate_, NULL); + + xmpp_connection.weak_xmpp_client_-> + SignalStateChange(buzz::XmppEngine::STATE_OPEN); + EXPECT_EQ(xmpp_connection.weak_xmpp_client_.get(), weak_ptr.get()); + } + + EXPECT_EQ(NULL, weak_ptr.get()); +} + +TEST_F(XmppConnectionTest, MultipleConnect) { + EXPECT_DEBUG_DEATH({ + base::WeakPtr<talk_base::Task> weak_ptr; + EXPECT_CALL(mock_xmpp_connection_delegate_, OnConnect(_)). + WillOnce(SaveArg<0>(&weak_ptr)); + + XmppConnection xmpp_connection(buzz::XmppClientSettings(), + &mock_xmpp_connection_delegate_, NULL); + + xmpp_connection.weak_xmpp_client_-> + SignalStateChange(buzz::XmppEngine::STATE_OPEN); + for (int i = 0; i < 3; ++i) { + xmpp_connection.weak_xmpp_client_-> + SignalStateChange(buzz::XmppEngine::STATE_OPEN); + } + + EXPECT_EQ(xmpp_connection.weak_xmpp_client_.get(), weak_ptr.get()); + }, "more than once"); +} + +TEST_F(XmppConnectionTest, ConnectThenError) { + base::WeakPtr<talk_base::Task> weak_ptr; + EXPECT_CALL(mock_xmpp_connection_delegate_, OnConnect(_)). + WillOnce(SaveArg<0>(&weak_ptr)); + EXPECT_CALL(mock_xmpp_connection_delegate_, + OnError(buzz::XmppEngine::ERROR_NONE, 0, NULL)); + + XmppConnection xmpp_connection(buzz::XmppClientSettings(), + &mock_xmpp_connection_delegate_, NULL); + + xmpp_connection.weak_xmpp_client_-> + SignalStateChange(buzz::XmppEngine::STATE_OPEN); + EXPECT_EQ(xmpp_connection.weak_xmpp_client_.get(), weak_ptr.get()); + + xmpp_connection.weak_xmpp_client_-> + SignalStateChange(buzz::XmppEngine::STATE_CLOSED); + EXPECT_EQ(NULL, weak_ptr.get()); +} + +} // namespace notifier diff --git a/jingle/notifier/communicator/connection_settings.cc b/jingle/notifier/communicator/connection_settings.cc index 7215417..a16ce39 100644 --- a/jingle/notifier/communicator/connection_settings.cc +++ b/jingle/notifier/communicator/connection_settings.cc @@ -26,26 +26,15 @@ void ConnectionSettings::FillXmppClientSettings( DCHECK(xcs); xcs->set_protocol(protocol_); xcs->set_server(server_); - xcs->set_proxy(proxy_.type); - if (proxy_.type != talk_base::PROXY_NONE) { - xcs->set_proxy_host(proxy_.address.IPAsString()); - xcs->set_proxy_port(proxy_.address.port()); - } - if ((proxy_.type != talk_base::PROXY_NONE) && !proxy_.username.empty()) { - xcs->set_use_proxy_auth(true); - xcs->set_proxy_user(proxy_.username); - xcs->set_proxy_pass(proxy_.password); - } else { - xcs->set_use_proxy_auth(false); - } + xcs->set_proxy(talk_base::PROXY_NONE); + xcs->set_use_proxy_auth(false); } void ConnectionSettingsList::AddPermutations(const std::string& hostname, const std::vector<uint32>& iplist, int16 port, bool special_port_magic, - bool try_ssltcp_first, - bool proxy_only) { + bool try_ssltcp_first) { // randomize the list. This ensures the iplist isn't always // evaluated in the order returned by DNS std::vector<uint32> iplist_random = iplist; @@ -63,7 +52,7 @@ void ConnectionSettingsList::AddPermutations(const std::string& hostname, if (iplist_random.empty()) { // We couldn't pre-resolve the hostname, so let's hope it will resolve // further down the pipeline (by a proxy, for example). - PermuteForAddress(server, special_port_magic, try_ssltcp_first, proxy_only, + PermuteForAddress(server, special_port_magic, try_ssltcp_first, &list_temp); } else { // Generate a set of possibilities for each server address. @@ -76,7 +65,7 @@ void ConnectionSettingsList::AddPermutations(const std::string& hostname, iplist_seen_.push_back(iplist_random[index]); server.SetResolvedIP(iplist_random[index]); PermuteForAddress(server, special_port_magic, try_ssltcp_first, - proxy_only, &list_temp); + &list_temp); } } @@ -92,7 +81,6 @@ void ConnectionSettingsList::PermuteForAddress( const talk_base::SocketAddress& server, bool special_port_magic, bool try_ssltcp_first, - bool proxy_only, std::deque<ConnectionSettings>* list_temp) { DCHECK(list_temp); *(template_.mutable_server()) = server; @@ -105,36 +93,11 @@ void ConnectionSettingsList::PermuteForAddress( ConnectionSettings settings(template_); settings.set_protocol(cricket::PROTO_SSLTCP); settings.mutable_server()->SetPort(443); - // HTTPS proxies usually require port 443, so try it first. In addition, - // when certain tests like the sync integration tests are run on the - // chromium builders, we try the SSLTCP port (443) first because the XMPP - // port (5222) is blocked. - if ((template_.proxy().type == talk_base::PROXY_HTTPS) || - (template_.proxy().type == talk_base::PROXY_UNKNOWN) || - try_ssltcp_first) { + if (try_ssltcp_first) { list_temp->push_front(settings); } else { list_temp->push_back(settings); } } - - if (!proxy_only) { - // Try without the proxy - if (template_.proxy().type != talk_base::PROXY_NONE) { - ConnectionSettings settings(template_); - settings.mutable_proxy()->type = talk_base::PROXY_NONE; - list_temp->push_back(settings); - - if (special_port_magic) { - settings.set_protocol(cricket::PROTO_SSLTCP); - settings.mutable_server()->SetPort(443); - if (try_ssltcp_first) { - list_temp->push_front(settings); - } else { - list_temp->push_back(settings); - } - } - } - } } } // namespace notifier diff --git a/jingle/notifier/communicator/connection_settings.h b/jingle/notifier/communicator/connection_settings.h index c01d8cf..9f7e4b1 100644 --- a/jingle/notifier/communicator/connection_settings.h +++ b/jingle/notifier/communicator/connection_settings.h @@ -19,18 +19,15 @@ class ConnectionSettings { cricket::ProtocolType protocol() { return protocol_; } const talk_base::SocketAddress& server() const { return server_; } - const talk_base::ProxyInfo& proxy() const { return proxy_; } void set_protocol(cricket::ProtocolType protocol) { protocol_ = protocol; } talk_base::SocketAddress* mutable_server() { return &server_; } - talk_base::ProxyInfo* mutable_proxy() { return &proxy_; } void FillXmppClientSettings(buzz::XmppClientSettings* xcs) const; private: cricket::ProtocolType protocol_; // PROTO_TCP, PROTO_SSLTCP, etc. talk_base::SocketAddress server_; // Server. - talk_base::ProxyInfo proxy_; // Proxy info. // Need copy constructor due to use in stl deque. }; @@ -38,14 +35,6 @@ class ConnectionSettingsList { public: ConnectionSettingsList() {} - void SetProxy(const talk_base::ProxyInfo& proxy) { - *(template_.mutable_proxy()) = proxy; - } - - const talk_base::ProxyInfo& proxy() const { - return template_.proxy(); - } - int GetCount() { return list_.size(); } ConnectionSettings* GetSettings(size_t index) { return &list_[index]; } @@ -58,13 +47,11 @@ class ConnectionSettingsList { const std::vector<uint32>& iplist, int16 port, bool special_port_magic, - bool try_ssltcp_first, - bool proxy_only); + bool try_ssltcp_first); private: void PermuteForAddress(const talk_base::SocketAddress& server, bool special_port_magic, bool try_ssltcp_first, - bool proxy_only, std::deque<ConnectionSettings>* list_temp); ConnectionSettings template_; diff --git a/jingle/notifier/communicator/login.cc b/jingle/notifier/communicator/login.cc index cd0cbbf..251a813 100644 --- a/jingle/notifier/communicator/login.cc +++ b/jingle/notifier/communicator/login.cc @@ -30,35 +30,24 @@ namespace notifier { // Redirect valid for 5 minutes. static const int kRedirectTimeoutMinutes = 5; -Login::Login(talk_base::TaskParent* parent, - const buzz::XmppClientSettings& user_settings, +Login::Login(const buzz::XmppClientSettings& user_settings, const ConnectionOptions& options, - std::string lang, net::HostResolver* host_resolver, ServerInformation* server_list, int server_count, - talk_base::FirewallManager* firewall, - bool try_ssltcp_first, - bool proxy_only) - : parent_(parent), - login_settings_(new LoginSettings(user_settings, + bool try_ssltcp_first) + : login_settings_(new LoginSettings(user_settings, options, - lang, host_resolver, server_list, server_count, - firewall, - try_ssltcp_first, - proxy_only)), - state_(STATE_DISCONNECTED), - single_attempt_(NULL), + try_ssltcp_first)), redirect_port_(0) { net::NetworkChangeNotifier::AddObserver(this); ResetReconnectState(); } Login::~Login() { - Disconnect(); net::NetworkChangeNotifier::RemoveObserver(this); } @@ -75,37 +64,18 @@ void Login::StartConnection() { login_settings_->clear_server_override(); } - Disconnect(); - LOG(INFO) << "Starting connection..."; - single_attempt_ = new SingleLoginAttempt(parent_, - login_settings_.get()); + single_attempt_.reset(new SingleLoginAttempt(login_settings_.get())); // Do the signaling hook-ups. - single_attempt_->SignalUnexpectedDisconnect.connect( - this, - &Login::TryReconnect); single_attempt_->SignalNeedAutoReconnect.connect( this, &Login::TryReconnect); - single_attempt_->SignalLoginFailure.connect(this, &Login::OnLoginFailure); - single_attempt_->SignalLogoff.connect( - this, - &Login::Disconnect); single_attempt_->SignalRedirect.connect(this, &Login::OnRedirect); - single_attempt_->SignalClientStateChange.connect( + single_attempt_->SignalConnect.connect( this, - &Login::OnClientStateChange); - SignalLogInput.repeat(single_attempt_->SignalLogInput); - SignalLogOutput.repeat(single_attempt_->SignalLogOutput); - - single_attempt_->Start(); -} - -void Login::OnLoginFailure(const LoginFailure& failure) { - SignalLoginFailure(failure); - TryReconnect(); + &Login::OnConnect); } void Login::OnRedirect(const std::string& redirect_server, int redirect_port) { @@ -119,27 +89,9 @@ void Login::OnRedirect(const std::string& redirect_server, int redirect_port) { StartConnection(); } -void Login::OnClientStateChange(buzz::XmppEngine::State state) { - // We only care about when we're connected. - if (state == buzz::XmppEngine::STATE_OPEN) { - ResetReconnectState(); - ChangeState(STATE_CONNECTED); - } -} - -void Login::ChangeState(LoginConnectionState new_state) { - if (state_ != new_state) { - state_ = new_state; - LOG(INFO) << "Signalling new state " << state_; - SignalClientStateChange(state_); - } -} - -buzz::XmppClient* Login::xmpp_client() { - if (!single_attempt_) { - return NULL; - } - return single_attempt_->xmpp_client(); +void Login::OnConnect(base::WeakPtr<talk_base::Task> base_task) { + ResetReconnectState(); + SignalConnect(base_task); } void Login::OnIPAddressChanged() { @@ -150,15 +102,6 @@ void Login::OnIPAddressChanged() { TryReconnect(); } -void Login::Disconnect() { - if (single_attempt_) { - LOG(INFO) << "Disconnecting"; - single_attempt_->Abort(); - single_attempt_ = NULL; - } - ChangeState(STATE_DISCONNECTED); -} - void Login::ResetReconnectState() { reconnect_interval_ = base::TimeDelta::FromSeconds(base::RandInt(5, 25)); @@ -167,12 +110,13 @@ void Login::ResetReconnectState() { void Login::TryReconnect() { DCHECK_GT(reconnect_interval_.InSeconds(), 0); - Disconnect(); + single_attempt_.reset(); reconnect_timer_.Stop(); LOG(INFO) << "Reconnecting in " << reconnect_interval_.InSeconds() << " seconds"; reconnect_timer_.Start( reconnect_interval_, this, &Login::DoReconnect); + SignalDisconnect(); } void Login::DoReconnect() { diff --git a/jingle/notifier/communicator/login.h b/jingle/notifier/communicator/login.h index 27b2543..b66ee7b 100644 --- a/jingle/notifier/communicator/login.h +++ b/jingle/notifier/communicator/login.h @@ -7,13 +7,12 @@ #include <string> +#include "base/scoped_ptr.h" #include "base/time.h" #include "base/timer.h" -#include "jingle/notifier/base/sigslotrepeater.h" -#include "jingle/notifier/communicator/login_connection_state.h" +#include "base/weak_ptr.h" #include "net/base/network_change_notifier.h" #include "talk/base/proxyinfo.h" -#include "talk/base/scoped_ptr.h" #include "talk/base/sigslot.h" #include "talk/xmpp/xmppengine.h" @@ -28,15 +27,13 @@ class HostResolver; } // namespace net namespace talk_base { -class FirewallManager; -struct ProxyInfo; +class Task; class TaskParent; } // namespace talk_base namespace notifier { class ConnectionOptions; -class LoginFailure; class LoginSettings; struct ServerInformation; class SingleLoginAttempt; @@ -48,41 +45,26 @@ class Login : public net::NetworkChangeNotifier::Observer, public sigslot::has_slots<> { public: // firewall may be NULL. - Login(talk_base::TaskParent* parent, - const buzz::XmppClientSettings& user_settings, + Login(const buzz::XmppClientSettings& user_settings, const ConnectionOptions& options, - std::string lang, net::HostResolver* host_resolver, ServerInformation* server_list, int server_count, - talk_base::FirewallManager* firewall, - bool try_ssltcp_first, - bool proxy_only); + bool try_ssltcp_first); virtual ~Login(); void StartConnection(); - buzz::XmppClient* xmpp_client(); - // net::NetworkChangeNotifier::Observer implementation. virtual void OnIPAddressChanged(); - sigslot::signal1<LoginConnectionState> SignalClientStateChange; - - sigslot::signal1<const LoginFailure&> SignalLoginFailure; - sigslot::repeater2<const char*, int> SignalLogInput; - sigslot::repeater2<const char*, int> SignalLogOutput; + sigslot::signal1<base::WeakPtr<talk_base::Task> > SignalConnect; + sigslot::signal0<> SignalDisconnect; private: - void OnLoginFailure(const LoginFailure& failure); void OnLogoff(); void OnRedirect(const std::string& redirect_server, int redirect_port); - void OnClientStateChange(buzz::XmppEngine::State state); - - void ChangeState(LoginConnectionState new_state); - - // Abort any existing connection. - void Disconnect(); + void OnConnect(base::WeakPtr<talk_base::Task> parent); // Stops any existing reconnect timer and sets an initial reconnect // interval. @@ -98,8 +80,7 @@ class Login : public net::NetworkChangeNotifier::Observer, talk_base::TaskParent* parent_; scoped_ptr<LoginSettings> login_settings_; - LoginConnectionState state_; - SingleLoginAttempt* single_attempt_; + scoped_ptr<SingleLoginAttempt> single_attempt_; // reconnection state. base::TimeDelta reconnect_interval_; diff --git a/jingle/notifier/communicator/login_connection_state.h b/jingle/notifier/communicator/login_connection_state.h deleted file mode 100644 index d4c6470..0000000 --- a/jingle/notifier/communicator/login_connection_state.h +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright (c) 2010 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. -// -// LoginConnectionState is an enum representing the state of the XMPP -// connection. - -#ifndef JINGLE_NOTIFIER_COMMUNICATOR_LOGIN_CONNECTION_STATE_H_ -#define JINGLE_NOTIFIER_COMMUNICATOR_LOGIN_CONNECTION_STATE_H_ - -namespace notifier { - -enum LoginConnectionState { - STATE_DISCONNECTED, - STATE_CONNECTED, -}; - -} // namespace notifier - -#endif // JINGLE_NOTIFIER_COMMUNICATOR_LOGIN_CONNECTION_STATE_H_ diff --git a/jingle/notifier/communicator/login_failure.cc b/jingle/notifier/communicator/login_failure.cc deleted file mode 100644 index 85b0fcf..0000000 --- a/jingle/notifier/communicator/login_failure.cc +++ /dev/null @@ -1,30 +0,0 @@ -// Copyright (c) 2009 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 "jingle/notifier/communicator/login_failure.h" - -#include "base/logging.h" - -namespace notifier { - -LoginFailure::LoginFailure(LoginError error) - : error_(error), - xmpp_error_(buzz::XmppEngine::ERROR_NONE), - subcode_(0) { -} - -LoginFailure::LoginFailure(LoginError error, - buzz::XmppEngine::Error xmpp_error, - int subcode) - : error_(error), - xmpp_error_(xmpp_error), - subcode_(subcode) { -} - -buzz::XmppEngine::Error LoginFailure::xmpp_error() const { - DCHECK_EQ(error_, XMPP_ERROR); - return xmpp_error_; -} - -} // namespace notifier diff --git a/jingle/notifier/communicator/login_failure.h b/jingle/notifier/communicator/login_failure.h deleted file mode 100644 index af4b66c..0000000 --- a/jingle/notifier/communicator/login_failure.h +++ /dev/null @@ -1,50 +0,0 @@ -// Copyright (c) 2009 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. - -#ifndef JINGLE_NOTIFIER_COMMUNICATOR_LOGIN_FAILURE_H_ -#define JINGLE_NOTIFIER_COMMUNICATOR_LOGIN_FAILURE_H_ - -#include "talk/xmpp/xmppengine.h" - -namespace notifier { - -class LoginFailure { - public: - enum LoginError { - // Check the xmpp_error for more information. - XMPP_ERROR, - - // If the certificate has expired, it usually means that the computer's - // clock isn't set correctly. - CERTIFICATE_EXPIRED_ERROR, - - // Apparently, there is a proxy that needs authentication information. - PROXY_AUTHENTICATION_ERROR, - }; - - explicit LoginFailure(LoginError error); - LoginFailure(LoginError error, - buzz::XmppEngine::Error xmpp_error, - int subcode); - - // Used as the first level of error information. - LoginError error() const { - return error_; - } - - // Returns the XmppEngine only. Valid if and only if error() == XMPP_ERROR. - // - // Handler should mimic logic from PhoneWindow::ShowConnectionError - // (except that the DiagnoseConnectionError has already been done). - buzz::XmppEngine::Error xmpp_error() const; - - private: - const LoginError error_; - const buzz::XmppEngine::Error xmpp_error_; - const int subcode_; -}; - -} // namespace notifier - -#endif // JINGLE_NOTIFIER_COMMUNICATOR_LOGIN_FAILURE_H_ diff --git a/jingle/notifier/communicator/login_settings.cc b/jingle/notifier/communicator/login_settings.cc index 0eefa42..01b47f6 100644 --- a/jingle/notifier/communicator/login_settings.cc +++ b/jingle/notifier/communicator/login_settings.cc @@ -17,17 +17,11 @@ namespace notifier { LoginSettings::LoginSettings(const buzz::XmppClientSettings& user_settings, const ConnectionOptions& options, - const std::string& lang, net::HostResolver* host_resolver, ServerInformation* server_list, int server_count, - talk_base::FirewallManager* firewall, - bool try_ssltcp_first, - bool proxy_only) + bool try_ssltcp_first) : try_ssltcp_first_(try_ssltcp_first), - proxy_only_(proxy_only), - firewall_(firewall), - lang_(lang), host_resolver_(host_resolver), server_list_(new ServerInformation[server_count]), server_count_(server_count), diff --git a/jingle/notifier/communicator/login_settings.h b/jingle/notifier/communicator/login_settings.h index babfa2e..d97e987 100644 --- a/jingle/notifier/communicator/login_settings.h +++ b/jingle/notifier/communicator/login_settings.h @@ -19,7 +19,6 @@ class HostResolver; } namespace talk_base { -class FirewallManager; class SocketAddress; } @@ -31,36 +30,17 @@ class LoginSettings { public: LoginSettings(const buzz::XmppClientSettings& user_settings, const ConnectionOptions& options, - const std::string& lang, net::HostResolver* host_resolver, ServerInformation* server_list, int server_count, - talk_base::FirewallManager* firewall, - bool try_ssltcp_first, - bool proxy_only); + bool try_ssltcp_first); ~LoginSettings(); - // Note: firewall() may return NULL. - // - // Could be a const method, but it allows - // modification of part (FirewallManager) of its state. - talk_base::FirewallManager* firewall() { - return firewall_; - } - bool try_ssltcp_first() const { return try_ssltcp_first_; } - bool proxy_only() const { - return proxy_only_; - } - - const std::string& lang() const { - return lang_; - } - net::HostResolver* host_resolver() { return host_resolver_; } @@ -90,9 +70,6 @@ class LoginSettings { private: bool try_ssltcp_first_; - bool proxy_only_; - talk_base::FirewallManager* firewall_; - std::string lang_; net::HostResolver* host_resolver_; talk_base::scoped_array<ServerInformation> server_list_; diff --git a/jingle/notifier/communicator/single_login_attempt.cc b/jingle/notifier/communicator/single_login_attempt.cc index 4db7758..83fd21f 100644 --- a/jingle/notifier/communicator/single_login_attempt.cc +++ b/jingle/notifier/communicator/single_login_attempt.cc @@ -10,17 +10,12 @@ #include "jingle/notifier/communicator/single_login_attempt.h" #include "base/logging.h" -#include "jingle/notifier/base/chrome_async_socket.h" -#include "jingle/notifier/base/xmpp_client_socket_factory.h" #include "jingle/notifier/communicator/connection_options.h" #include "jingle/notifier/communicator/connection_settings.h" #include "jingle/notifier/communicator/const_communicator.h" #include "jingle/notifier/communicator/gaia_token_pre_xmpp_auth.h" -#include "jingle/notifier/communicator/login_failure.h" #include "jingle/notifier/communicator/login_settings.h" -#include "jingle/notifier/communicator/xmpp_connection_generator.h" -#include "net/base/ssl_config_service.h" -#include "net/socket/client_socket_factory.h" +#include "jingle/notifier/listener/xml_element_util.h" #include "talk/xmllite/xmlelement.h" #include "talk/xmpp/xmppclient.h" #include "talk/xmpp/xmppclientsettings.h" @@ -32,296 +27,40 @@ class NetLog; namespace notifier { -static void GetClientErrorInformation( - buzz::XmppClient* client, - buzz::XmppEngine::Error* error, - int* subcode, - buzz::XmlElement** stream_error) { - DCHECK(client); - DCHECK(error); - DCHECK(subcode); - DCHECK(stream_error); - - *error = client->GetError(subcode); - - *stream_error = NULL; - if (*error == buzz::XmppEngine::ERROR_STREAM) { - const buzz::XmlElement* error_element = client->GetStreamError(); - if (error_element) { - *stream_error = new buzz::XmlElement(*error_element); - } - } -} - -SingleLoginAttempt::SingleLoginAttempt(talk_base::TaskParent* parent, - LoginSettings* login_settings) - : talk_base::Task(parent), - state_(buzz::XmppEngine::STATE_NONE), - code_(buzz::XmppEngine::ERROR_NONE), - subcode_(0), - need_authentication_(false), - certificate_expired_(false), - login_settings_(login_settings), - client_(NULL) { - connection_generator_.reset(new XmppConnectionGenerator( - this, - login_settings_->host_resolver(), - &login_settings_->connection_options(), - login_settings_->try_ssltcp_first(), - login_settings_->proxy_only(), - login_settings_->server_list(), - login_settings_->server_count())); - - connection_generator_->SignalExhaustedSettings.connect( +SingleLoginAttempt::SingleLoginAttempt(LoginSettings* login_settings) + : login_settings_(login_settings), + connection_generator_( + login_settings_->host_resolver(), + &login_settings_->connection_options(), + login_settings_->try_ssltcp_first(), + login_settings_->server_list(), + login_settings_->server_count()) { + connection_generator_.SignalExhaustedSettings.connect( this, &SingleLoginAttempt::OnAttemptedAllConnections); - connection_generator_->SignalNewSettings.connect( + connection_generator_.SignalNewSettings.connect( this, &SingleLoginAttempt::DoLogin); -} - -SingleLoginAttempt::~SingleLoginAttempt() { - // If this assertion goes off, it means that "Stop()" didn't get called like - // it should have been. - DCHECK(!client_); -} - -const talk_base::ProxyInfo& SingleLoginAttempt::proxy() const { - DCHECK(connection_generator_.get()); - return connection_generator_->proxy(); -} - -int SingleLoginAttempt::ProcessStart() { - DCHECK_EQ(GetState(), talk_base::Task::STATE_START); - connection_generator_->StartGenerating(); - - // After being started, this class is callback driven and does signaling from - // those callbacks (with checks to see if it is done if it may be called back - // from something that isn't a child task). - return talk_base::Task::STATE_BLOCKED; -} - -void SingleLoginAttempt::Stop() { - ClearClient(); - talk_base::Task::Stop(); - - // No more signals should happen after being stopped. This is needed because - // some of these signals happen due to other components doing signaling which - // may continue running even though this task is stopped. - SignalUnexpectedDisconnect.disconnect_all(); - SignalRedirect.disconnect_all(); - SignalLoginFailure.disconnect_all(); - SignalNeedAutoReconnect.disconnect_all(); - SignalClientStateChange.disconnect_all(); -} - -void SingleLoginAttempt::OnAttemptedAllConnections( - bool successfully_resolved_dns, - int first_dns_error) { - - // Maybe we needed proxy authentication? - if (need_authentication_) { - LoginFailure failure(LoginFailure::PROXY_AUTHENTICATION_ERROR); - SignalLoginFailure(failure); - return; - } - - if (certificate_expired_) { - LoginFailure failure(LoginFailure::CERTIFICATE_EXPIRED_ERROR); - SignalLoginFailure(failure); - return; - } - - if (!successfully_resolved_dns) { - code_ = buzz::XmppEngine::ERROR_SOCKET; - subcode_ = first_dns_error; - } - - LOG(INFO) << "Connection failed with error " << code_; - - SignalNeedAutoReconnect(); -} - -void SingleLoginAttempt::UseNextConnection() { - DCHECK(connection_generator_.get()); - ClearClient(); - connection_generator_->UseNextConnection(); -} - -void SingleLoginAttempt::UseCurrentConnection() { - DCHECK(connection_generator_.get()); - ClearClient(); - connection_generator_->UseCurrentConnection(); -} - -void SingleLoginAttempt::DoLogin( - const ConnectionSettings& connection_settings) { - if (client_) { - return; - } - // TODO(akalin): Resolve any unresolved IPs, possibly through a - // proxy, instead of skipping them. - if (connection_settings.server().IsUnresolvedIP()) { - // Handler should attempt reconnect. - SignalUnexpectedDisconnect(); - return; - } - - buzz::XmppClientSettings client_settings; - // Set the user settings portion. - *static_cast<buzz::XmppClientSettings*>(&client_settings) = - login_settings_->user_settings(); - // Fill in the rest of the client settings. - connection_settings.FillXmppClientSettings(&client_settings); - client_ = new buzz::XmppClient(this); - SignalLogInput.repeat(client_->SignalLogInput); - SignalLogOutput.repeat(client_->SignalLogOutput); - - // Listen for connection progress. - client_->SignalStateChange.connect(this, - &SingleLoginAttempt::OnClientStateChange); - - // Transition to "start". - OnClientStateChange(buzz::XmppEngine::STATE_START); - // Start connecting. - client_->Connect(client_settings, login_settings_->lang(), - CreateSocket(client_settings), - CreatePreXmppAuth(client_settings)); - client_->Start(); -} - -void SingleLoginAttempt::OnAuthenticationError() { - // We can check this flag later if all connection options fail. - need_authentication_ = true; -} - -void SingleLoginAttempt::OnCertificateExpired() { - // We can check this flag later if all connection options fail. - certificate_expired_ = true; -} - -buzz::AsyncSocket* SingleLoginAttempt::CreateSocket( - const buzz::XmppClientSettings& xcs) { - bool use_fake_ssl_client_socket = - (xcs.protocol() == cricket::PROTO_SSLTCP); - net::ClientSocketFactory* const client_socket_factory = - new XmppClientSocketFactory( - net::ClientSocketFactory::GetDefaultFactory(), - use_fake_ssl_client_socket); - // The default SSLConfig is good enough for us for now. - const net::SSLConfig ssl_config; - // A read buffer of 64k ought to be sufficient. - const size_t kReadBufSize = 64U * 1024U; - // This number was taken from a similar number in - // XmppSocketAdapter. - const size_t kWriteBufSize = 64U * 1024U; - // TODO(akalin): Use a real NetLog. - net::NetLog* const net_log = NULL; - return new ChromeAsyncSocket( - client_socket_factory, ssl_config, - kReadBufSize, kWriteBufSize, net_log); -} - -buzz::PreXmppAuth* SingleLoginAttempt::CreatePreXmppAuth( - const buzz::XmppClientSettings& xcs) { - buzz::Jid jid(xcs.user(), xcs.host(), buzz::STR_EMPTY); - return new GaiaTokenPreXmppAuth( - jid.Str(), xcs.auth_cookie(), xcs.token_service()); + connection_generator_.StartGenerating(); } -void SingleLoginAttempt::OnClientStateChange(buzz::XmppEngine::State state) { - if (state_ == state) - return; - - buzz::XmppEngine::State previous_state = state_; - state_ = state; - - switch (state) { - case buzz::XmppEngine::STATE_NONE: - case buzz::XmppEngine::STATE_START: - case buzz::XmppEngine::STATE_OPENING: - case buzz::XmppEngine::STATE_OPEN: - // Do nothing. - break; - case buzz::XmppEngine::STATE_CLOSED: - OnClientStateChangeClosed(previous_state); - break; - } - SignalClientStateChange(state); - if (state_ == buzz::XmppEngine::STATE_CLOSED) { - OnClientStateChange(buzz::XmppEngine::STATE_NONE); - } -} - -void SingleLoginAttempt::ClearClient() { - if (client_ != NULL) { - client_->Disconnect(); - - // If this assertion goes off, it means that the disconnect didn't occur - // properly. See SingleLoginAttempt::OnClientStateChange, - // case XmppEngine::STATE_CLOSED - DCHECK(!client_); - } -} - -void SingleLoginAttempt::OnClientStateChangeClosed( - buzz::XmppEngine::State previous_state) { - buzz::XmppEngine::Error error = buzz::XmppEngine::ERROR_NONE; - int error_subcode = 0; - buzz::XmlElement* stream_error_ptr; - GetClientErrorInformation(client_, - &error, - &error_subcode, - &stream_error_ptr); - scoped_ptr<buzz::XmlElement> stream_error(stream_error_ptr); - - client_->SignalStateChange.disconnect(this); - client_ = NULL; - - if (error == buzz::XmppEngine::ERROR_NONE) { - SignalLogoff(); - return; - } else if (previous_state == buzz::XmppEngine::STATE_OPEN) { - // Handler should attempt reconnect. - SignalUnexpectedDisconnect(); - return; - } else { - HandleConnectionError(error, error_subcode, stream_error.get()); - } -} +SingleLoginAttempt::~SingleLoginAttempt() {} -void SingleLoginAttempt::HandleConnectionPasswordError() { - LOG(INFO) << "SingleLoginAttempt::HandleConnectionPasswordError"; - LoginFailure failure(LoginFailure::XMPP_ERROR, code_, subcode_); - SignalLoginFailure(failure); +void SingleLoginAttempt::OnConnect(base::WeakPtr<talk_base::Task> base_task) { + SignalConnect(base_task); } -void SingleLoginAttempt::HandleConnectionError( - buzz::XmppEngine::Error code, - int subcode, - const buzz::XmlElement* stream_error) { - LOG(INFO) << "(" << code << ", " << subcode << ")"; - - // Save off the error code information, so we can use it to tell the user - // what went wrong if all else fails. - code_ = code; - subcode_ = subcode; - if ((code_ == buzz::XmppEngine::ERROR_UNAUTHORIZED) || - (code_ == buzz::XmppEngine::ERROR_MISSING_USERNAME)) { - // There was a problem with credentials (username/password). - HandleConnectionPasswordError(); - return; +void SingleLoginAttempt::OnError(buzz::XmppEngine::Error error, int subcode, + const buzz::XmlElement* stream_error) { + LOG(INFO) << "Error: " << error << ", subcode: " << subcode; + if (stream_error) { + DCHECK_EQ(error, buzz::XmppEngine::ERROR_STREAM); + LOG(INFO) << "Stream error: " + << XmlElementToString(*stream_error); } - // Unexpected disconnect, - // Unreachable host, - // Or internal server binding error - - // All these are temporary problems, so continue reconnecting. - - login_settings_->modifiable_user_settings()->set_resource(""); - - // Look for stream::error server redirection stanza "see-other-host". + // Check for redirection. if (stream_error) { const buzz::XmlElement* other = stream_error->FirstNamed(buzz::QN_XSTREAM_SEE_OTHER_HOST); @@ -329,8 +68,9 @@ void SingleLoginAttempt::HandleConnectionError( const buzz::XmlElement* text = stream_error->FirstNamed(buzz::QN_XSTREAM_TEXT); if (text) { - // Yep, its a "stream:error" with "see-other-host" text, let's parse - // out the server:port, and then reconnect with that. + // Yep, its a "stream:error" with "see-other-host" text, + // let's parse out the server:port, and then reconnect + // with that. const std::string& redirect = text->BodyText(); size_t colon = redirect.find(":"); int redirect_port = kDefaultXmppPort; @@ -354,13 +94,42 @@ void SingleLoginAttempt::HandleConnectionError( } } - DCHECK(connection_generator_.get()); - if (!connection_generator_.get()) { + // Iterate to the next possible connection (still trying to connect). + connection_generator_.UseNextConnection(); +} + +void SingleLoginAttempt::OnAttemptedAllConnections( + bool successfully_resolved_dns, + int first_dns_error) { + if (!successfully_resolved_dns) { + LOG(INFO) << "Could not resolve DNS: " << first_dns_error; + } + LOG(INFO) << "Could not connect to any XMPP server"; + SignalNeedAutoReconnect(); +} + +void SingleLoginAttempt::DoLogin( + const ConnectionSettings& connection_settings) { + // TODO(akalin): Resolve any unresolved IPs, possibly through a + // proxy, instead of skipping them. + if (connection_settings.server().IsUnresolvedIP()) { + connection_generator_.UseNextConnection(); return; } - // Iterate to the next possible connection (still trying to connect). - UseNextConnection(); + buzz::XmppClientSettings client_settings = + login_settings_->user_settings(); + // Fill in the rest of the client settings. + connection_settings.FillXmppClientSettings(&client_settings); + + buzz::Jid jid(client_settings.user(), client_settings.host(), + buzz::STR_EMPTY); + buzz::PreXmppAuth* pre_xmpp_auth = + new GaiaTokenPreXmppAuth( + jid.Str(), client_settings.auth_cookie(), + client_settings.token_service()); + xmpp_connection_.reset( + new XmppConnection(client_settings, this, pre_xmpp_auth)); } } // namespace notifier diff --git a/jingle/notifier/communicator/single_login_attempt.h b/jingle/notifier/communicator/single_login_attempt.h index 1b9900c..d2c947b 100644 --- a/jingle/notifier/communicator/single_login_attempt.h +++ b/jingle/notifier/communicator/single_login_attempt.h @@ -7,63 +7,39 @@ #include <string> -#include "jingle/notifier/communicator/login.h" -#include "talk/base/scoped_ptr.h" +#include "base/scoped_ptr.h" +#include "jingle/notifier/base/xmpp_connection.h" +#include "jingle/notifier/communicator/xmpp_connection_generator.h" #include "talk/base/sigslot.h" -#include "talk/base/task.h" #include "talk/xmpp/xmppengine.h" -namespace buzz { -class AsyncSocket; -class PreXmppAuth; -class XmppClient; -class XmppClientSettings; -class XmppClientSettings; -} - namespace talk_base { -class FirewallManager; -struct ProxyInfo; -class SignalThread; class Task; } namespace notifier { class ConnectionSettings; -class LoginFailure; class LoginSettings; -struct ServerInformation; class XmppConnectionGenerator; +class XmppConnection; // Handles all of the aspects of a single login attempt (across multiple ip // addresses) and maintainence. By containing this within one class, when // another login attempt is made, this class will be disposed and all of the // signalling for the previous login attempt will be cleaned up immediately. -// -// This is a task to allow for cleaning this up when a signal is being fired. -// Technically, delete this during the firing of a signal could work but it is -// fragile. -class SingleLoginAttempt : public talk_base::Task, public sigslot::has_slots<> { +class SingleLoginAttempt : public XmppConnection::Delegate, + public sigslot::has_slots<> { public: - SingleLoginAttempt(talk_base::TaskParent* parent, - LoginSettings* login_settings); - ~SingleLoginAttempt(); - virtual int ProcessStart(); - void UseNextConnection(); - void UseCurrentConnection(); - - buzz::XmppClient* xmpp_client() { - return client_; - } - - // Returns the proxy that is being used to connect (or the default proxy - // information if all attempted connections failed). - const talk_base::ProxyInfo& proxy() const; - - // Typically handled by creating a new SingleLoginAttempt and doing - // StartConnection. - sigslot::signal0<> SignalUnexpectedDisconnect; + explicit SingleLoginAttempt(LoginSettings* login_settings); + + virtual ~SingleLoginAttempt(); + + virtual void OnConnect(base::WeakPtr<talk_base::Task> parent); + + virtual void OnError(buzz::XmppEngine::Error error, + int error_subcode, + const buzz::XmlElement* stream_error); // Typically handled by storing the redirect for 5 seconds, and setting it // into LoginSettings, then creating a new SingleLoginAttempt, and doing @@ -74,52 +50,17 @@ class SingleLoginAttempt : public talk_base::Task, public sigslot::has_slots<> { sigslot::signal0<> SignalNeedAutoReconnect; - // SignalClientStateChange(buzz::XmppEngine::State new_state); - sigslot::signal1<buzz::XmppEngine::State> SignalClientStateChange; - - // See the LoginFailure for how to handle this. - sigslot::signal1<const LoginFailure&> SignalLoginFailure; - - // Sent when there is a graceful log-off (state goes to closed with no - // error). - sigslot::signal0<> SignalLogoff; - - sigslot::repeater2<const char*, int> SignalLogInput; - sigslot::repeater2<const char*, int> SignalLogOutput; - - protected: - virtual void Stop(); + sigslot::signal1<base::WeakPtr<talk_base::Task> > SignalConnect; private: void DoLogin(const ConnectionSettings& connection_settings); - buzz::AsyncSocket* CreateSocket(const buzz::XmppClientSettings& xcs); - static buzz::PreXmppAuth* CreatePreXmppAuth( - const buzz::XmppClientSettings& xcs); - - // Cleans up any xmpp client state to get ready for a new one. - void ClearClient(); - - void HandleConnectionError( - buzz::XmppEngine::Error code, - int subcode, - const buzz::XmlElement* stream_error); - void HandleConnectionPasswordError(); - - void OnAuthenticationError(); - void OnCertificateExpired(); - void OnClientStateChange(buzz::XmppEngine::State state); - void OnClientStateChangeClosed(buzz::XmppEngine::State previous_state); + void OnAttemptedAllConnections(bool successfully_resolved_dns, int first_dns_error); - buzz::XmppEngine::State state_; - buzz::XmppEngine::Error code_; - int subcode_; - bool need_authentication_; - bool certificate_expired_; LoginSettings* login_settings_; - buzz::XmppClient* client_; - scoped_ptr<XmppConnectionGenerator> connection_generator_; + XmppConnectionGenerator connection_generator_; + scoped_ptr<XmppConnection> xmpp_connection_; DISALLOW_COPY_AND_ASSIGN(SingleLoginAttempt); }; diff --git a/jingle/notifier/communicator/xmpp_connection_generator.cc b/jingle/notifier/communicator/xmpp_connection_generator.cc index d4a9c79..0a78fef 100644 --- a/jingle/notifier/communicator/xmpp_connection_generator.cc +++ b/jingle/notifier/communicator/xmpp_connection_generator.cc @@ -37,11 +37,9 @@ namespace notifier { XmppConnectionGenerator::XmppConnectionGenerator( - talk_base::Task* parent, const scoped_refptr<net::HostResolver>& host_resolver, const ConnectionOptions* options, bool try_ssltcp_first, - bool proxy_only, const ServerInformation* server_list, int server_count) : host_resolver_(host_resolver), @@ -55,13 +53,10 @@ XmppConnectionGenerator::XmppConnectionGenerator( server_count_(server_count), server_index_(-1), try_ssltcp_first_(try_ssltcp_first), - proxy_only_(proxy_only), successfully_resolved_dns_(false), first_dns_error_(0), - options_(options), - parent_(parent) { + options_(options) { DCHECK(host_resolver); - DCHECK(parent); DCHECK(options); DCHECK_GT(server_count_, 0); for (int i = 0; i < server_count_; ++i) { @@ -73,16 +68,6 @@ XmppConnectionGenerator::~XmppConnectionGenerator() { LOG(INFO) << "XmppConnectionGenerator::~XmppConnectionGenerator"; } -const talk_base::ProxyInfo& XmppConnectionGenerator::proxy() const { - DCHECK(settings_list_.get()); - if (settings_index_ >= settings_list_->GetCount()) { - return settings_list_->proxy(); - } - - ConnectionSettings* settings = settings_list_->GetSettings(settings_index_); - return settings->proxy(); -} - // Starts resolving proxy information. void XmppConnectionGenerator::StartGenerating() { LOG(INFO) << "XmppConnectionGenerator::StartGenerating"; @@ -178,8 +163,7 @@ void XmppConnectionGenerator::HandleServerDNSResolved(int status) { ip_list, server_list_[server_index_].server.port(), server_list_[server_index_].special_port_magic, - try_ssltcp_first_, - proxy_only_); + try_ssltcp_first_); } static const char* const PROTO_NAMES[cricket::PROTO_LAST + 1] = { @@ -197,10 +181,7 @@ void XmppConnectionGenerator::UseCurrentConnection() { LOG(INFO) << "*** Attempting " << ProtocolToString(settings->protocol()) << " connection to " << settings->server().IPAsString() << ":" - << settings->server().port() - << " (via " << ProxyToString(settings->proxy().type) - << " proxy @ " << settings->proxy().address.IPAsString() << ":" - << settings->proxy().address.port() << ")"; + << settings->server().port(); SignalNewSettings(*settings); } diff --git a/jingle/notifier/communicator/xmpp_connection_generator.h b/jingle/notifier/communicator/xmpp_connection_generator.h index 8a31bb6..26b040d 100644 --- a/jingle/notifier/communicator/xmpp_connection_generator.h +++ b/jingle/notifier/communicator/xmpp_connection_generator.h @@ -19,7 +19,6 @@ namespace talk_base { struct ProxyInfo; class SignalThread; -class Task; } namespace notifier { @@ -37,18 +36,13 @@ struct ServerInformation { // combinations. class XmppConnectionGenerator : public sigslot::has_slots<> { public: - // parent is the parent for any tasks needed during this operation. // try_ssltcp_first indicates that SSLTCP is tried before XMPP. Used by tests. - // proxy_only indicates if true connections are only attempted using the - // proxy. // server_list is the list of connections to attempt in priority order. // server_count is the number of items in the server list. XmppConnectionGenerator( - talk_base::Task* parent, const scoped_refptr<net::HostResolver>& host_resolver, const ConnectionOptions* options, bool try_ssltcp_first, - bool proxy_only, const ServerInformation* server_list, int server_count); ~XmppConnectionGenerator(); @@ -60,8 +54,6 @@ class XmppConnectionGenerator : public sigslot::has_slots<> { void UseNextConnection(); void UseCurrentConnection(); - const talk_base::ProxyInfo& proxy() const; - sigslot::signal1<const ConnectionSettings&> SignalNewSettings; // SignalExhaustedSettings(bool successfully_resolved_dns, @@ -83,12 +75,10 @@ class XmppConnectionGenerator : public sigslot::has_slots<> { int server_count_; int server_index_; // The server that is current being used. bool try_ssltcp_first_; // Used when sync tests are run on chromium builders. - bool proxy_only_; bool successfully_resolved_dns_; int first_dns_error_; const ConnectionOptions* options_; - talk_base::Task* parent_; DISALLOW_COPY_AND_ASSIGN(XmppConnectionGenerator); }; diff --git a/jingle/notifier/listener/mediator_thread_impl.cc b/jingle/notifier/listener/mediator_thread_impl.cc index 918b49b..aea38e7 100644 --- a/jingle/notifier/listener/mediator_thread_impl.cc +++ b/jingle/notifier/listener/mediator_thread_impl.cc @@ -34,6 +34,12 @@ MediatorThreadImpl::MediatorThreadImpl(const NotifierOptions& notifier_options) MediatorThreadImpl::~MediatorThreadImpl() { DCHECK_EQ(MessageLoop::current(), parent_message_loop_); + // If the worker thread is still around, we need to call Logout() so + // that all the variables living it get destroyed properly (i.e., on + // the worker thread). + if (worker_thread_.IsRunning()) { + Logout(); + } } void MediatorThreadImpl::SetDelegate(Delegate* delegate) { @@ -73,7 +79,6 @@ void MediatorThreadImpl::Logout() { parent_message_loop_->SetNestableTasksAllowed(old_state); // worker_thread_ should have cleaned all this up. CHECK(!login_.get()); - CHECK(!pump_.get()); } void MediatorThreadImpl::ListenForUpdates() { @@ -111,29 +116,20 @@ MessageLoop* MediatorThreadImpl::worker_message_loop() { return worker_message_loop; } -buzz::XmppClient* MediatorThreadImpl::xmpp_client() { - DCHECK_EQ(MessageLoop::current(), worker_message_loop()); - DCHECK(login_.get()); - buzz::XmppClient* xmpp_client = login_->xmpp_client(); - DCHECK(xmpp_client); - return xmpp_client; -} void MediatorThreadImpl::DoLogin( const buzz::XmppClientSettings& settings) { DCHECK_EQ(MessageLoop::current(), worker_message_loop()); LOG(INFO) << "P2P: Thread logging into talk network."; + base_task_.reset(); + // TODO(akalin): Use an existing HostResolver from somewhere (maybe // the IOThread one). host_resolver_ = net::CreateSystemHostResolver(net::HostResolver::kDefaultParallelism, NULL); - // Start a new pump for the login. - login_.reset(); - pump_.reset(new notifier::TaskPump()); - notifier::ServerInformation server_list[2]; int server_list_count = 0; @@ -157,26 +153,17 @@ void MediatorThreadImpl::DoLogin( // Autodetect proxy is on by default. notifier::ConnectionOptions options; - // Language is not used in the stanza so we default to |en|. - std::string lang = "en"; - login_.reset(new notifier::Login(pump_.get(), - settings, + login_.reset(new notifier::Login(settings, options, - lang, host_resolver_.get(), server_list, server_list_count, - // talk_base::FirewallManager* is NULL. - NULL, - notifier_options_.try_ssltcp_first, - // Both the proxy and a non-proxy route - // will be attempted. - false)); - - login_->SignalClientStateChange.connect( - this, &MediatorThreadImpl::OnClientStateChangeMessage); - login_->SignalLoginFailure.connect( - this, &MediatorThreadImpl::OnLoginFailureMessage); + notifier_options_.try_ssltcp_first)); + + login_->SignalConnect.connect( + this, &MediatorThreadImpl::OnConnect); + login_->SignalDisconnect.connect( + this, &MediatorThreadImpl::OnDisconnect); login_->StartConnection(); } @@ -184,18 +171,19 @@ void MediatorThreadImpl::DoDisconnect() { DCHECK_EQ(MessageLoop::current(), worker_message_loop()); LOG(INFO) << "P2P: Thread logging out of talk network."; login_.reset(); - // Delete the old pump while on the thread to ensure that everything is - // cleaned-up in a predicatable manner. - pump_.reset(); - host_resolver_ = NULL; + base_task_.reset(); } void MediatorThreadImpl::DoSubscribeForUpdates( const std::vector<std::string>& subscribed_services_list) { DCHECK_EQ(MessageLoop::current(), worker_message_loop()); + if (!base_task_.get()) { + return; + } + // Owned by |base_task_|. SubscribeTask* subscription = - new SubscribeTask(xmpp_client(), subscribed_services_list); + new SubscribeTask(base_task_, subscribed_services_list); subscription->SignalStatusUpdate.connect( this, &MediatorThreadImpl::OnSubscriptionStateChange); @@ -204,7 +192,11 @@ void MediatorThreadImpl::DoSubscribeForUpdates( void MediatorThreadImpl::DoListenForUpdates() { DCHECK_EQ(MessageLoop::current(), worker_message_loop()); - ListenTask* listener = new ListenTask(xmpp_client()); + if (!base_task_.get()) { + return; + } + // Owned by |base_task_|. + ListenTask* listener = new ListenTask(base_task_); listener->SignalUpdateAvailable.connect( this, &MediatorThreadImpl::OnIncomingNotification); @@ -214,7 +206,11 @@ void MediatorThreadImpl::DoListenForUpdates() { void MediatorThreadImpl::DoSendNotification( const OutgoingNotificationData& data) { DCHECK_EQ(MessageLoop::current(), worker_message_loop()); - SendUpdateTask* task = new SendUpdateTask(xmpp_client(), data); + if (!base_task_.get()) { + return; + } + // Owned by |base_task_|. + SendUpdateTask* task = new SendUpdateTask(base_task_, data); task->SignalStatusUpdate.connect( this, &MediatorThreadImpl::OnOutgoingNotification); @@ -258,57 +254,37 @@ void MediatorThreadImpl::OnOutgoingNotificationOnParentThread( } } -void MediatorThreadImpl::OnLoginFailureMessage( - const notifier::LoginFailure& failure) { +void MediatorThreadImpl::OnConnect(base::WeakPtr<talk_base::Task> base_task) { DCHECK_EQ(MessageLoop::current(), worker_message_loop()); + base_task_ = base_task; parent_message_loop_->PostTask( FROM_HERE, NewRunnableMethod( this, - &MediatorThreadImpl::OnLoginFailureMessageOnParentThread, - failure)); + &MediatorThreadImpl::OnConnectOnParentThread)); } -void MediatorThreadImpl::OnLoginFailureMessageOnParentThread( - const notifier::LoginFailure& failure) { +void MediatorThreadImpl::OnConnectOnParentThread() { DCHECK_EQ(MessageLoop::current(), parent_message_loop_); if (delegate_) { - delegate_->OnConnectionStateChange(false); + delegate_->OnConnectionStateChange(true); } } -void MediatorThreadImpl::OnClientStateChangeMessage( - LoginConnectionState state) { +void MediatorThreadImpl::OnDisconnect() { DCHECK_EQ(MessageLoop::current(), worker_message_loop()); + base_task_.reset(); parent_message_loop_->PostTask( FROM_HERE, NewRunnableMethod( this, - &MediatorThreadImpl::OnClientStateChangeMessageOnParentThread, - state)); + &MediatorThreadImpl::OnDisconnectOnParentThread)); } -void MediatorThreadImpl::OnClientStateChangeMessageOnParentThread( - LoginConnectionState state) { +void MediatorThreadImpl::OnDisconnectOnParentThread() { DCHECK_EQ(MessageLoop::current(), parent_message_loop_); - switch (state) { - case STATE_DISCONNECTED: - LOG(INFO) << "P2P: Thread trying to connect."; - // Maybe first time logon, maybe intermediate network disruption. Assume - // the server went down, and lost our subscription for updates. - if (delegate_) { - delegate_->OnConnectionStateChange(false); - delegate_->OnSubscriptionStateChange(false); - } - break; - case STATE_CONNECTED: - if (delegate_) { - delegate_->OnConnectionStateChange(true); - } - break; - default: - LOG(WARNING) << "P2P: Unknown client state change."; - break; + if (delegate_) { + delegate_->OnConnectionStateChange(false); } } diff --git a/jingle/notifier/listener/mediator_thread_impl.h b/jingle/notifier/listener/mediator_thread_impl.h index 342079f..f329ee5 100644 --- a/jingle/notifier/listener/mediator_thread_impl.h +++ b/jingle/notifier/listener/mediator_thread_impl.h @@ -29,8 +29,6 @@ #include "base/thread.h" #include "jingle/notifier/base/notifier_options.h" #include "jingle/notifier/communicator/login.h" -#include "jingle/notifier/communicator/login_connection_state.h" -#include "jingle/notifier/communicator/login_failure.h" #include "jingle/notifier/listener/mediator_thread.h" #include "talk/base/sigslot.h" #include "talk/xmpp/xmppclientsettings.h" @@ -81,19 +79,13 @@ class MediatorThreadImpl // Should only be called after Start(). MessageLoop* worker_message_loop(); - // Should only be called after OnConnectionStateChange() is called - // on the delegate with true and before it is called with false. - buzz::XmppClient* xmpp_client(); - - // This is virtual so that subclasses can also know when the - // connection state changes. - virtual void OnClientStateChangeMessage(LoginConnectionState state); + virtual void OnDisconnect(); Delegate* delegate_; MessageLoop* parent_message_loop_; + base::WeakPtr<talk_base::Task> base_task_; private: - // Called from within the thread on internal events. void DoLogin(const buzz::XmppClientSettings& settings); void DoDisconnect(); void DoSubscribeForUpdates( @@ -107,17 +99,15 @@ class MediatorThreadImpl void OnIncomingNotification( const IncomingNotificationData& notification_data); void OnOutgoingNotification(bool success); - void OnLoginFailureMessage(const notifier::LoginFailure& failure); + void OnConnect(base::WeakPtr<talk_base::Task> parent); void OnSubscriptionStateChange(bool success); // Equivalents of the above functions called from the parent thread. void OnIncomingNotificationOnParentThread( const IncomingNotificationData& notification_data); void OnOutgoingNotificationOnParentThread(bool success); - void OnLoginFailureMessageOnParentThread( - const notifier::LoginFailure& failure); - void OnClientStateChangeMessageOnParentThread( - LoginConnectionState state); + void OnConnectOnParentThread(); + void OnDisconnectOnParentThread(); void OnSubscriptionStateChangeOnParentThread( bool success); @@ -126,11 +116,6 @@ class MediatorThreadImpl base::Thread worker_thread_; scoped_refptr<net::HostResolver> host_resolver_; - // All buzz::XmppClients are owned by their parent. The root parent is the - // SingleLoginTask created by the notifier::Login object. This in turn is - // owned by the TaskPump. They are destroyed either when processing is - // complete or the pump shuts down. - scoped_ptr<notifier::TaskPump> pump_; scoped_ptr<notifier::Login> login_; DISALLOW_COPY_AND_ASSIGN(MediatorThreadImpl); |