Fixed a null reference exception seen during a login attempt

[appurist]

Reported by Vinhold on GW grid, probably a NRE resulting from crash/quit/disconnect during login. Also added comments on the crapaciousness of some of this code, where appropriate.

2017-04-19 10:51:09,610 [STP SmartThreadPool Thread #0] ERROR
OpenSim.Framework.Servers.HttpServer.XmlRpcStreamHandler - [XMLRPC
STREAM HANDLER]: Requested method [login_to_simulator] from 192.168.1.1
threw exception: Object reference not set to an instance of an object.
at
OpenSim.Grid.MessagingServer.Modules.MessageService.enqueuePresenceUpdate(UserPresenceData
talkingAbout, UserPresenceData receiver) in
X:\halcyon-0.9.32.6272\OpenSim\Grid\MessagingServer.Modules\MessageService.cs:line
142
at
OpenSim.Grid.MessagingServer.Modules.MessageService.SubscribeToPresenceUpdates(UserPresenceData
userpresence, UserPresenceData friendpresence, FriendListItem
uFriendListItem) in
X:\halcyon-0.9.32.6272\OpenSim\Grid\MessagingServer.Modules\MessageService.cs:line
186
at
OpenSim.Grid.MessagingServer.Modules.MessageService.ProcessFriendListSubscriptions(UserPresenceData
userpresence) in
X:\halcyon-0.9.32.6272\OpenSim\Grid\MessagingServer.Modules\MessageService.cs:line
125
at
OpenSim.Grid.MessagingServer.Modules.MessageService.UserLoggedOn(XmlRpcRequest
request, IPEndPoint remoteClient) in
X:\halcyon-0.9.32.6272\OpenSim\Grid\MessagingServer.Modules\MessageService.cs:line
485

[appurist]

I've merged the other straight-forward PRs but this one is less obvious. It is an incremental improvement over the old code to make the null-reference conditional so as to avoid the exception, but I'm not sure the handling of that case is good enough. It seems to be incrementally better but could probably use more attention. Wondering if @ddaeschler has any better ideas than just merging this as-is?

[ddaeschler]

I would think an invalid region handle should probably shunt the user to their home as if the region is down. The conditions of this crash seem odd

[appurist]

The conditions are certainly odd, but they are real-world (actual exception report included above). There is no option to route the user anywhere; this isn't processing the login, it's processing the result of a login that has already happened. This is code in the Messaging service, notifying friends of this user's login status change.

The region in question is the region that one of the friends is supposedly in. We get into this error situation when the friend's login is also in transition.

I believe what has happened in this case is that two users, friends, have logged in or logged out at roughly the same time and the friend's region is in transition (e.g. two friends in a region when it is restarted, or at least the friend is likely in a region being restarted). The problem is the old line 482, which where GetRegionInfo may return (has returned, in this case) null. This is then passed to ProcessFriendListSubscriptions even though the region lookup failed:

up.regionData = m_regionModule.GetRegionInfo(regionHandle);
up.OnlineYN = true;
up.lookupUserRegionYN = false;
ProcessFriendListSubscriptions(up);

So that code unconditionally passes a up with a null regionData, which ProcessFriendListSubscriptions barfs on. The problem is that there is no check for a null on the GetRegionInfo call, even though it passes the result to a function that needs it.

This is really a question of how to handle not being able to notify a friend of this user's presence status change.

I think the answer is likely that there is no friend there anyway, the region they were on is down or being restarted, but their presence still shows as online. This inconsistency in online status is almost certainly the result of #367, now also fixed, but there are other conditions that could produce the same status inconsistency (e.g. temporarily after a viewer crash).

Now that I've reviewed this further, I think this commit is handling it properly. If GetRegionInfo returns a null, it takes no action. The friend "in the region we can't look up" isn't notified of this user's status change. (Previously, it triggered the null-reference exception.) Only the null check is new:

    // Null reference exception in een by Vinhold on GW grid in enqueuePresenceUpdate with a null receiver.regionData
    // Should never be null but GetRegionInfo() can return that. Probably crash/quit/disconnect during login.
    RegionProfileData regionData = m_regionModule.GetRegionInfo(regionHandle);
    if (regionData != null) // else go with non-null default UserPresenceData.regionData
    {
        up.regionData = regionData;
        up.OnlineYN = true;
        up.lookupUserRegionYN = false;
        ProcessFriendListSubscriptions(up);
    }

    return new XmlRpcResponse();
}