From cb706cb9d9f35ebbfd884e44437c7929aa4af157 Mon Sep 17 00:00:00 2001 From: samcake Date: Thu, 19 Apr 2018 17:24:04 -0700 Subject: [PATCH 01/14] Reapply extra sanity check that hasn;t been applied correctly in rc66 --- libraries/render/src/render/Item.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/libraries/render/src/render/Item.cpp b/libraries/render/src/render/Item.cpp index ed052adf6e..9c5efb9fa7 100644 --- a/libraries/render/src/render/Item.cpp +++ b/libraries/render/src/render/Item.cpp @@ -118,9 +118,15 @@ uint32_t Item::fetchMetaSubItemBounds(ItemBounds& subItemBounds, Scene& scene) c auto numSubs = fetchMetaSubItems(subItems); for (auto id : subItems) { - auto& item = scene.getItem(id); - if (item.exist()) { - subItemBounds.emplace_back(id, item.getBound()); + // TODO: Adding an extra check here even thought we shouldn't have too. + // We have cases when the id returned by fetchMetaSubItems is not allocated + if (scene.isAllocatedID(id)) { + auto& item = scene.getItem(id); + if (item.exist()) { + subItemBounds.emplace_back(id, item.getBound()); + } else { + numSubs--; + } } else { numSubs--; } From a930e39bd434d6d7268a0ea55ccbeebb66e8bfb4 Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Wed, 28 Mar 2018 15:05:51 -0700 Subject: [PATCH 02/14] Fix nodelist connections not resetting on both ends This change adds a connection ID to ping packets. Each node keeps a connection id for each other node that it has connected to. When a node is removed the connection id is incremented. If a node sees another node with a higher connection id, it will reset its connection with the new connection id, ensuring that local state is reset on both ends when nodes lose contact. --- domain-server/src/DomainGatekeeper.cpp | 12 ++++++--- libraries/networking/src/LimitedNodeList.cpp | 27 +++++++++++++++----- libraries/networking/src/LimitedNodeList.h | 11 +++++--- libraries/networking/src/NodeList.cpp | 24 ++++++++++++++--- 4 files changed, 58 insertions(+), 16 deletions(-) diff --git a/domain-server/src/DomainGatekeeper.cpp b/domain-server/src/DomainGatekeeper.cpp index 7d0b538f6e..9f24036e92 100644 --- a/domain-server/src/DomainGatekeeper.cpp +++ b/domain-server/src/DomainGatekeeper.cpp @@ -451,11 +451,12 @@ SharedNodePointer DomainGatekeeper::processAgentConnectRequest(const NodeConnect return SharedNodePointer(); } - QUuid hintNodeID; + QUuid existingNodeID; // in case this is a node that's failing to connect // double check we don't have the same node whose sockets match exactly already in the list limitedNodeList->eachNodeBreakable([&](const SharedNodePointer& node){ + if (node->getPublicSocket() == nodeConnection.publicSockAddr && node->getLocalSocket() == nodeConnection.localSockAddr) { // we have a node that already has these exact sockets - this can occur if a node // is failing to connect to the domain @@ -465,15 +466,20 @@ SharedNodePointer DomainGatekeeper::processAgentConnectRequest(const NodeConnect auto existingNodeData = static_cast(node->getLinkedData()); if (existingNodeData->getUsername() == username) { - hintNodeID = node->getUUID(); + qDebug() << "Deleting existing connection from same sockaddr: " << node->getUUID(); + existingNodeID = node->getUUID(); return false; } } return true; }); + if (!existingNodeID.isNull()) { + limitedNodeList->killNodeWithUUID(existingNodeID); + } + // add the connecting node (or re-use the matched one from eachNodeBreakable above) - SharedNodePointer newNode = addVerifiedNodeFromConnectRequest(nodeConnection, hintNodeID); + SharedNodePointer newNode = addVerifiedNodeFromConnectRequest(nodeConnection); // set the edit rights for this user newNode->setPermissions(userPerms); diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index 0803e380f2..e27e2d6d08 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -578,9 +578,10 @@ void LimitedNodeList::reset() { // we need to make sure any socket connections are gone so wait on that here _nodeSocket.clearConnections(); + _connectionIDs.clear(); } -bool LimitedNodeList::killNodeWithUUID(const QUuid& nodeUUID) { +bool LimitedNodeList::killNodeWithUUID(const QUuid& nodeUUID, ConnectionID newConnectionID) { QReadLocker readLocker(&_nodeMutex); NodeHash::iterator it = _nodeHash.find(nodeUUID); @@ -594,7 +595,7 @@ bool LimitedNodeList::killNodeWithUUID(const QUuid& nodeUUID) { _nodeHash.unsafe_erase(it); } - handleNodeKill(matchingNode); + handleNodeKill(matchingNode, newConnectionID); return true; } @@ -609,7 +610,7 @@ void LimitedNodeList::processKillNode(ReceivedMessage& message) { killNodeWithUUID(nodeUUID); } -void LimitedNodeList::handleNodeKill(const SharedNodePointer& node) { +void LimitedNodeList::handleNodeKill(const SharedNodePointer& node, ConnectionID nextConnectionID) { qCDebug(networking) << "Killed" << *node; node->stopPingTimer(); emit nodeKilled(node); @@ -617,6 +618,15 @@ void LimitedNodeList::handleNodeKill(const SharedNodePointer& node) { if (auto activeSocket = node->getActiveSocket()) { _nodeSocket.cleanupConnection(*activeSocket); } + + auto it = _connectionIDs.find(node->getUUID()); + if (it != _connectionIDs.end()) { + if (nextConnectionID == NULL_CONNECTION_ID) { + it->second++; + } else { + it->second = nextConnectionID; + } + } } SharedNodePointer LimitedNodeList::addOrUpdateNode(const QUuid& uuid, NodeType_t nodeType, @@ -638,6 +648,11 @@ SharedNodePointer LimitedNodeList::addOrUpdateNode(const QUuid& uuid, NodeType_t return matchingNode; } else { + auto it = _connectionIDs.find(uuid); + if (it == _connectionIDs.end()) { + _connectionIDs[uuid] = INITIAL_CONNECTION_ID; + } + // we didn't have this node, so add them Node* newNode = new Node(uuid, nodeType, publicSocket, localSocket); newNode->setIsReplicated(isReplicated); @@ -712,13 +727,13 @@ SharedNodePointer LimitedNodeList::addOrUpdateNode(const QUuid& uuid, NodeType_t } } -std::unique_ptr LimitedNodeList::constructPingPacket(PingType_t pingType) { - int packetSize = sizeof(PingType_t) + sizeof(quint64); +std::unique_ptr LimitedNodeList::constructPingPacket(const QUuid& nodeId, PingType_t pingType) { + int packetSize = sizeof(PingType_t) + sizeof(quint64) + sizeof(int64_t); auto pingPacket = NLPacket::create(PacketType::Ping, packetSize); - pingPacket->writePrimitive(pingType); pingPacket->writePrimitive(usecTimestampNow()); + pingPacket->writePrimitive(_connectionIDs[nodeId]); return pingPacket; } diff --git a/libraries/networking/src/LimitedNodeList.h b/libraries/networking/src/LimitedNodeList.h index 7165b3dd63..7ec3a41450 100644 --- a/libraries/networking/src/LimitedNodeList.h +++ b/libraries/networking/src/LimitedNodeList.h @@ -66,6 +66,10 @@ const QHostAddress DEFAULT_ASSIGNMENT_CLIENT_MONITOR_HOSTNAME = QHostAddress::Lo const QString USERNAME_UUID_REPLACEMENT_STATS_KEY = "$username"; +using ConnectionID = int64_t; +const ConnectionID NULL_CONNECTION_ID { -1 }; +const ConnectionID INITIAL_CONNECTION_ID { 0 }; + typedef std::pair UUIDNodePair; typedef tbb::concurrent_unordered_map NodeHash; @@ -180,7 +184,7 @@ public: void getPacketStats(float& packetsInPerSecond, float& bytesInPerSecond, float& packetsOutPerSecond, float& bytesOutPerSecond); void resetPacketStats(); - std::unique_ptr constructPingPacket(PingType_t pingType = PingType::Agnostic); + std::unique_ptr constructPingPacket(const QUuid& nodeId, PingType_t pingType = PingType::Agnostic); std::unique_ptr constructPingReplyPacket(ReceivedMessage& message); static std::unique_ptr constructICEPingPacket(PingType_t pingType, const QUuid& iceID); @@ -319,7 +323,7 @@ public slots: void startSTUNPublicSocketUpdate(); virtual void sendSTUNRequest(); - bool killNodeWithUUID(const QUuid& nodeUUID); + bool killNodeWithUUID(const QUuid& nodeUUID, ConnectionID newConnectionID = NULL_CONNECTION_ID); signals: void dataSent(quint8 channelType, int bytes); @@ -371,7 +375,7 @@ protected: bool packetSourceAndHashMatchAndTrackBandwidth(const udt::Packet& packet, Node* sourceNode = nullptr); void processSTUNResponse(std::unique_ptr packet); - void handleNodeKill(const SharedNodePointer& node); + void handleNodeKill(const SharedNodePointer& node, ConnectionID newConnectionID = NULL_CONNECTION_ID); void stopInitialSTUNUpdate(bool success); @@ -418,6 +422,7 @@ protected: } } + std::unordered_map _connectionIDs; private slots: void flagTimeForConnectionStep(ConnectionStep connectionStep, quint64 timestamp); diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index 13931be2ac..21c77fe059 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/libraries/networking/src/NodeList.cpp @@ -214,6 +214,20 @@ void NodeList::processPingPacket(QSharedPointer message, Shared sendingNode->setSymmetricSocket(senderSockAddr); } } + + int64_t connectionId; + + message->readPrimitive(&connectionId); + + auto it = _connectionIDs.find(sendingNode->getUUID()); + if (it != _connectionIDs.end()) { + if (connectionId > it->second) { + qDebug() << "Received a ping packet with a larger connection id (" << connectionId << ">" << it->second << ") from " + << sendingNode->getUUID(); + killNodeWithUUID(sendingNode->getUUID(), connectionId); + } + } + } void NodeList::processPingReplyPacket(QSharedPointer message, SharedNodePointer sendingNode) { @@ -705,16 +719,18 @@ void NodeList::pingPunchForInactiveNode(const SharedNodePointer& node) { if (node->getConnectionAttempts() > 0 && node->getConnectionAttempts() % NUM_DEBUG_CONNECTION_ATTEMPTS == 0) { qCDebug(networking) << "No response to UDP hole punch pings for node" << node->getUUID() << "in last second."; } + + auto nodeId = node->getUUID(); // send the ping packet to the local and public sockets for this node - auto localPingPacket = constructPingPacket(PingType::Local); + auto localPingPacket = constructPingPacket(nodeId, PingType::Local); sendPacket(std::move(localPingPacket), *node, node->getLocalSocket()); - auto publicPingPacket = constructPingPacket(PingType::Public); + auto publicPingPacket = constructPingPacket(nodeId, PingType::Public); sendPacket(std::move(publicPingPacket), *node, node->getPublicSocket()); if (!node->getSymmetricSocket().isNull()) { - auto symmetricPingPacket = constructPingPacket(PingType::Symmetric); + auto symmetricPingPacket = constructPingPacket(nodeId, PingType::Symmetric); sendPacket(std::move(symmetricPingPacket), *node, node->getSymmetricSocket()); } @@ -784,7 +800,7 @@ void NodeList::sendKeepAlivePings() { auto type = node->getType(); return !node->isUpstream() && _nodeTypesOfInterest.contains(type) && !NodeType::isDownstream(type); }, [&](const SharedNodePointer& node) { - sendPacket(constructPingPacket(), *node); + sendPacket(constructPingPacket(node->getUUID()), *node); }); } From 5bd89159dd263abc338c1a680f6fbc9c3f9c2e6d Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Mon, 16 Apr 2018 12:03:58 -0700 Subject: [PATCH 03/14] Update variable naming in NodeList from Id to ID --- libraries/networking/src/NodeList.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index 21c77fe059..172b016e13 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/libraries/networking/src/NodeList.cpp @@ -215,16 +215,16 @@ void NodeList::processPingPacket(QSharedPointer message, Shared } } - int64_t connectionId; + int64_t connectionID; - message->readPrimitive(&connectionId); + message->readPrimitive(&connectionID); auto it = _connectionIDs.find(sendingNode->getUUID()); if (it != _connectionIDs.end()) { - if (connectionId > it->second) { - qDebug() << "Received a ping packet with a larger connection id (" << connectionId << ">" << it->second << ") from " + if (connectionID > it->second) { + qDebug() << "Received a ping packet with a larger connection id (" << connectionID << ">" << it->second << ") from " << sendingNode->getUUID(); - killNodeWithUUID(sendingNode->getUUID(), connectionId); + killNodeWithUUID(sendingNode->getUUID(), connectionID); } } @@ -720,17 +720,17 @@ void NodeList::pingPunchForInactiveNode(const SharedNodePointer& node) { qCDebug(networking) << "No response to UDP hole punch pings for node" << node->getUUID() << "in last second."; } - auto nodeId = node->getUUID(); + auto nodeID = node->getUUID(); // send the ping packet to the local and public sockets for this node - auto localPingPacket = constructPingPacket(nodeId, PingType::Local); + auto localPingPacket = constructPingPacket(nodeID, PingType::Local); sendPacket(std::move(localPingPacket), *node, node->getLocalSocket()); - auto publicPingPacket = constructPingPacket(nodeId, PingType::Public); + auto publicPingPacket = constructPingPacket(nodeID, PingType::Public); sendPacket(std::move(publicPingPacket), *node, node->getPublicSocket()); if (!node->getSymmetricSocket().isNull()) { - auto symmetricPingPacket = constructPingPacket(nodeId, PingType::Symmetric); + auto symmetricPingPacket = constructPingPacket(nodeID, PingType::Symmetric); sendPacket(std::move(symmetricPingPacket), *node, node->getSymmetricSocket()); } From 9cf133cff216c710cb71c9a0411ce098c199766c Mon Sep 17 00:00:00 2001 From: Ryan Huffman Date: Thu, 19 Apr 2018 14:03:51 -0700 Subject: [PATCH 04/14] Increase packet version for Ping --- libraries/networking/src/udt/PacketHeaders.cpp | 2 ++ libraries/networking/src/udt/PacketHeaders.h | 4 ++++ 2 files changed, 6 insertions(+) diff --git a/libraries/networking/src/udt/PacketHeaders.cpp b/libraries/networking/src/udt/PacketHeaders.cpp index a83924ee58..11b2c516f8 100644 --- a/libraries/networking/src/udt/PacketHeaders.cpp +++ b/libraries/networking/src/udt/PacketHeaders.cpp @@ -75,6 +75,8 @@ PacketVersion versionForPacketType(PacketType packetType) { return static_cast(IcePingVersion::SendICEPeerID); case PacketType::DomainSettings: return 18; // replace min_avatar_scale and max_avatar_scale with min_avatar_height and max_avatar_height + case PacketType::Ping: + return static_cast(PingVersion::IncludeConnectionID); default: return 17; } diff --git a/libraries/networking/src/udt/PacketHeaders.h b/libraries/networking/src/udt/PacketHeaders.h index 09fd31a41e..091fcb1091 100644 --- a/libraries/networking/src/udt/PacketHeaders.h +++ b/libraries/networking/src/udt/PacketHeaders.h @@ -322,4 +322,8 @@ enum class IcePingVersion : PacketVersion { SendICEPeerID = 18 }; +enum class PingVersion : PacketVersion { + IncludeConnectionID = 18 +}; + #endif // hifi_PacketHeaders_h From fb496156cec7ce83c32f6a0aba347774ac3dd4ea Mon Sep 17 00:00:00 2001 From: Clement Date: Fri, 13 Apr 2018 15:04:25 -0700 Subject: [PATCH 05/14] Fix threaded force crashes --- interface/src/Menu.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/interface/src/Menu.cpp b/interface/src/Menu.cpp index e74339a019..389913f7e1 100644 --- a/interface/src/Menu.cpp +++ b/interface/src/Menu.cpp @@ -749,32 +749,32 @@ Menu::Menu() { action = addActionToQMenuAndActionHash(crashMenu, MenuOption::CrashPureVirtualFunction); connect(action, &QAction::triggered, qApp, []() { crash::pureVirtualCall(); }); action = addActionToQMenuAndActionHash(crashMenu, MenuOption::CrashPureVirtualFunctionThreaded); - connect(action, &QAction::triggered, qApp, []() { std::thread([]() { crash::pureVirtualCall(); }); }); + connect(action, &QAction::triggered, qApp, []() { std::thread(crash::pureVirtualCall).join(); }); action = addActionToQMenuAndActionHash(crashMenu, MenuOption::CrashDoubleFree); connect(action, &QAction::triggered, qApp, []() { crash::doubleFree(); }); action = addActionToQMenuAndActionHash(crashMenu, MenuOption::CrashDoubleFreeThreaded); - connect(action, &QAction::triggered, qApp, []() { std::thread([]() { crash::doubleFree(); }); }); + connect(action, &QAction::triggered, qApp, []() { std::thread(crash::doubleFree).join(); }); action = addActionToQMenuAndActionHash(crashMenu, MenuOption::CrashAbort); connect(action, &QAction::triggered, qApp, []() { crash::doAbort(); }); action = addActionToQMenuAndActionHash(crashMenu, MenuOption::CrashAbortThreaded); - connect(action, &QAction::triggered, qApp, []() { std::thread([]() { crash::doAbort(); }); }); + connect(action, &QAction::triggered, qApp, []() { std::thread(crash::doAbort).join(); }); action = addActionToQMenuAndActionHash(crashMenu, MenuOption::CrashNullDereference); connect(action, &QAction::triggered, qApp, []() { crash::nullDeref(); }); action = addActionToQMenuAndActionHash(crashMenu, MenuOption::CrashNullDereferenceThreaded); - connect(action, &QAction::triggered, qApp, []() { std::thread([]() { crash::nullDeref(); }); }); + connect(action, &QAction::triggered, qApp, []() { std::thread(crash::nullDeref).join(); }); action = addActionToQMenuAndActionHash(crashMenu, MenuOption::CrashOutOfBoundsVectorAccess); connect(action, &QAction::triggered, qApp, []() { crash::outOfBoundsVectorCrash(); }); action = addActionToQMenuAndActionHash(crashMenu, MenuOption::CrashOutOfBoundsVectorAccessThreaded); - connect(action, &QAction::triggered, qApp, []() { std::thread([]() { crash::outOfBoundsVectorCrash(); }); }); + connect(action, &QAction::triggered, qApp, []() { std::thread(crash::outOfBoundsVectorCrash).join(); }); action = addActionToQMenuAndActionHash(crashMenu, MenuOption::CrashNewFault); connect(action, &QAction::triggered, qApp, []() { crash::newFault(); }); action = addActionToQMenuAndActionHash(crashMenu, MenuOption::CrashNewFaultThreaded); - connect(action, &QAction::triggered, qApp, []() { std::thread([]() { crash::newFault(); }); }); + connect(action, &QAction::triggered, qApp, []() { std::thread(crash::newFault).join(); }); // Developer > Log... addActionToQMenuAndActionHash(developerMenu, MenuOption::Log, Qt::CTRL | Qt::SHIFT | Qt::Key_L, From bc251115edc891519a4b4d47470134be09398915 Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Tue, 17 Apr 2018 15:09:10 -0700 Subject: [PATCH 06/14] Add special handler for heap corruption --- interface/src/Crashpad.cpp | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/interface/src/Crashpad.cpp b/interface/src/Crashpad.cpp index 8ed3fc23bd..c824aaa9e4 100644 --- a/interface/src/Crashpad.cpp +++ b/interface/src/Crashpad.cpp @@ -31,10 +31,40 @@ using namespace crashpad; static const std::string BACKTRACE_URL { CMAKE_BACKTRACE_URL }; static const std::string BACKTRACE_TOKEN { CMAKE_BACKTRACE_TOKEN }; +static std::wstring gIPCPipe; + extern QString qAppFileName(); // crashpad::AnnotationList* crashpadAnnotations { nullptr }; +#include + +LONG WINAPI vectoredExceptionHandler(PEXCEPTION_POINTERS pExceptionInfo) { + if (pExceptionInfo->ExceptionRecord->ExceptionCode == 0xe06d7363) { // external exception + return EXCEPTION_CONTINUE_SEARCH; + } + + if (pExceptionInfo->ExceptionRecord->ExceptionCode == 0xc0000374) { // heap corruption + qCritical() << "VectoredExceptionHandler"; + qCritical() << QString::number(pExceptionInfo->ExceptionRecord->ExceptionCode, 16); + qCritical() << "Heap corruption!"; + + CrashpadClient client; + if (gIPCPipe.length()) { + + bool rc = client.SetHandlerIPCPipe(gIPCPipe); + qCritical() << "SetHandlerIPCPipe = " << rc; + } else { + qCritical() << "No IPC Pipe was previously defined for crash handler."; + } + qCritical() << "Calling DumpAndCrash()"; + client.DumpAndCrash(pExceptionInfo); + return EXCEPTION_CONTINUE_SEARCH; + } + + return EXCEPTION_CONTINUE_SEARCH; +} + bool startCrashHandler() { if (BACKTRACE_URL.empty() || BACKTRACE_TOKEN.empty()) { return false; @@ -76,7 +106,12 @@ bool startCrashHandler() { // Enable automated uploads. database->GetSettings()->SetUploadsEnabled(true); - return client.StartHandler(handler, db, db, BACKTRACE_URL, annotations, arguments, true, true); + bool result = client.StartHandler(handler, db, db, BACKTRACE_URL, annotations, arguments, true, true); + gIPCPipe = client.GetHandlerIPCPipe(); + + AddVectoredExceptionHandler(0, vectoredExceptionHandler); + + return result; } void setCrashAnnotation(std::string name, std::string value) { From f2c471558c535a46ddeb5e8add3b75fa2e5c2317 Mon Sep 17 00:00:00 2001 From: Atlante45 Date: Tue, 17 Apr 2018 16:22:42 -0700 Subject: [PATCH 07/14] CR --- interface/src/Crashpad.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/interface/src/Crashpad.cpp b/interface/src/Crashpad.cpp index c824aaa9e4..27da619af1 100644 --- a/interface/src/Crashpad.cpp +++ b/interface/src/Crashpad.cpp @@ -40,18 +40,19 @@ extern QString qAppFileName(); #include LONG WINAPI vectoredExceptionHandler(PEXCEPTION_POINTERS pExceptionInfo) { - if (pExceptionInfo->ExceptionRecord->ExceptionCode == 0xe06d7363) { // external exception + static const DWORD EXTERNAL_EXCEPTION_CODE{ 0xe06d7363 }; + static const DWORD HEAP_CORRUPTION_CODE{ 0xc0000374 }; + + auto exceptionCode = pExceptionInfo->ExceptionRecord->ExceptionCode; + if (exceptionCode == EXTERNAL_EXCEPTION_CODE) { return EXCEPTION_CONTINUE_SEARCH; } - if (pExceptionInfo->ExceptionRecord->ExceptionCode == 0xc0000374) { // heap corruption - qCritical() << "VectoredExceptionHandler"; - qCritical() << QString::number(pExceptionInfo->ExceptionRecord->ExceptionCode, 16); - qCritical() << "Heap corruption!"; + if (exceptionCode == HEAP_CORRUPTION_CODE) { + qCritical() << "VectoredExceptionHandler: Heap corruption:" << QString::number(exceptionCode, 16); CrashpadClient client; if (gIPCPipe.length()) { - bool rc = client.SetHandlerIPCPipe(gIPCPipe); qCritical() << "SetHandlerIPCPipe = " << rc; } else { From a6509f5ea7dcac0258c115256383a0bc8f936dad Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Mon, 23 Apr 2018 13:25:18 -0700 Subject: [PATCH 08/14] fix connection to start stats sending for assignments --- libraries/networking/src/ThreadedAssignment.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/libraries/networking/src/ThreadedAssignment.cpp b/libraries/networking/src/ThreadedAssignment.cpp index 18e4593c91..8b6de7da11 100644 --- a/libraries/networking/src/ThreadedAssignment.cpp +++ b/libraries/networking/src/ThreadedAssignment.cpp @@ -84,7 +84,8 @@ void ThreadedAssignment::commonInit(const QString& targetName, NodeType_t nodeTy _domainServerTimer.start(); // start sending stats packet once we connect to the domain - connect(&nodeList->getDomainHandler(), SIGNAL(connectedToDomain(const QString&)), &_statsTimer, SLOT(start())); + connect(&nodeList->getDomainHandler(), &DomainHandler::connectedToDomain, + &_statsTimer, static_cast(&QTimer::start)); // stop sending stats if we disconnect connect(&nodeList->getDomainHandler(), &DomainHandler::disconnectedFromDomain, &_statsTimer, &QTimer::stop); From 95d07345d46587a297d1c2cc5e52226b7db3f449 Mon Sep 17 00:00:00 2001 From: Andrew Meadows Date: Mon, 23 Apr 2018 17:00:38 -0700 Subject: [PATCH 09/14] don't try to build convex hulls with nan data --- libraries/physics/src/ShapeFactory.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/libraries/physics/src/ShapeFactory.cpp b/libraries/physics/src/ShapeFactory.cpp index f484f32fdf..5abeb022aa 100644 --- a/libraries/physics/src/ShapeFactory.cpp +++ b/libraries/physics/src/ShapeFactory.cpp @@ -114,6 +114,11 @@ btConvexHullShape* createConvexHull(const ShapeInfo::PointList& points) { minCorner = glm::min(minCorner, points[i]); } center /= (float)(points.size()); + if (glm::any(glm::isnan(center))) { + // don't feed garbage to Bullet + assert(false); // crash here in DEBUG so we can investigate source of bad input + return nullptr; + } float margin = hull->getMargin(); @@ -265,7 +270,7 @@ btTriangleIndexVertexArray* createStaticMeshArray(const ShapeInfo& info) { } const btCollisionShape* ShapeFactory::createShapeFromInfo(const ShapeInfo& info) { - btCollisionShape* shape = NULL; + btCollisionShape* shape = nullptr; int type = info.getType(); switch(type) { case SHAPE_TYPE_BOX: { From a144de54e753bf759906292aa220957a7635ca72 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Mon, 23 Apr 2018 12:29:41 -0700 Subject: [PATCH 10/14] Attempt to shutdown web surfaces more consistently --- interface/src/Application.cpp | 13 ++++- interface/src/Application.h | 7 ++- interface/src/ui/overlays/Web3DOverlay.cpp | 12 ++-- .../src/RenderableWebEntityItem.cpp | 39 +++++++------ libraries/qml/src/qml/OffscreenSurface.cpp | 7 +-- .../qml/src/qml/impl/RenderEventHandler.cpp | 11 ++-- libraries/qml/src/qml/impl/SharedObject.cpp | 56 +++++++++++++------ .../src/AbstractViewStateInterface.h | 19 +++++-- 8 files changed, 99 insertions(+), 65 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index f5aaa2fbe9..0160f465a9 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -4743,7 +4743,7 @@ void Application::updateLOD(float deltaTime) const { } } -void Application::pushPostUpdateLambda(void* key, std::function func) { +void Application::pushPostUpdateLambda(void* key, const std::function& func) { std::unique_lock guard(_postUpdateLambdasLock); _postUpdateLambdas[key] = func; } @@ -7346,7 +7346,7 @@ void Application::windowMinimizedChanged(bool minimized) { } } -void Application::postLambdaEvent(std::function f) { +void Application::postLambdaEvent(const std::function& f) { if (this->thread() == QThread::currentThread()) { f(); } else { @@ -7354,6 +7354,15 @@ void Application::postLambdaEvent(std::function f) { } } +void Application::sendLambdaEvent(const std::function& f) { + if (this->thread() == QThread::currentThread()) { + f(); + } else { + LambdaEvent event(f); + QCoreApplication::sendEvent(this, &event); + } +} + void Application::initPlugins(const QStringList& arguments) { QCommandLineOption display("display", "Preferred displays", "displays"); QCommandLineOption disableDisplays("disable-displays", "Displays to disable", "displays"); diff --git a/interface/src/Application.h b/interface/src/Application.h index 812e51e49c..d58bbad8af 100644 --- a/interface/src/Application.h +++ b/interface/src/Application.h @@ -136,7 +136,8 @@ public: Application(int& argc, char** argv, QElapsedTimer& startup_time, bool runningMarkerExisted); ~Application(); - void postLambdaEvent(std::function f) override; + void postLambdaEvent(const std::function& f) override; + void sendLambdaEvent(const std::function& f) override; QString getPreviousScriptLocation(); void setPreviousScriptLocation(const QString& previousScriptLocation); @@ -239,7 +240,7 @@ public: qint64 getCurrentSessionRuntime() const { return _sessionRunTimer.elapsed(); } - bool isAboutToQuit() const override { return _aboutToQuit; } + bool isAboutToQuit() const { return _aboutToQuit; } bool isPhysicsEnabled() const { return _physicsEnabled; } // the isHMDMode is true whenever we use the interface from an HMD and not a standard flat display @@ -263,7 +264,7 @@ public: render::EnginePointer getRenderEngine() override { return _renderEngine; } gpu::ContextPointer getGPUContext() const { return _gpuContext; } - virtual void pushPostUpdateLambda(void* key, std::function func) override; + virtual void pushPostUpdateLambda(void* key, const std::function& func) override; void updateMyAvatarLookAtPosition(); diff --git a/interface/src/ui/overlays/Web3DOverlay.cpp b/interface/src/ui/overlays/Web3DOverlay.cpp index 10050c94d0..d574e08a94 100644 --- a/interface/src/ui/overlays/Web3DOverlay.cpp +++ b/interface/src/ui/overlays/Web3DOverlay.cpp @@ -65,14 +65,10 @@ const QString Web3DOverlay::TYPE = "web3d"; const QString Web3DOverlay::QML = "Web3DOverlay.qml"; static auto qmlSurfaceDeleter = [](OffscreenQmlSurface* surface) { - AbstractViewStateInterface::instance()->postLambdaEvent([surface] { - if (AbstractViewStateInterface::instance()->isAboutToQuit()) { - // WebEngineView may run other threads (wasapi), so they must be deleted for a clean shutdown - // if the application has already stopped its event loop, delete must be explicit - delete surface; - } else { - surface->deleteLater(); - } + AbstractViewStateInterface::instance()->sendLambdaEvent([surface] { + // WebEngineView may run other threads (wasapi), so they must be deleted for a clean shutdown + // if the application has already stopped its event loop, delete must be explicit + delete surface; }); }; diff --git a/libraries/entities-renderer/src/RenderableWebEntityItem.cpp b/libraries/entities-renderer/src/RenderableWebEntityItem.cpp index f31ed4e238..33b02976c1 100644 --- a/libraries/entities-renderer/src/RenderableWebEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableWebEntityItem.cpp @@ -25,7 +25,7 @@ #include #include "EntitiesRendererLogging.h" - +#include using namespace render; using namespace render::entities; @@ -45,6 +45,7 @@ static int DEFAULT_MAX_FPS = 10; static int YOUTUBE_MAX_FPS = 30; static QTouchDevice _touchDevice; +static const char* URL_PROPERTY = "url"; WebEntityRenderer::ContentType WebEntityRenderer::getContentType(const QString& urlString) { if (urlString.isEmpty()) { @@ -52,7 +53,7 @@ WebEntityRenderer::ContentType WebEntityRenderer::getContentType(const QString& } const QUrl url(urlString); - if (url.scheme() == "http" || url.scheme() == "https" || + if (url.scheme() == URL_SCHEME_HTTP || url.scheme() == URL_SCHEME_HTTPS || urlString.toLower().endsWith(".htm") || urlString.toLower().endsWith(".html")) { return ContentType::HtmlContent; } @@ -164,6 +165,8 @@ void WebEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scene if (urlChanged) { if (newContentType != ContentType::HtmlContent || currentContentType != ContentType::HtmlContent) { destroyWebSurface(); + // If we destroyed the surface, the URL change will be implicitly handled by the re-creation + urlChanged = false; } withWriteLock([&] { @@ -185,8 +188,8 @@ void WebEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scene return; } - if (urlChanged) { - _webSurface->getRootItem()->setProperty("url", _lastSourceUrl); + if (urlChanged && _contentType == ContentType::HtmlContent) { + _webSurface->getRootItem()->setProperty(URL_PROPERTY, _lastSourceUrl); } if (_contextPosition != entity->getWorldPosition()) { @@ -254,6 +257,14 @@ bool WebEntityRenderer::hasWebSurface() { return (bool)_webSurface && _webSurface->getRootItem(); } +static const auto WebSurfaceDeleter = [](OffscreenQmlSurface* webSurface) { + AbstractViewStateInterface::instance()->sendLambdaEvent([webSurface] { + // WebEngineView may run other threads (wasapi), so they must be deleted for a clean shutdown + // if the application has already stopped its event loop, delete must be explicit + delete webSurface; + }); +}; + bool WebEntityRenderer::buildWebSurface(const TypedEntityPointer& entity) { if (_currentWebCount >= MAX_CONCURRENT_WEB_VIEWS) { qWarning() << "Too many concurrent web views to create new view"; @@ -261,20 +272,9 @@ bool WebEntityRenderer::buildWebSurface(const TypedEntityPointer& entity) { } ++_currentWebCount; - auto deleter = [](OffscreenQmlSurface* webSurface) { - AbstractViewStateInterface::instance()->postLambdaEvent([webSurface] { - if (AbstractViewStateInterface::instance()->isAboutToQuit()) { - // WebEngineView may run other threads (wasapi), so they must be deleted for a clean shutdown - // if the application has already stopped its event loop, delete must be explicit - delete webSurface; - } else { - webSurface->deleteLater(); - } - }); - }; // FIXME use the surface cache instead of explicit creation - _webSurface = QSharedPointer(new OffscreenQmlSurface(), deleter); + _webSurface = QSharedPointer(new OffscreenQmlSurface(), WebSurfaceDeleter); // FIXME, the max FPS could be better managed by being dynamic (based on the number of current surfaces // and the current rendering load) _webSurface->setMaxFps(DEFAULT_MAX_FPS); @@ -302,7 +302,7 @@ bool WebEntityRenderer::buildWebSurface(const TypedEntityPointer& entity) { _webSurface->setMaxFps(DEFAULT_MAX_FPS); } _webSurface->load("controls/WebEntityView.qml", [this](QQmlContext* context, QObject* item) { - item->setProperty("url", _lastSourceUrl); + item->setProperty(URL_PROPERTY, _lastSourceUrl); }); } else if (_contentType == ContentType::QmlContent) { _webSurface->load(_lastSourceUrl, [this](QQmlContext* context, QObject* item) { @@ -327,6 +327,11 @@ void WebEntityRenderer::destroyWebSurface() { if (webSurface) { --_currentWebCount; QQuickItem* rootItem = webSurface->getRootItem(); + // Explicitly set the web URL to an empty string, in an effort to get a + // faster shutdown of any chromium processes interacting with audio + if (rootItem && _contentType == ContentType::HtmlContent) { + rootItem->setProperty(URL_PROPERTY, ""); + } if (rootItem && rootItem->objectName() == "tabletRoot") { auto tabletScriptingInterface = DependencyManager::get(); diff --git a/libraries/qml/src/qml/OffscreenSurface.cpp b/libraries/qml/src/qml/OffscreenSurface.cpp index 6580c5b1e8..d259851e1c 100644 --- a/libraries/qml/src/qml/OffscreenSurface.cpp +++ b/libraries/qml/src/qml/OffscreenSurface.cpp @@ -64,14 +64,11 @@ OffscreenSurface::OffscreenSurface() } OffscreenSurface::~OffscreenSurface() { - disconnect(qApp); - _sharedObject->destroy(); + delete _sharedObject; + const_cast(_sharedObject) = nullptr; } bool OffscreenSurface::fetchTexture(TextureAndFence& textureAndFence) { - if (!_sharedObject) { - return false; - } hifi::qml::impl::TextureAndFence typedTextureAndFence; bool result = _sharedObject->fetchTexture(typedTextureAndFence); textureAndFence = typedTextureAndFence; diff --git a/libraries/qml/src/qml/impl/RenderEventHandler.cpp b/libraries/qml/src/qml/impl/RenderEventHandler.cpp index 6b66ce9314..945a469611 100644 --- a/libraries/qml/src/qml/impl/RenderEventHandler.cpp +++ b/libraries/qml/src/qml/impl/RenderEventHandler.cpp @@ -49,8 +49,8 @@ RenderEventHandler::RenderEventHandler(SharedObject* shared, QThread* targetThre qFatal("Unable to create new offscreen GL context"); } - moveToThread(targetThread); _canvas.moveToThreadWithContext(targetThread); + moveToThread(targetThread); } void RenderEventHandler::onInitalize() { @@ -160,11 +160,8 @@ void RenderEventHandler::onQuit() { } _shared->shutdownRendering(_canvas, _currentSize); - // Release the reference to the shared object. This will allow it to - // be destroyed (should happen on it's own thread). - _shared->deleteLater(); - - deleteLater(); - + _canvas.doneCurrent(); + _canvas.moveToThreadWithContext(qApp->thread()); + moveToThread(qApp->thread()); QThread::currentThread()->quit(); } diff --git a/libraries/qml/src/qml/impl/SharedObject.cpp b/libraries/qml/src/qml/impl/SharedObject.cpp index d593169d94..08b3b1991a 100644 --- a/libraries/qml/src/qml/impl/SharedObject.cpp +++ b/libraries/qml/src/qml/impl/SharedObject.cpp @@ -72,26 +72,35 @@ SharedObject::SharedObject() { QObject::connect(qApp, &QCoreApplication::aboutToQuit, this, &SharedObject::onAboutToQuit); } + SharedObject::~SharedObject() { - if (_quickWindow) { - _quickWindow->destroy(); - _quickWindow = nullptr; + // After destroy returns, the rendering thread should be gone + destroy(); + + // _renderTimer is created with `this` as the parent, so need no explicit destruction + + // Destroy the event hand + if (_renderObject) { + delete _renderObject; + _renderObject = nullptr; } if (_renderControl) { - _renderControl->deleteLater(); + delete _renderControl; _renderControl = nullptr; } - if (_renderThread) { - _renderThread->quit(); - _renderThread->deleteLater(); + if (_quickWindow) { + _quickWindow->destroy(); + delete _quickWindow; + _quickWindow = nullptr; } - if (_rootItem) { - _rootItem->deleteLater(); - _rootItem = nullptr; - } + // _rootItem is parented to the quickWindow, so needs no explicit destruction + //if (_rootItem) { + // delete _rootItem; + // _rootItem = nullptr; + //} releaseEngine(_qmlContext->engine()); } @@ -116,6 +125,10 @@ void SharedObject::create(OffscreenSurface* surface) { } void SharedObject::setRootItem(QQuickItem* rootItem) { + if (_quit) { + return; + } + _rootItem = rootItem; _rootItem->setSize(_quickWindow->size()); @@ -124,7 +137,6 @@ void SharedObject::setRootItem(QQuickItem* rootItem) { _renderThread->setObjectName(objectName()); _renderThread->start(); - // Create event handler for the render thread _renderObject = new RenderEventHandler(this, _renderThread); QCoreApplication::postEvent(this, new OffscreenEvent(OffscreenEvent::Initialize)); @@ -134,35 +146,43 @@ void SharedObject::setRootItem(QQuickItem* rootItem) { } void SharedObject::destroy() { + // `destroy` is idempotent, it can be called multiple times without issues if (_quit) { return; } if (!_rootItem) { - deleteLater(); return; } - _paused = true; if (_renderTimer) { + _renderTimer->stop(); QObject::disconnect(_renderTimer); - _renderTimer->deleteLater(); } - QObject::disconnect(_renderControl); + if (_renderControl) { + QObject::disconnect(_renderControl); + } + QObject::disconnect(qApp); { QMutexLocker lock(&_mutex); _quit = true; - QCoreApplication::postEvent(_renderObject, new OffscreenEvent(OffscreenEvent::Quit), Qt::HighEventPriority); + if (_renderObject) { + QCoreApplication::postEvent(_renderObject, new OffscreenEvent(OffscreenEvent::Quit), Qt::HighEventPriority); + } } // Block until the rendering thread has stopped // FIXME this is undesirable because this is blocking the main thread, // but I haven't found a reliable way to do this only at application // shutdown - _renderThread->wait(); + if (_renderThread) { + _renderThread->wait(); + delete _renderThread; + _renderThread = nullptr; + } } diff --git a/libraries/render-utils/src/AbstractViewStateInterface.h b/libraries/render-utils/src/AbstractViewStateInterface.h index 96e9f4d222..54fdc903ca 100644 --- a/libraries/render-utils/src/AbstractViewStateInterface.h +++ b/libraries/render-utils/src/AbstractViewStateInterface.h @@ -37,15 +37,25 @@ public: virtual glm::vec3 getAvatarPosition() const = 0; - virtual bool isAboutToQuit() const = 0; - virtual void postLambdaEvent(std::function f) = 0; + // Unfortunately, having this here is a bad idea. Lots of objects connect to + // the aboutToQuit signal, and it's impossible to know the order in which + // the receivers will be called, so this might return false negatives + //virtual bool isAboutToQuit() const = 0; + + // Queue code to execute on the main thread. + // If called from the main thread, the lambda will execute synchronously + virtual void postLambdaEvent(const std::function& f) = 0; + // Synchronously execute code on the main thread. This function will + // not return until the code is executed, regardles of which thread it + // is called from + virtual void sendLambdaEvent(const std::function& f) = 0; virtual qreal getDevicePixelRatio() = 0; virtual render::ScenePointer getMain3DScene() = 0; virtual render::EnginePointer getRenderEngine() = 0; - virtual void pushPostUpdateLambda(void* key, std::function func) = 0; + virtual void pushPostUpdateLambda(void* key, const std::function& func) = 0; virtual bool isHMDMode() const = 0; @@ -54,5 +64,4 @@ public: static void setInstance(AbstractViewStateInterface* instance); }; - -#endif // hifi_AbstractViewStateInterface_h +#endif // hifi_AbstractViewStateInterface_h From 4228b4269f37027e6eaada1f170e4a37dd0fcf49 Mon Sep 17 00:00:00 2001 From: Brad Davis Date: Mon, 23 Apr 2018 20:41:36 -0700 Subject: [PATCH 11/14] Fixing PR build --- libraries/qml/src/qml/OffscreenSurface.cpp | 1 - tests/render-perf/src/main.cpp | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/libraries/qml/src/qml/OffscreenSurface.cpp b/libraries/qml/src/qml/OffscreenSurface.cpp index d259851e1c..03453a99f7 100644 --- a/libraries/qml/src/qml/OffscreenSurface.cpp +++ b/libraries/qml/src/qml/OffscreenSurface.cpp @@ -65,7 +65,6 @@ OffscreenSurface::OffscreenSurface() OffscreenSurface::~OffscreenSurface() { delete _sharedObject; - const_cast(_sharedObject) = nullptr; } bool OffscreenSurface::fetchTexture(TextureAndFence& textureAndFence) { diff --git a/tests/render-perf/src/main.cpp b/tests/render-perf/src/main.cpp index 93672cc5a2..9249b3d957 100644 --- a/tests/render-perf/src/main.cpp +++ b/tests/render-perf/src/main.cpp @@ -453,8 +453,8 @@ protected: return vec3(); } - bool isAboutToQuit() const override { return false; } - void postLambdaEvent(std::function f) override {} + void postLambdaEvent(const std::function& f) override {} + void sendLambdaEvent(const std::function& f) override {} qreal getDevicePixelRatio() override { return 1.0f; @@ -469,7 +469,7 @@ protected: } std::map> _postUpdateLambdas; - void pushPostUpdateLambda(void* key, std::function func) override { + void pushPostUpdateLambda(void* key, const std::function& func) override { _postUpdateLambdas[key] = func; } From 0804abcca6021bc600294737ee2a8a55b3f1a313 Mon Sep 17 00:00:00 2001 From: Clement Date: Tue, 24 Apr 2018 10:57:31 -0700 Subject: [PATCH 12/14] Heap corruption crashes CR --- interface/src/Crashpad.cpp | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/interface/src/Crashpad.cpp b/interface/src/Crashpad.cpp index 27da619af1..ae2f341337 100644 --- a/interface/src/Crashpad.cpp +++ b/interface/src/Crashpad.cpp @@ -40,27 +40,13 @@ extern QString qAppFileName(); #include LONG WINAPI vectoredExceptionHandler(PEXCEPTION_POINTERS pExceptionInfo) { - static const DWORD EXTERNAL_EXCEPTION_CODE{ 0xe06d7363 }; - static const DWORD HEAP_CORRUPTION_CODE{ 0xc0000374 }; - - auto exceptionCode = pExceptionInfo->ExceptionRecord->ExceptionCode; - if (exceptionCode == EXTERNAL_EXCEPTION_CODE) { - return EXCEPTION_CONTINUE_SEARCH; - } - - if (exceptionCode == HEAP_CORRUPTION_CODE) { - qCritical() << "VectoredExceptionHandler: Heap corruption:" << QString::number(exceptionCode, 16); - + if (pExceptionInfo->ExceptionRecord->ExceptionCode == STATUS_HEAP_CORRUPTION || + pExceptionInfo->ExceptionRecord->ExceptionCode == STATUS_STACK_BUFFER_OVERRUN) { CrashpadClient client; if (gIPCPipe.length()) { - bool rc = client.SetHandlerIPCPipe(gIPCPipe); - qCritical() << "SetHandlerIPCPipe = " << rc; - } else { - qCritical() << "No IPC Pipe was previously defined for crash handler."; + client.SetHandlerIPCPipe(gIPCPipe); } - qCritical() << "Calling DumpAndCrash()"; client.DumpAndCrash(pExceptionInfo); - return EXCEPTION_CONTINUE_SEARCH; } return EXCEPTION_CONTINUE_SEARCH; From ea0ef20416d44932a21421bd7be8d50a0e0b5424 Mon Sep 17 00:00:00 2001 From: Bradley Austin Davis Date: Wed, 25 Apr 2018 09:43:20 -0700 Subject: [PATCH 13/14] Revert "HOTFIX version of PR 12969: Attempt to shutdown web surfaces more consistently" --- interface/src/Application.cpp | 13 +--- interface/src/Application.h | 7 +-- interface/src/ui/overlays/Web3DOverlay.cpp | 12 ++-- .../src/RenderableWebEntityItem.cpp | 39 +++++------- libraries/qml/src/qml/OffscreenSurface.cpp | 6 +- .../qml/src/qml/impl/RenderEventHandler.cpp | 11 ++-- libraries/qml/src/qml/impl/SharedObject.cpp | 62 +++++++------------ .../src/AbstractViewStateInterface.h | 19 ++---- tests/render-perf/src/main.cpp | 6 +- 9 files changed, 71 insertions(+), 104 deletions(-) diff --git a/interface/src/Application.cpp b/interface/src/Application.cpp index 0160f465a9..f5aaa2fbe9 100644 --- a/interface/src/Application.cpp +++ b/interface/src/Application.cpp @@ -4743,7 +4743,7 @@ void Application::updateLOD(float deltaTime) const { } } -void Application::pushPostUpdateLambda(void* key, const std::function& func) { +void Application::pushPostUpdateLambda(void* key, std::function func) { std::unique_lock guard(_postUpdateLambdasLock); _postUpdateLambdas[key] = func; } @@ -7346,7 +7346,7 @@ void Application::windowMinimizedChanged(bool minimized) { } } -void Application::postLambdaEvent(const std::function& f) { +void Application::postLambdaEvent(std::function f) { if (this->thread() == QThread::currentThread()) { f(); } else { @@ -7354,15 +7354,6 @@ void Application::postLambdaEvent(const std::function& f) { } } -void Application::sendLambdaEvent(const std::function& f) { - if (this->thread() == QThread::currentThread()) { - f(); - } else { - LambdaEvent event(f); - QCoreApplication::sendEvent(this, &event); - } -} - void Application::initPlugins(const QStringList& arguments) { QCommandLineOption display("display", "Preferred displays", "displays"); QCommandLineOption disableDisplays("disable-displays", "Displays to disable", "displays"); diff --git a/interface/src/Application.h b/interface/src/Application.h index d58bbad8af..812e51e49c 100644 --- a/interface/src/Application.h +++ b/interface/src/Application.h @@ -136,8 +136,7 @@ public: Application(int& argc, char** argv, QElapsedTimer& startup_time, bool runningMarkerExisted); ~Application(); - void postLambdaEvent(const std::function& f) override; - void sendLambdaEvent(const std::function& f) override; + void postLambdaEvent(std::function f) override; QString getPreviousScriptLocation(); void setPreviousScriptLocation(const QString& previousScriptLocation); @@ -240,7 +239,7 @@ public: qint64 getCurrentSessionRuntime() const { return _sessionRunTimer.elapsed(); } - bool isAboutToQuit() const { return _aboutToQuit; } + bool isAboutToQuit() const override { return _aboutToQuit; } bool isPhysicsEnabled() const { return _physicsEnabled; } // the isHMDMode is true whenever we use the interface from an HMD and not a standard flat display @@ -264,7 +263,7 @@ public: render::EnginePointer getRenderEngine() override { return _renderEngine; } gpu::ContextPointer getGPUContext() const { return _gpuContext; } - virtual void pushPostUpdateLambda(void* key, const std::function& func) override; + virtual void pushPostUpdateLambda(void* key, std::function func) override; void updateMyAvatarLookAtPosition(); diff --git a/interface/src/ui/overlays/Web3DOverlay.cpp b/interface/src/ui/overlays/Web3DOverlay.cpp index d574e08a94..10050c94d0 100644 --- a/interface/src/ui/overlays/Web3DOverlay.cpp +++ b/interface/src/ui/overlays/Web3DOverlay.cpp @@ -65,10 +65,14 @@ const QString Web3DOverlay::TYPE = "web3d"; const QString Web3DOverlay::QML = "Web3DOverlay.qml"; static auto qmlSurfaceDeleter = [](OffscreenQmlSurface* surface) { - AbstractViewStateInterface::instance()->sendLambdaEvent([surface] { - // WebEngineView may run other threads (wasapi), so they must be deleted for a clean shutdown - // if the application has already stopped its event loop, delete must be explicit - delete surface; + AbstractViewStateInterface::instance()->postLambdaEvent([surface] { + if (AbstractViewStateInterface::instance()->isAboutToQuit()) { + // WebEngineView may run other threads (wasapi), so they must be deleted for a clean shutdown + // if the application has already stopped its event loop, delete must be explicit + delete surface; + } else { + surface->deleteLater(); + } }); }; diff --git a/libraries/entities-renderer/src/RenderableWebEntityItem.cpp b/libraries/entities-renderer/src/RenderableWebEntityItem.cpp index 33b02976c1..f31ed4e238 100644 --- a/libraries/entities-renderer/src/RenderableWebEntityItem.cpp +++ b/libraries/entities-renderer/src/RenderableWebEntityItem.cpp @@ -25,7 +25,7 @@ #include #include "EntitiesRendererLogging.h" -#include + using namespace render; using namespace render::entities; @@ -45,7 +45,6 @@ static int DEFAULT_MAX_FPS = 10; static int YOUTUBE_MAX_FPS = 30; static QTouchDevice _touchDevice; -static const char* URL_PROPERTY = "url"; WebEntityRenderer::ContentType WebEntityRenderer::getContentType(const QString& urlString) { if (urlString.isEmpty()) { @@ -53,7 +52,7 @@ WebEntityRenderer::ContentType WebEntityRenderer::getContentType(const QString& } const QUrl url(urlString); - if (url.scheme() == URL_SCHEME_HTTP || url.scheme() == URL_SCHEME_HTTPS || + if (url.scheme() == "http" || url.scheme() == "https" || urlString.toLower().endsWith(".htm") || urlString.toLower().endsWith(".html")) { return ContentType::HtmlContent; } @@ -165,8 +164,6 @@ void WebEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scene if (urlChanged) { if (newContentType != ContentType::HtmlContent || currentContentType != ContentType::HtmlContent) { destroyWebSurface(); - // If we destroyed the surface, the URL change will be implicitly handled by the re-creation - urlChanged = false; } withWriteLock([&] { @@ -188,8 +185,8 @@ void WebEntityRenderer::doRenderUpdateSynchronousTyped(const ScenePointer& scene return; } - if (urlChanged && _contentType == ContentType::HtmlContent) { - _webSurface->getRootItem()->setProperty(URL_PROPERTY, _lastSourceUrl); + if (urlChanged) { + _webSurface->getRootItem()->setProperty("url", _lastSourceUrl); } if (_contextPosition != entity->getWorldPosition()) { @@ -257,14 +254,6 @@ bool WebEntityRenderer::hasWebSurface() { return (bool)_webSurface && _webSurface->getRootItem(); } -static const auto WebSurfaceDeleter = [](OffscreenQmlSurface* webSurface) { - AbstractViewStateInterface::instance()->sendLambdaEvent([webSurface] { - // WebEngineView may run other threads (wasapi), so they must be deleted for a clean shutdown - // if the application has already stopped its event loop, delete must be explicit - delete webSurface; - }); -}; - bool WebEntityRenderer::buildWebSurface(const TypedEntityPointer& entity) { if (_currentWebCount >= MAX_CONCURRENT_WEB_VIEWS) { qWarning() << "Too many concurrent web views to create new view"; @@ -272,9 +261,20 @@ bool WebEntityRenderer::buildWebSurface(const TypedEntityPointer& entity) { } ++_currentWebCount; + auto deleter = [](OffscreenQmlSurface* webSurface) { + AbstractViewStateInterface::instance()->postLambdaEvent([webSurface] { + if (AbstractViewStateInterface::instance()->isAboutToQuit()) { + // WebEngineView may run other threads (wasapi), so they must be deleted for a clean shutdown + // if the application has already stopped its event loop, delete must be explicit + delete webSurface; + } else { + webSurface->deleteLater(); + } + }); + }; // FIXME use the surface cache instead of explicit creation - _webSurface = QSharedPointer(new OffscreenQmlSurface(), WebSurfaceDeleter); + _webSurface = QSharedPointer(new OffscreenQmlSurface(), deleter); // FIXME, the max FPS could be better managed by being dynamic (based on the number of current surfaces // and the current rendering load) _webSurface->setMaxFps(DEFAULT_MAX_FPS); @@ -302,7 +302,7 @@ bool WebEntityRenderer::buildWebSurface(const TypedEntityPointer& entity) { _webSurface->setMaxFps(DEFAULT_MAX_FPS); } _webSurface->load("controls/WebEntityView.qml", [this](QQmlContext* context, QObject* item) { - item->setProperty(URL_PROPERTY, _lastSourceUrl); + item->setProperty("url", _lastSourceUrl); }); } else if (_contentType == ContentType::QmlContent) { _webSurface->load(_lastSourceUrl, [this](QQmlContext* context, QObject* item) { @@ -327,11 +327,6 @@ void WebEntityRenderer::destroyWebSurface() { if (webSurface) { --_currentWebCount; QQuickItem* rootItem = webSurface->getRootItem(); - // Explicitly set the web URL to an empty string, in an effort to get a - // faster shutdown of any chromium processes interacting with audio - if (rootItem && _contentType == ContentType::HtmlContent) { - rootItem->setProperty(URL_PROPERTY, ""); - } if (rootItem && rootItem->objectName() == "tabletRoot") { auto tabletScriptingInterface = DependencyManager::get(); diff --git a/libraries/qml/src/qml/OffscreenSurface.cpp b/libraries/qml/src/qml/OffscreenSurface.cpp index 03453a99f7..6580c5b1e8 100644 --- a/libraries/qml/src/qml/OffscreenSurface.cpp +++ b/libraries/qml/src/qml/OffscreenSurface.cpp @@ -64,10 +64,14 @@ OffscreenSurface::OffscreenSurface() } OffscreenSurface::~OffscreenSurface() { - delete _sharedObject; + disconnect(qApp); + _sharedObject->destroy(); } bool OffscreenSurface::fetchTexture(TextureAndFence& textureAndFence) { + if (!_sharedObject) { + return false; + } hifi::qml::impl::TextureAndFence typedTextureAndFence; bool result = _sharedObject->fetchTexture(typedTextureAndFence); textureAndFence = typedTextureAndFence; diff --git a/libraries/qml/src/qml/impl/RenderEventHandler.cpp b/libraries/qml/src/qml/impl/RenderEventHandler.cpp index 945a469611..6b66ce9314 100644 --- a/libraries/qml/src/qml/impl/RenderEventHandler.cpp +++ b/libraries/qml/src/qml/impl/RenderEventHandler.cpp @@ -49,8 +49,8 @@ RenderEventHandler::RenderEventHandler(SharedObject* shared, QThread* targetThre qFatal("Unable to create new offscreen GL context"); } - _canvas.moveToThreadWithContext(targetThread); moveToThread(targetThread); + _canvas.moveToThreadWithContext(targetThread); } void RenderEventHandler::onInitalize() { @@ -160,8 +160,11 @@ void RenderEventHandler::onQuit() { } _shared->shutdownRendering(_canvas, _currentSize); - _canvas.doneCurrent(); - _canvas.moveToThreadWithContext(qApp->thread()); - moveToThread(qApp->thread()); + // Release the reference to the shared object. This will allow it to + // be destroyed (should happen on it's own thread). + _shared->deleteLater(); + + deleteLater(); + QThread::currentThread()->quit(); } diff --git a/libraries/qml/src/qml/impl/SharedObject.cpp b/libraries/qml/src/qml/impl/SharedObject.cpp index 08b3b1991a..d593169d94 100644 --- a/libraries/qml/src/qml/impl/SharedObject.cpp +++ b/libraries/qml/src/qml/impl/SharedObject.cpp @@ -72,35 +72,26 @@ SharedObject::SharedObject() { QObject::connect(qApp, &QCoreApplication::aboutToQuit, this, &SharedObject::onAboutToQuit); } - SharedObject::~SharedObject() { - // After destroy returns, the rendering thread should be gone - destroy(); - - // _renderTimer is created with `this` as the parent, so need no explicit destruction - - // Destroy the event hand - if (_renderObject) { - delete _renderObject; - _renderObject = nullptr; - } - - if (_renderControl) { - delete _renderControl; - _renderControl = nullptr; - } - if (_quickWindow) { _quickWindow->destroy(); - delete _quickWindow; _quickWindow = nullptr; } - // _rootItem is parented to the quickWindow, so needs no explicit destruction - //if (_rootItem) { - // delete _rootItem; - // _rootItem = nullptr; - //} + if (_renderControl) { + _renderControl->deleteLater(); + _renderControl = nullptr; + } + + if (_renderThread) { + _renderThread->quit(); + _renderThread->deleteLater(); + } + + if (_rootItem) { + _rootItem->deleteLater(); + _rootItem = nullptr; + } releaseEngine(_qmlContext->engine()); } @@ -125,10 +116,6 @@ void SharedObject::create(OffscreenSurface* surface) { } void SharedObject::setRootItem(QQuickItem* rootItem) { - if (_quit) { - return; - } - _rootItem = rootItem; _rootItem->setSize(_quickWindow->size()); @@ -137,6 +124,7 @@ void SharedObject::setRootItem(QQuickItem* rootItem) { _renderThread->setObjectName(objectName()); _renderThread->start(); + // Create event handler for the render thread _renderObject = new RenderEventHandler(this, _renderThread); QCoreApplication::postEvent(this, new OffscreenEvent(OffscreenEvent::Initialize)); @@ -146,43 +134,35 @@ void SharedObject::setRootItem(QQuickItem* rootItem) { } void SharedObject::destroy() { - // `destroy` is idempotent, it can be called multiple times without issues if (_quit) { return; } if (!_rootItem) { + deleteLater(); return; } + _paused = true; if (_renderTimer) { - _renderTimer->stop(); QObject::disconnect(_renderTimer); + _renderTimer->deleteLater(); } - if (_renderControl) { - QObject::disconnect(_renderControl); - } - + QObject::disconnect(_renderControl); QObject::disconnect(qApp); { QMutexLocker lock(&_mutex); _quit = true; - if (_renderObject) { - QCoreApplication::postEvent(_renderObject, new OffscreenEvent(OffscreenEvent::Quit), Qt::HighEventPriority); - } + QCoreApplication::postEvent(_renderObject, new OffscreenEvent(OffscreenEvent::Quit), Qt::HighEventPriority); } // Block until the rendering thread has stopped // FIXME this is undesirable because this is blocking the main thread, // but I haven't found a reliable way to do this only at application // shutdown - if (_renderThread) { - _renderThread->wait(); - delete _renderThread; - _renderThread = nullptr; - } + _renderThread->wait(); } diff --git a/libraries/render-utils/src/AbstractViewStateInterface.h b/libraries/render-utils/src/AbstractViewStateInterface.h index 54fdc903ca..96e9f4d222 100644 --- a/libraries/render-utils/src/AbstractViewStateInterface.h +++ b/libraries/render-utils/src/AbstractViewStateInterface.h @@ -37,25 +37,15 @@ public: virtual glm::vec3 getAvatarPosition() const = 0; - // Unfortunately, having this here is a bad idea. Lots of objects connect to - // the aboutToQuit signal, and it's impossible to know the order in which - // the receivers will be called, so this might return false negatives - //virtual bool isAboutToQuit() const = 0; - - // Queue code to execute on the main thread. - // If called from the main thread, the lambda will execute synchronously - virtual void postLambdaEvent(const std::function& f) = 0; - // Synchronously execute code on the main thread. This function will - // not return until the code is executed, regardles of which thread it - // is called from - virtual void sendLambdaEvent(const std::function& f) = 0; + virtual bool isAboutToQuit() const = 0; + virtual void postLambdaEvent(std::function f) = 0; virtual qreal getDevicePixelRatio() = 0; virtual render::ScenePointer getMain3DScene() = 0; virtual render::EnginePointer getRenderEngine() = 0; - virtual void pushPostUpdateLambda(void* key, const std::function& func) = 0; + virtual void pushPostUpdateLambda(void* key, std::function func) = 0; virtual bool isHMDMode() const = 0; @@ -64,4 +54,5 @@ public: static void setInstance(AbstractViewStateInterface* instance); }; -#endif // hifi_AbstractViewStateInterface_h + +#endif // hifi_AbstractViewStateInterface_h diff --git a/tests/render-perf/src/main.cpp b/tests/render-perf/src/main.cpp index 9249b3d957..93672cc5a2 100644 --- a/tests/render-perf/src/main.cpp +++ b/tests/render-perf/src/main.cpp @@ -453,8 +453,8 @@ protected: return vec3(); } - void postLambdaEvent(const std::function& f) override {} - void sendLambdaEvent(const std::function& f) override {} + bool isAboutToQuit() const override { return false; } + void postLambdaEvent(std::function f) override {} qreal getDevicePixelRatio() override { return 1.0f; @@ -469,7 +469,7 @@ protected: } std::map> _postUpdateLambdas; - void pushPostUpdateLambda(void* key, const std::function& func) override { + void pushPostUpdateLambda(void* key, std::function func) override { _postUpdateLambdas[key] = func; } From 6c0177e2bdfc086ed7f1e83e8d70df02a0993c9c Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Wed, 25 Apr 2018 11:50:53 -0700 Subject: [PATCH 14/14] use the correct joint count for other avatar last sent --- assignment-client/src/avatars/AvatarMixerClientData.h | 6 +----- assignment-client/src/avatars/AvatarMixerSlave.cpp | 3 +++ 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixerClientData.h b/assignment-client/src/avatars/AvatarMixerClientData.h index 7a7210a0e8..e6b7d9d121 100644 --- a/assignment-client/src/avatars/AvatarMixerClientData.h +++ b/assignment-client/src/avatars/AvatarMixerClientData.h @@ -115,11 +115,7 @@ public: uint64_t getLastOtherAvatarEncodeTime(QUuid otherAvatar) const; void setLastOtherAvatarEncodeTime(const QUuid& otherAvatar, const uint64_t& time); - QVector& getLastOtherAvatarSentJoints(QUuid otherAvatar) { - auto& lastOtherAvatarSentJoints = _lastOtherAvatarSentJoints[otherAvatar]; - lastOtherAvatarSentJoints.resize(_avatar->getJointCount()); - return lastOtherAvatarSentJoints; - } + QVector& getLastOtherAvatarSentJoints(QUuid otherAvatar) { return _lastOtherAvatarSentJoints[otherAvatar]; } void queuePacket(QSharedPointer message, SharedNodePointer node); int processPackets(); // returns number of packets processed diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index fb4b65726a..6f19b73cc5 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -381,6 +381,9 @@ void AvatarMixerSlave::broadcastAvatarDataToAgent(const SharedNodePointer& node) bool includeThisAvatar = true; auto lastEncodeForOther = nodeData->getLastOtherAvatarEncodeTime(otherNode->getUUID()); QVector& lastSentJointsForOther = nodeData->getLastOtherAvatarSentJoints(otherNode->getUUID()); + + lastSentJointsForOther.resize(otherAvatar->getJointCount()); + bool distanceAdjust = true; glm::vec3 viewerPosition = myPosition; AvatarDataPacket::HasFlags hasFlagsOut; // the result of the toByteArray