Defer processing AvatarAppearance until after first ObjectUpdate (#6017)

* make AvatarAppearance a low-priority message

* remember agent_id of unexpected AvatarAppearance message for later

1 files changed, 64 insertions, 16 deletions

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 );
     }