From d508ac6ac41918d2c831b47b29761980989dd7f6 Mon Sep 17 00:00:00 2001 From: Tobias Markmann <tm@ayena.de> Date: Mon, 9 May 2016 11:22:29 +0200 Subject: Fix handling of incorrect MUC component behavior Swift used to crash when a MUC component returned multiple unavailable presences on rejoin of a room hosted on a restarting buggy MUC component. Test-Information: Added test case that used to crash Swift. Tests pass without crash on OS X 10.11.4 Change-Id: I52280976944170c6e143197d4b3dc517dc13ecbb diff --git a/Swift/Controllers/Chat/UnitTest/ChatsManagerTest.cpp b/Swift/Controllers/Chat/UnitTest/ChatsManagerTest.cpp index e45bcae..31c9be9 100644 --- a/Swift/Controllers/Chat/UnitTest/ChatsManagerTest.cpp +++ b/Swift/Controllers/Chat/UnitTest/ChatsManagerTest.cpp @@ -89,6 +89,7 @@ class ChatsManagerTest : public CppUnit::TestFixture { CPPUNIT_TEST(testChatControllerHighlightingNotificationTesting); CPPUNIT_TEST(testChatControllerHighlightingNotificationDeduplicateSounds); CPPUNIT_TEST(testChatControllerMeMessageHandling); + CPPUNIT_TEST(testRestartingMUCComponentCrash); CPPUNIT_TEST(testChatControllerMeMessageHandlingInMUC); // Carbons tests @@ -822,6 +823,54 @@ public: CPPUNIT_ASSERT_EQUAL(std::string("is feeling delighted."), window->bodyFromMessage(window->lastAddedAction_)); } + void testRestartingMUCComponentCrash() { + JID mucJID = JID("teaparty@rooms.wonderland.lit"); + JID self = JID("girl@wonderland.lit/rabbithole"); + std::string nick = "aLiCe"; + + MockChatWindow* window; + + auto genRemoteMUCPresence = [=]() { + auto presence = Presence::create(); + presence->setFrom(mucJID.withResource(nick)); + presence->setTo(self); + return presence; + }; + + // User rejoins. + window = new MockChatWindow(); + mocks_->ExpectCall(chatWindowFactory_, ChatWindowFactory::createChatWindow).With(mucJID, uiEventStream_).Return(window); + + // Join room + { + auto joinRoomEvent = std::make_shared<JoinMUCUIEvent>(mucJID, boost::optional<std::string>(), nick); + uiEventStream_->send(joinRoomEvent); + } + + { + auto firstPresence = genRemoteMUCPresence(); + firstPresence->setType(Presence::Unavailable); + auto userPayload = std::make_shared<MUCUserPayload>(); + userPayload->addItem(MUCItem(MUCOccupant::Owner, MUCOccupant::NoRole)); + firstPresence->addPayload(userPayload); + stanzaChannel_->onPresenceReceived(firstPresence); + } + CPPUNIT_ASSERT_EQUAL(std::string("Couldn't enter room: Unable to enter this room."), MockChatWindow::bodyFromMessage(window->lastAddedErrorMessage_)); + + { + auto presence = genRemoteMUCPresence(); + presence->setType(Presence::Unavailable); + auto userPayload = std::make_shared<MUCUserPayload>(); + userPayload->addStatusCode(303); + auto item = MUCItem(MUCOccupant::Owner, self, MUCOccupant::Moderator); + item.nick = nick; + userPayload->addItem(item); + userPayload->addStatusCode(110); + presence->addPayload(userPayload); + stanzaChannel_->onPresenceReceived(presence); + } + } + void testChatControllerMeMessageHandlingInMUC() { JID mucJID("mucroom@rooms.test.com"); std::string nickname = "toodles"; diff --git a/Swift/Controllers/UnitTest/MockChatWindow.h b/Swift/Controllers/UnitTest/MockChatWindow.h index d7942ff..054cd31 100644 --- a/Swift/Controllers/UnitTest/MockChatWindow.h +++ b/Swift/Controllers/UnitTest/MockChatWindow.h @@ -40,7 +40,9 @@ namespace Swift { lastAddedPresence_ = message; } - virtual void addErrorMessage(const ChatMessage& /*message*/) {} + virtual void addErrorMessage(const ChatMessage& message) { + lastAddedErrorMessage_ = message; + } virtual void replaceMessage(const ChatMessage& /*message*/, const std::string& /*id*/, const boost::posix_time::ptime& /*time*/) {} virtual void replaceWithAction(const ChatMessage& /*message*/, const std::string& /*id*/, const boost::posix_time::ptime& /*time*/) {} virtual void replaceLastMessage(const ChatMessage& message, const TimestampBehaviour /*timestampBehaviour*/) { @@ -134,6 +136,7 @@ namespace Swift { ChatMessage lastReplacedMessage_; ChatMessage lastAddedSystemMessage_; ChatMessage lastReplacedSystemMessage_; + ChatMessage lastAddedErrorMessage_; JID lastMUCInvitationJID_; std::vector<SecurityLabelsCatalog::Item> labels_; bool labelsEnabled_; diff --git a/Swiften/MUC/MUCImpl.cpp b/Swiften/MUC/MUCImpl.cpp index 4bab39d..ba8d03e 100644 --- a/Swiften/MUC/MUCImpl.cpp +++ b/Swiften/MUC/MUCImpl.cpp @@ -10,7 +10,6 @@ #include <boost/bind.hpp> -#include <Swiften/Base/foreach.h> #include <Swiften/Client/StanzaChannel.h> #include <Swiften/Elements/CapsInfo.h> #include <Swiften/Elements/Form.h> @@ -138,7 +137,7 @@ void MUCImpl::handleIncomingPresence(Presence::ref presence) { } MUCUserPayload::ref mucPayload; - foreach (MUCUserPayload::ref payload, presence->getPayloads<MUCUserPayload>()) { + for (MUCUserPayload::ref payload : presence->getPayloads<MUCUserPayload>()) { if (!payload->getItems().empty() || !payload->getStatusCodes().empty()) { mucPayload = payload; } @@ -148,11 +147,10 @@ void MUCImpl::handleIncomingPresence(Presence::ref presence) { // (i.e. we start getting non-error presence from the MUC) or not if (!joinSucceeded_) { if (presence->getType() == Presence::Error) { - std::string reason; onJoinFailed(presence->getPayload<ErrorPayload>()); return; } - else { + else if (presence->getType() == Presence::Available) { joinSucceeded_ = true; presenceSender->addDirectedPresenceReceiver(ownMUCJID, DirectedPresenceSender::DontSendPresence); if (presenceSender->getLastSentUndirectedPresence() && !isEqualExceptID(**(presenceSender->getLastSentUndirectedPresence()), *joinRequestPresence_)) { @@ -162,6 +160,10 @@ void MUCImpl::handleIncomingPresence(Presence::ref presence) { presenceSender->sendPresence(latestPresence); } } + else if (presence->getType() == Presence::Unavailable) { + onJoinFailed(std::shared_ptr<ErrorPayload>()); + return; + } } std::string nick = presence->getFrom().getResource(); @@ -188,19 +190,21 @@ void MUCImpl::handleIncomingPresence(Presence::ref presence) { if (std::dynamic_pointer_cast<MUCDestroyPayload>(mucPayload->getPayload())) { type = LeaveDestroy; } - else foreach (MUCUserPayload::StatusCode status, mucPayload->getStatusCodes()) { - if (status.code == 307) { - type = LeaveKick; - } - else if (status.code == 301) { - type = LeaveBan; - } - else if (status.code == 321) { - type = LeaveNotMember; - } - else if (status.code == 303) { - if (mucPayload->getItems().size() == 1) { - newNickname = mucPayload->getItems()[0].nick; + else { + for (MUCUserPayload::StatusCode status : mucPayload->getStatusCodes()) { + if (status.code == 307) { + type = LeaveKick; + } + else if (status.code == 301) { + type = LeaveBan; + } + else if (status.code == 321) { + type = LeaveNotMember; + } + else if (status.code == 303) { + if (mucPayload->getItems().size() == 1) { + newNickname = mucPayload->getItems()[0].nick; + } } } } @@ -254,50 +258,53 @@ void MUCImpl::handleIncomingPresence(Presence::ref presence) { onOccupantJoined(result.first->second); } onOccupantPresenceChange(presence); - } - if (mucPayload && !joinComplete_) { - bool isLocked = false; - foreach (MUCUserPayload::StatusCode status, mucPayload->getStatusCodes()) { - if (status.code == 110) { - /* Simply knowing this is your presence is enough, 210 doesn't seem to be necessary. */ - joinComplete_ = true; - if (ownMUCJID != presence->getFrom()) { - presenceSender->removeDirectedPresenceReceiver(ownMUCJID, DirectedPresenceSender::DontSendPresence); - ownMUCJID = presence->getFrom(); - presenceSender->addDirectedPresenceReceiver(ownMUCJID, DirectedPresenceSender::AndSendPresence); - } - } - // MUC status 201: a new room has been created - if (status.code == 201) { - isLocked = true; - /* Room is created and locked */ - /* Currently deal with this by making an instant room */ - if (ownMUCJID != presence->getFrom()) { - presenceSender->removeDirectedPresenceReceiver(ownMUCJID, DirectedPresenceSender::DontSendPresence); - ownMUCJID = presence->getFrom(); - presenceSender->addDirectedPresenceReceiver(ownMUCJID, DirectedPresenceSender::AndSendPresence); - } - if (createAsReservedIfNew) { - unlocking = true; - requestConfigurationForm(); + + if (mucPayload && !joinComplete_) { + bool isLocked = false; + for (MUCUserPayload::StatusCode status : mucPayload->getStatusCodes()) { + if (status.code == 110) { + /* Simply knowing this is your presence is enough, 210 doesn't seem to be necessary. */ + joinComplete_ = true; + if (ownMUCJID != presence->getFrom()) { + presenceSender->removeDirectedPresenceReceiver(ownMUCJID, DirectedPresenceSender::DontSendPresence); + ownMUCJID = presence->getFrom(); + presenceSender->addDirectedPresenceReceiver(ownMUCJID, DirectedPresenceSender::AndSendPresence); + } } - else { - // Accept default room configuration and create an instant room http://xmpp.org/extensions/xep-0045.html#createroom-instant - MUCOwnerPayload::ref mucPayload(new MUCOwnerPayload()); - presenceSender->addDirectedPresenceReceiver(ownMUCJID, DirectedPresenceSender::DontSendPresence); - mucPayload->setPayload(std::make_shared<Form>(Form::SubmitType)); - std::shared_ptr< GenericRequest<MUCOwnerPayload> > request = std::make_shared< GenericRequest<MUCOwnerPayload> >(IQ::Set, getJID(), mucPayload, iqRouter_); - request->onResponse.connect(boost::bind(&MUCImpl::handleCreationConfigResponse, this, _1, _2)); - request->send(); + // MUC status 201: a new room has been created + if (status.code == 201) { + isLocked = true; + /* Room is created and locked */ + /* Currently deal with this by making an instant room */ + if (ownMUCJID != presence->getFrom()) { + presenceSender->removeDirectedPresenceReceiver(ownMUCJID, DirectedPresenceSender::DontSendPresence); + ownMUCJID = presence->getFrom(); + presenceSender->addDirectedPresenceReceiver(ownMUCJID, DirectedPresenceSender::AndSendPresence); + } + if (createAsReservedIfNew) { + unlocking = true; + requestConfigurationForm(); + } + else { + // Accept default room configuration and create an instant room http://xmpp.org/extensions/xep-0045.html#createroom-instant + MUCOwnerPayload::ref mucPayload(new MUCOwnerPayload()); + presenceSender->addDirectedPresenceReceiver(ownMUCJID, DirectedPresenceSender::DontSendPresence); + mucPayload->setPayload(std::make_shared<Form>(Form::SubmitType)); + std::shared_ptr< GenericRequest<MUCOwnerPayload> > request = std::make_shared< GenericRequest<MUCOwnerPayload> >(IQ::Set, getJID(), mucPayload, iqRouter_); + request->onResponse.connect(boost::bind(&MUCImpl::handleCreationConfigResponse, this, _1, _2)); + request->send(); + } } } - } - if (joinComplete_ && !isLocked) { - onJoinComplete(getOwnNick()); - } - if (!isLocked && !isUnlocked_ && (presence->getFrom() == ownMUCJID)) { - isUnlocked_ = true; - onUnlocked(); + + if (joinComplete_ && !isLocked) { + assert(hasOccupant(getOwnNick())); + onJoinComplete(getOwnNick()); + } + if (!isLocked && !isUnlocked_ && (presence->getFrom() == ownMUCJID)) { + isUnlocked_ = true; + onUnlocked(); + } } } } @@ -377,7 +384,7 @@ void MUCImpl::handleAffiliationListResponse(MUCAdminPayload::ref payload, ErrorP } else { std::vector<JID> jids; - foreach (MUCItem item, payload->getItems()) { + for (MUCItem item : payload->getItems()) { if (item.realJID) { jids.push_back(*item.realJID); } -- cgit v0.10.2-6-g49f6