From ce8441132e43dc8e3579f413daf914d48b8f115e Mon Sep 17 00:00:00 2001
From: "Justin Clark-Casey (justincc)" <jjustincc@googlemail.com>
Date: Fri, 11 Nov 2011 22:37:57 +0000
Subject: [PATCH] Restore sending of OutPacket() for object kills removed in
 commit c7dd7b1.

OutPacket() must be called within the m_killRecord lock.  Otherwise the following event sequence is possible
1) LLClientView.ProcessEntityUpdates() passes the kill record check for a particular part suspends before OutPacket()
2) Another thread calls LLClientView.SendKillObject() to delete the same part and modifies the kill record
3) The same thread places the kill packet on the Task queue.
4) The earlier thread resumes and places the update packet on the Task queue after the kill packet.
This results in a ghost part in the sim that only goes away after client relog.
This commit also removes the unnecessary m_entityUpdates.SyncRoot locking in SendKillObject.
---
 .../ClientStack/Linden/UDP/LLClientView.cs    | 46 ++++++++-----------
 1 file changed, 20 insertions(+), 26 deletions(-)

diff --git a/OpenSim/Region/ClientStack/Linden/UDP/LLClientView.cs b/OpenSim/Region/ClientStack/Linden/UDP/LLClientView.cs
index 4a0b0c6dd1..626ebb5939 100644
--- a/OpenSim/Region/ClientStack/Linden/UDP/LLClientView.cs
+++ b/OpenSim/Region/ClientStack/Linden/UDP/LLClientView.cs
@@ -1546,41 +1546,35 @@ namespace OpenSim.Region.ClientStack.LindenUDP
             kill.Header.Reliable = true;
             kill.Header.Zerocoded = true;
 
-            lock (m_killRecord)
+            if (localIDs.Count == 1 && m_scene.GetScenePresence(localIDs[0]) != null)
             {
-                if (localIDs.Count == 1)
+                OutPacket(kill, ThrottleOutPacketType.State);
+            }
+            else
+            {
+                // We MUST lock for both manipulating the kill record and sending the packet, in order to avoid a race
+                // condition where a kill can be processed before an out-of-date update for the same object.
+                // ProcessEntityUpdates() also takes the m_killRecord lock.
+                lock (m_killRecord)
                 {
-                    if (m_scene.GetScenePresence(localIDs[0]) != null)
-                    {
-                        OutPacket(kill, ThrottleOutPacketType.State);
-                        return;
-                    }
-                    m_killRecord.Add(localIDs[0]);
-                }
-                else
-                {
-                    lock (m_entityUpdates.SyncRoot)
-                    {
-                        foreach (uint localID in localIDs)
-                            m_killRecord.Add(localID);
-                    }
+                    foreach (uint localID in localIDs)
+                        m_killRecord.Add(localID);
+
+                    // The throttle queue used here must match that being used for updates.  Otherwise, there is a
+                    // chance that a kill packet put on a separate queue will be sent to the client before an existing
+                    // update packet on another queue.  Receiving updates after kills results in unowned and undeletable
+                    // scene objects in a viewer until that viewer is relogged in.
+                    OutPacket(kill, ThrottleOutPacketType.Task);
                 }
             }
-
-            // The throttle queue used here must match that being used for
-            // updates. Otherwise, there is a chance that a kill packet put
-            // on a separate queue will be sent to the client before an
-            // existing update packet on another queue. Receiving updates
-            // after kills results in unowned and undeletable
-            // scene objects in a viewer until that viewer is relogged in.
-            OutPacket(kill, ThrottleOutPacketType.Task);
         }
 
         /// <summary>
         /// Send information about the items contained in a folder to the client.
-        ///
-        /// XXX This method needs some refactoring loving
         /// </summary>
+        /// <remarks>
+        /// XXX This method needs some refactoring loving
+        /// </remarks>
         /// <param name="ownerID">The owner of the folder</param>
         /// <param name="folderID">The id of the folder</param>
         /// <param name="items">The items contained in the folder identified by folderID</param>