World Crash in Client::GetCharacter()

[John Adams]

Getting a lot more crashes than usual again. If this is a work-in-progress, please let me know what rev to roll New Telon back to until the work is finished.

Stack

Code: Select all

>	WorldServer.exe!std::_Ptr_base<WorldCharacter>::_Reset<WorldCharacter>(const std::_Ptr_base<WorldCharacter> & _Other, bool _Throw) Line 363	C++
 	WorldServer.exe!std::shared_ptr<WorldCharacter>::shared_ptr<WorldCharacter><WorldCharacter>(const std::weak_ptr<WorldCharacter> & _Other, bool _Throw) Line 552	C++
 	WorldServer.exe!std::weak_ptr<WorldCharacter>::lock() Line 1051	C++
 	WorldServer.exe!Client::GetCharacter() Line 103	C++
 	WorldServer.exe!ChunkServer::DisconnectChunkClient(std::shared_ptr<Client> chunk_client, unsigned short reason_id) Line 2352	C++
 	WorldServer.exe!UDPServer::KickDupeClients(unsigned int account_id, unsigned int connection_id) Line 249	C++
 	WorldServer.exe!Net::HandleCharacterSelected(std::shared_ptr<Client> & client, PacketStruct * packet_struct) Line 377	C++
 	WorldServer.exe!Net::Process() Line 159	C++
 	WorldServer.exe!main(int argc, char * * argv) Line 159	C++
 	WorldServer.exe!__tmainCRTStartup() Line 241	C
 	WorldServer.exe!mainCRTStartup() Line 164	C
 	kernel32.dll!76a1338a()	Unknown
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	
 	ntdll.dll!77c49f72()	Unknown
 	ntdll.dll!77c49f45()	Unknown

Code:

Code: Select all

shared_ptr<WorldCharacter> Client::GetCharacter() {    return character.lock();}

"character" is unreadable data.

[Lokked]

All I could find online about shared_ptr crashes on Reset() is there may be a double delete happening. At this time, I don't see this happening anywhere.

I don't know if this bit of info will fix the above, but this failed on the GetCharacter() function, so perhaps it will:
When we call GetCharacter(), this returns a shared_ptr<WorldCharacter> object ONLY IF the WorldCharacter object has not been deleted. If the WorldCharacter object has been deleted (for whatever reason, all shared_ptrs of this object have gone out of scope), it will return nullptr (0, basically). We have to check for this.

In our code, the first time in a code block that we need to obtain a WorldCharacter object from the Client object, we must do one of these things:

Code: Select all

auto character = client->GetCharacter();
if (!character)
    //return or do something else

or
auto character = client->GetCharacter();
if (character)
    //Perform this code block. character will contain a valid WorldCharacter object for the rest of this code block.

Be smart about chaining functions with GetCharacter(). Doing a client->GetCharacter()->SomeWorldCharacterFunction() without being sure that the WorldCharacter object is valid could result in a crash.

If we need to use the same WorldCharacter object multiple times in the same function, avoid doing multiple client->GetCharacter() calls, unless you plan on checking every one of them: If you call client->GetCharacter() + Check at the start of the function, that WorldCharacter object will persist through the entire function, at LEAST until the function returns, where it may or may not be destroyed if it's the last reference.

Briefly searching the code, here are a few instances where there is no check after client->GetCharacter(). Please note that I'm not saying these functions are necessarily dangerous: It may be that they are not called unless there is a valid WorldCharacter object in the calling function, in which case the WorldCharacter object will also exist in these functions, but it would take more analysis to determine this. Probably just easier to put checks in place.
ChunkServer::SendCharacterTitles()
ChunkServer::SendCharacterFactions()
ChunkServer::SendPlayerControllerInitPackets()
ChunkServer::HandleChatSay() //Lots of calls to GetCharacter(). Might be best to have 1 at the start, saving the WorldCharacter to a variable (auto character), so it can persist for the duration of the function.


**Edit - Well I was on the right track. I found another instance in KickDupeClients(), and this might speak more to the error John posted above.
This section (which was written by me, doh!):

Code: Select all

#ifdef VG_WORLD
if (client->GetCharacter()) {
    client->GetCharacter()->GetCurrentChunk()->DisconnectChunkClient(client->GetCharacter()->GetCurrentChunk()->GetClient(client->GetAccountID()),CLIENT_DISCONNECT_NEW_CONNECTION_ATTEMPT);
}

is not safe.

I can't do this right now, but when I get home, I'll change this to:

Code: Select all

#ifdef VG_WORLD
auto character = client->GetCharacter();
if (character) {
    character->GetCurrentChunk()->DisconnectChunkClient(character->GetCurrentChunk()->GetClient(client->GetAccountID()),CLIENT_DISCONNECT_NEW_CONNECTION_ATTEMPT);
}

[Volt]

Code: Select all

.rift isleofshadow

makes my Vgclient go bang.

[Xinux]

[quote="Volt"]

Code: Select all

.rift isleofshadow

makes my Vgclient go bang.[/quote]


Chunk table in your DB need's to be changed

Delete ID 110 (Isle of Shadow)

Change ID 113 from trialisland to IsleofShadow and Isle of Shadow

[Lokked]

I've committed Rev 676 to Dev SVN to attempt to mitigate the crashes in the OP, based on my post above.

[jimbo]

Hi - I've made the changes to the chunk table as suggested - and can confirm that rifting to chunk isleofshadow now works as expected - make sure you restart server after making chunk table change.

[Blackstorm]

Hi,
I don't know if it's the same thing, but each time i want to go to newtargonor the server kick me, and i am back to the character selection.
If i relog my character, and retry the command : .rift newtargonor, it's work fine.

[jimbo]

Hi - I've been using these coords for New Targonor Chunk - ( -72027, -98530, 35516 ) - this will bring you right to the Riftway at New Targonor

Dev's - would it be useful for you if we tested 'rifting' to all chunks - and possibly suggesting chunk start coords for you - i.e if there is a riftway in that chunk - then
next to it - otherwise a piece of land in the chunk as opposed to water - I'm not sure how you came up with the existing Chunk start point in the chunk table - maybe you had some purpose for picking those points?

Jim

[Xinux]

Yea i think every start point should be next to a rift it has one if it doesn't have one then a nice safe spot in the chunk will work.

The current start points we actually got from the map files in the clients so there is no real purpose to where they are now.

[jimbo]

I'm currently rifting to every chunk - and will find suggested coords for each one for you - as well as check for any other chunk issues - will take me a while
but I had a nasty encounter with an escalator (moving stairs) yesty and can' move around much - let just say bouncing down an escalator on your back is not the best approach !!

Jim