Defer processing AvatarAppearance until after first ObjectUpdate (#6017)

* make AvatarAppearance a low-priority message

* remember agent_id of unexpected AvatarAppearance message for later

9 files changed, 128 insertions, 25 deletions

diff --git a/indra/llmessage/llpacketbuffer.h b/indra/llmessage/llpacketbuffer.h
index b0505e7afd..a08e6c22e5 100644
--- a/indra/llmessage/llpacketbuffer.h
+++ b/indra/llmessage/llpacketbuffer.h
@@ -49,11 +49,18 @@ public:
     void init(S32 hSocket);
     void init(const char* buffer, S32 data_size, const LLHost& host);
 
+    // Whether LLCircuitData::checkPacketInID() has already been run for this
+    // packet (done at socket-read time, before this packet was sorted into
+    // the high/low priority inbound queue).
+    bool    getPacketIDChecked() const          { return mPacketIDChecked; }
+    void    setPacketIDChecked(bool checked)    { mPacketIDChecked = checked; }
+
 protected:
     char    mData[NET_BUFFER_SIZE]; // packet data       /* Flawfinder : ignore */
     S32     mSize;                  // size of buffer in bytes
     LLHost  mHost;                  // source/dest IP and port
     LLHost  mReceivingIF;           // source/dest IP and port
+    bool    mPacketIDChecked = false;
 };
 
 #endif
diff --git a/indra/llmessage/message.cpp b/indra/llmessage/message.cpp
index 52180b20f1..8d86856706 100644
--- a/indra/llmessage/message.cpp
+++ b/indra/llmessage/message.cpp
@@ -519,10 +519,11 @@ bool LLMessageSystem::checkMessages(LockMessageChecker&, S64 frame_count )
         bool recv_resent = false;
         S32 num_acks = 0;
         S32 true_rcv_size = 0;
+        bool recv_packet_id_checked = false;
 
         U8* buffer = mTrueReceiveBuffer;
 
-        mTrueReceiveSize = receivePacketOrDrop((char *)mTrueReceiveBuffer);
+        mTrueReceiveSize = receivePacketOrDrop((char *)mTrueReceiveBuffer, recv_packet_id_checked);
         // If you want to dump all received packets into SecondLife.log, uncomment this
         //dumpPacketToLog();
 
@@ -686,7 +687,7 @@ bool LLMessageSystem::checkMessages(LockMessageChecker&, S64 frame_count )
 
             if ( valid_packet )
             {
-                logValidMsg(cdp, host, recv_reliable, recv_resent, num_acks>0 );
+                logValidMsg(cdp, host, recv_reliable, recv_resent, num_acks>0, recv_packet_id_checked );
                 valid_packet = mTemplateMessageReader->readMessage(buffer, host);
             }
 
@@ -866,19 +867,37 @@ bool LLMessageSystem::computeDrop()
 bool LLMessageSystem::isHighPriorityMessage(const LLPacketBuffer& pkt) const
 {
     S32 size = pkt.getSize();
-    if (size < LL_PACKET_ID_SIZE + 1)
+
+    // avoid stepping out of bounds
+    const S32 MIN_VALID_PACKET_SIZE = LL_PACKET_ID_SIZE + 4;
+    if (size < MIN_VALID_PACKET_SIZE)
     {
         return false;
     }
 
-    // We want to prioritize crucial messages use to establish viewer <--> simulator connection,
+    // We want to prioritize crucial messages used to establish viewer <--> simulator connection,
     // which are all low-frequency. A simple approximation is to just prioritize all non high-
     // frequency messages.
     //
-    // High frequency messages use a single byte for message_id whereas all low- and medium-
-    // frequency messages have 255 at the first byte of the message_id (which is after the
-    // LL_PACKET_ID_SIZE bytes of packet_id).
-    return *((const U8*)pkt.getData() + LL_PACKET_ID_SIZE) == 255;
+    // High frequency messages use a single byte for message_id whereas medium- and low-
+    // frequency messages have 255 at the first byte (which is after the LL_PACKET_ID_SIZE
+    // bytes of packet_id).
+    const U8* header = (const U8*)pkt.getData() + LL_PACKET_ID_SIZE;
+    if (header[0] != 255)
+    {
+        // high-frequency message
+        return false;
+    }
+
+    // BUG: The low-frequency AvatarAppearance message will be ignored if its agent is unknown
+    // and the agent is created upon receipt of its first high-frequency ObjectUpdate message.
+    // This race condition will be exacerbated by our default prioritization strategy.
+    //
+    // WORKAROUND: All middle- and low-frequency messages are high-priority except AvatarAppearance
+    //
+    // AvatarAppearance is "Low 158" which means it is stored in four bytes: 0xff 0xff 0x00 0x9E
+    // the last two bytes represent 158 in a BigEndian U16
+    return header[1] != 255 && header[2] != 0 && header[3] != 158;
 }
 
 void LLMessageSystem::dropPackets(U32 num_to_drop)
@@ -891,8 +910,10 @@ void LLMessageSystem::setDropPercentage(F32 percent_to_drop)
     mDropPercentage = percent_to_drop;
 }
 
-S32 LLMessageSystem::receivePacketOrDrop(char* datap)
+S32 LLMessageSystem::receivePacketOrDrop(char* datap, bool& packet_id_already_checked)
 {
+    packet_id_already_checked = false;
+
     if (getNumBufferedPackets() > 0)
     {
         LLHost invalid_host;
@@ -905,6 +926,7 @@ S32 LLMessageSystem::receivePacketOrDrop(char* datap)
         S32 packet_size = pkt.getSize();
         mLastSender      = pkt.getHost();
         mLastReceivingIF = pkt.getReceivingInterface();
+        packet_id_already_checked = pkt.getPacketIDChecked();
 
         if (packet_size > 0)
         {
@@ -913,7 +935,8 @@ S32 LLMessageSystem::receivePacketOrDrop(char* datap)
         return packet_size;
     }
 
-    // Read directly from the socket.
+    // Read directly from the socket. checkPacketInID() has not run yet for
+    // this packet.
     bool drop = computeDrop();
     S32 packet_size = 0;
     if (LLProxy::isSOCKSProxyEnabled())
@@ -1034,10 +1057,24 @@ S32 LLMessageSystem::bufferInboundPacket()
             }
         }
 
-        // ACK inbound reliable packet ASAP
-        if (cdp && (data[0] & LL_RELIABLE_FLAG))
+        if (cdp)
         {
-            cdp->collectRAck(recv_packet_id);
+            // ACK inbound reliable packet ASAP
+            if ((data[0] & LL_RELIABLE_FLAG))
+            {
+                cdp->collectRAck(recv_packet_id);
+            }
+
+            // Check packet sequencing here, in true socket-arrival order, before
+            // this packet is sorted into the high/low priority inbound queue.
+            // Skip genuine duplicate resends, same as checkMessages()/logValidMsg()
+            // would do further downstream.
+            bool recv_resent = (data[0] & LL_RESENT_FLAG) != 0;
+            if (!recv_resent || !cdp->isDuplicateResend(recv_packet_id))
+            {
+                cdp->checkPacketInID(recv_packet_id, recv_resent);
+                pkt.setPacketIDChecked(true);
+            }
         }
 
         if (isHighPriorityMessage(pkt))
@@ -1668,7 +1705,13 @@ void LLMessageSystem::logTrustedMsgFromUntrustedCircuit( const LLHost& host )
     }
 }
 
-void LLMessageSystem::logValidMsg(LLCircuitData *cdp, const LLHost& host, bool recv_reliable, bool recv_resent, bool recv_acks )
+void LLMessageSystem::logValidMsg(
+        LLCircuitData *cdp,
+        const LLHost& host,
+        bool recv_reliable,
+        bool recv_resent,
+        bool recv_acks,
+        bool skip_packet_id_check )
 {
     if (mNumMessageCounts >= MAX_MESSAGE_COUNT_NUM)
     {
@@ -1685,8 +1728,13 @@ void LLMessageSystem::logValidMsg(LLCircuitData *cdp, const LLHost& host, bool r
 
     if (cdp)
     {
-        // update circuit packet ID tracking (missing/out of order packets)
-        cdp->checkPacketInID( mCurrentRecvPacketID, recv_resent );
+        if (!skip_packet_id_check)
+        {
+            // update circuit packet ID tracking (missing/out of order packets)
+            // Already done in bufferInboundPacket(), in true socket-arrival
+            // order, if this packet came off the high/low priority queues.
+            cdp->checkPacketInID( mCurrentRecvPacketID, recv_resent );
+        }
         cdp->addBytesIn( (S32Bytes)mTrueReceiveSize );
     }
 
diff --git a/indra/llmessage/message.h b/indra/llmessage/message.h
index e81cfe16a9..e23a67c04a 100644
--- a/indra/llmessage/message.h
+++ b/indra/llmessage/message.h
@@ -910,7 +910,7 @@ private:
 
     void        logMsgFromInvalidCircuit( const LLHost& sender, bool recv_reliable );
     void        logTrustedMsgFromUntrustedCircuit( const LLHost& sender );
-    void        logValidMsg(LLCircuitData *cdp, const LLHost& sender, bool recv_reliable, bool recv_resent, bool recv_acks );
+    void        logValidMsg(LLCircuitData *cdp, const LLHost& sender, bool recv_reliable, bool recv_resent, bool recv_acks, bool skip_packet_id_check );
 
     class LLMessageCountInfo
     {
@@ -967,8 +967,10 @@ private:
 
     // Receive one packet: pop from ring if buffered, else read from mSocket.
     // Sets mLastSender and mLastReceivingIF.
+    // Sets packet_id_already_checked to whether checkPacketInID() was already
+    // run for this packet back when it was buffered (see bufferInboundPacket()).
     // Returns packet_size, or 0 if no packet or packet was dropped.
-    S32  receivePacketOrDrop(char* datap);
+    S32  receivePacketOrDrop(char* datap, bool& packet_id_already_checked);
 
     // Read one raw packet from mSocket into inbound message queues
     // Returns packet_size (0 if no packet was available).
diff --git a/indra/llprimitive/llprimitive.cpp b/indra/llprimitive/llprimitive.cpp
index 7a638dd625..fd755e35dd 100644
--- a/indra/llprimitive/llprimitive.cpp
+++ b/indra/llprimitive/llprimitive.cpp
@@ -1370,6 +1370,12 @@ bool LLPrimitive::packTEMessage(LLDataPacker &dp) const
 
 S32 LLPrimitive::parseTEMessage(LLMessageSystem* mesgsys, char const* block_name, const S32 block_num, LLTEContents& tec)
 {
+    return parseTEMessage(mesgsys, block_name, block_num, tec, getNumTEs());
+}
+
+// static
+S32 LLPrimitive::parseTEMessage(LLMessageSystem* mesgsys, char const* block_name, const S32 block_num, LLTEContents& tec, U8 face_count)
+{
     S32 retval = 0;
     // temp buffer for material ID processing
     // data will end up in tec.material_id[]
@@ -1403,7 +1409,7 @@ S32 LLPrimitive::parseTEMessage(LLMessageSystem* mesgsys, char const* block_name
     tec.packed_buffer[tec.size] = 0x00;
     ++tec.size;
 
-    tec.face_count = llmin((U32)getNumTEs(),(U32)LLTEContents::MAX_TES);
+    tec.face_count = llmin((U32)face_count,(U32)LLTEContents::MAX_TES);
 
     U8 *cur_ptr = tec.packed_buffer;
     LL_DEBUGS("TEXTUREENTRY") << "Texture Entry with buffere sized: " << tec.size << LL_ENDL;
diff --git a/indra/llprimitive/llprimitive.h b/indra/llprimitive/llprimitive.h
index c3e3e19ee9..21bf52610e 100644
--- a/indra/llprimitive/llprimitive.h
+++ b/indra/llprimitive/llprimitive.h
@@ -502,6 +502,9 @@ public:
     S32 unpackTEMessage(LLMessageSystem* mesgsys, char const* block_name, const S32 block_num); // Variable num of blocks
     S32 unpackTEMessage(LLDataPacker &dp);
     S32 parseTEMessage(LLMessageSystem* mesgsys, char const* block_name, const S32 block_num, LLTEContents& tec);
+    // Same as above, but usable before an instance exists to derive the face count from
+    // (e.g. decoding a message for an object that hasn't been created yet).
+    static S32 parseTEMessage(LLMessageSystem* mesgsys, char const* block_name, const S32 block_num, LLTEContents& tec, U8 face_count);
     S32 applyParsedTEMessage(LLTEContents& tec);
 
 #ifdef CHECK_FOR_FINITE
diff --git a/indra/newview/llagent.cpp b/indra/newview/llagent.cpp
index 35d60e6595..2f0ad15d18 100644
--- a/indra/newview/llagent.cpp
+++ b/indra/newview/llagent.cpp
@@ -4151,6 +4151,7 @@ void LLAgent::handleTeleportFinished()
     }
     clearTeleportRequest();
     mTeleportCanceled.reset();
+    LLVOAvatar::resetEarlyAppearanceList();
     if (mIsMaturityRatingChangingDuringTeleport)
     {
         // notify user that the maturity preference has been changed
diff --git a/indra/newview/llviewermessage.cpp b/indra/newview/llviewermessage.cpp
index 1f1ff30b8f..e435ce08e3 100644
--- a/indra/newview/llviewermessage.cpp
+++ b/indra/newview/llviewermessage.cpp
@@ -4181,11 +4181,12 @@ void process_avatar_appearance(LLMessageSystem *mesgsys, void **user_data)
     if (avatarp)
     {
         avatarp->processAvatarAppearance( mesgsys );
+        return;
     }
-    else
-    {
-        LL_WARNS("Messaging") << "avatar_appearance sent for unknown avatar " << uuid << LL_ENDL;
-    }
+    // The avatar object doesn't exist yet.
+    // We will re-request its appearance data after it is created.
+    LLVOAvatar::registerEarlyAppearance(uuid);
+    LL_WARNS("Messaging") << "AvatarAppearance received for avatar " << uuid << " before object created" << LL_ENDL;
 }
 
 void process_camera_constraint(LLMessageSystem *mesgsys, void **user_data)
diff --git a/indra/newview/llvoavatar.cpp b/indra/newview/llvoavatar.cpp
index cd71a2c660..89ee5ede2a 100644
--- a/indra/newview/llvoavatar.cpp
+++ b/indra/newview/llvoavatar.cpp
@@ -615,6 +615,8 @@ const LLUUID LLVOAvatar::sStepSounds[LL_MCODE_END] =
     SND_RUBBER_RUBBER
 };
 
+uuid_list_t LLVOAvatar::sEarlyAppearanceList;
+
 S32 LLVOAvatar::sRenderName = RENDER_NAME_ALWAYS;
 S32 LLVOAvatar::sRenderGroupTitles = RENDER_GROUP_TITLE_ALWAYS;
 S32 LLVOAvatar::sNumVisibleChatBubbles = 0;
@@ -779,6 +781,20 @@ LLVOAvatar::LLVOAvatar(const LLUUID& id,
     mVisuallyMuteSetting = LLVOAvatar::VisualMuteSettings(LLRenderMuteList::getInstance()->getSavedVisualMuteSetting(getID()));
 
     sInstances.push_back(this);
+
+    uuid_list_t::iterator it = sEarlyAppearanceList.find(id);
+    if (it != sEarlyAppearanceList.end())
+    {
+        // Note: aside from LLVOAvatar::resetEarlyAppearanceList() (called on
+        // teleport), this is the only place where we remove from
+        // sEarlyAppearanceList, which means any agent who receives an
+        // AvatarAppearance message but is never actually instantiated will
+        // remain on the list until the next teleport. This is a resource leak
+        // but we expect it to be small enough per-session to not cause problems.
+        sEarlyAppearanceList.erase(it);
+        LL_INFOS("Avatar") << "Re-requesting AvatarAppearance for new avatar " << id << LL_ENDL;
+        LLAvatarPropertiesProcessor::getInstance()->sendAvatarTexturesRequest(getID());
+    }
 }
 
 std::string LLVOAvatar::avString() const
@@ -9937,7 +9953,7 @@ void LLVOAvatar::applyParsedAppearanceMessage(LLAppearanceMessageContents& conte
         if (visualParamWeightsAreDefault() && mRuthTimer.getElapsedTimeF32() > LOADING_TIMEOUT_SECONDS)
         {
             // re-request appearance, hoping that it comes back with a shape next time
-            LL_INFOS() << "Re-requesting AvatarAppearance for object: "  << getID() << LL_ENDL;
+            LL_INFOS() << "Re-requesting AvatarAppearance for agent: "  << getID() << LL_ENDL;
             LLAvatarPropertiesProcessor::getInstance()->sendAvatarTexturesRequest(getID());
             mRuthTimer.reset();
         }
diff --git a/indra/newview/llvoavatar.h b/indra/newview/llvoavatar.h
index 3f33dc54b8..3457e7bb6a 100644
--- a/indra/newview/llvoavatar.h
+++ b/indra/newview/llvoavatar.h
@@ -949,9 +949,26 @@ protected:
  **                    APPEARANCE
  **/
 
+public:
+    // Used when an AvatarAppearance UDP message is received before the
+    // corresponding avatar could be created.
+    static void registerEarlyAppearance(const LLUUID& av_id)
+    {
+        sEarlyAppearanceList.emplace(av_id);
+    }
+
+    // Entries left behind by agents who never get instantiated (e.g. an
+    // AvatarAppearance message arrives for an avatar we never rez) are a
+    // small resource leak. Teleporting to another region invalidates the
+    // whole list, since it was only ever relevant to avatars in the region
+    // we're leaving, so clear it out at that point to bound the leak.
+    static void resetEarlyAppearanceList()
+    {
+        sEarlyAppearanceList.clear();
+    }
+
     LLPointer<LLAppearanceMessageContents>  mLastProcessedAppearance;
 
-public:
     void            parseAppearanceMessage(LLMessageSystem* mesgsys, LLAppearanceMessageContents& msg);
     void            processAvatarAppearance(LLMessageSystem* mesgsys);
     void            applyParsedAppearanceMessage(LLAppearanceMessageContents& contents, bool slam_params);
@@ -983,6 +1000,8 @@ private:
     bool            mIsEditingAppearance; // flag for if we're actively in appearance editing mode
     bool            mUseLocalAppearance; // flag for if we're using a local composite
 
+    static uuid_list_t  sEarlyAppearanceList;
+
     //--------------------------------------------------------------------
     // Visibility
     //--------------------------------------------------------------------