Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash caused by freeing internal V8 BackingStore memory #64

Merged

Conversation

MoritzBrueckner
Copy link
Contributor

@MoritzBrueckner MoritzBrueckner commented May 25, 2023

This PR fixes a crash that would happen in random places after krom_create_texture_from_bytes() was called.

According to this document which I found here (linked from the BackingStore docstring), references to BackingStore objects have to be managed via shared pointers that automatically free the backing store memory when the last reference is removed. Since krom_create_texture_from_bytes() manually freed the backing store data if the image was not set to be readable, there would be a crash later on when V8 attempted to free the memory, for example when the Haxe array to which the backing store corresponded got GC'd. I guess similar issues would happen if the array was accessed again from Haxe.

This was quite difficult to find, since there were a dozen or so different stack traces of the crash that were completely different every time the crash occured and the crash happened in various different background threads. I guess this was due to the fact that the time at which the last reference from the backing store gets removed varies a lot in V8. Luckily there was one stack trace that actually contained an entry for the BackingStore destructor:

 	ntdll.dll!RtlReportCriticalFailure�()	Unbekannt
 	ntdll.dll!RtlpHeapHandleError�()	Unbekannt
 	ntdll.dll!RtlpHpHeapHandleError�()	Unbekannt
 	ntdll.dll!RtlpLogHeapFailure�()	Unbekannt
 	ntdll.dll!RtlpFreeHeapInternal()	Unbekannt
 	ntdll.dll!RtlFreeHeap()	Unbekannt
>	Armory.exe!_free_base(void * block) Zeile 105	C++
 	Armory.exe!v8::internal::BackingStore::~BackingStore(void)	C++
 	Armory.exe!std::_Ref_count_resource<class v8::internal::BackingStore *,struct std::default_delete<class v8::internal::BackingStore> >::_Destroy(void)	C++
 	Armory.exe!v8::internal::ArrayBufferSweeper::SweepingJob::SweepYoung(void)	C++
 	Armory.exe!v8::internal::ArrayBufferSweeper::SweepingJob::Sweep(void)	C++
 	Armory.exe!v8::internal::ArrayBufferSweeper::~ArrayBufferSweeper(void)	C++
 	Armory.exe!v8::internal::CancelableTask::Run(void)	C++
 	Armory.exe!v8::platform::DefaultWorkerThreadsTaskRunner::WorkerThread::Run(void)	C++
 	Armory.exe!v8::base::OS::StrNCpy(char *,int,char const *,unsigned __int64)	C++
 	Armory.exe!thread_start<unsigned int (__cdecl*)(void *),1>(void * const parameter) Zeile 97	C++
 	kernel32.dll!BaseThreadInitThunk�()	Unbekannt
 	ntdll.dll!RtlUserThreadStart�()	Unbekannt

After noticing this, it was rather easy to find the cause, given the fact that I was able to reproduce the crash every time an Armory scene with the Nishita sky model was used, which calls kha.Image.fromBytes().

Due to #63 I wasn't able to test my fix with the latest Armorcore main branch but with 93b696f, it should still work with newer versions though :)

Edit: also fixed this for krom_create_texture_from_bytes3d(). There could be some more misuses of backing stores, but the other usages look correct to me (though I still understand too little of the Armorcore and Kinc code in order to be certain here).

@luboslenco
Copy link
Member

luboslenco commented Jun 1, 2023

Great find, thanks! Do we need the additional malloc? What happens if we just omit delete[] content->Data();? Can we avoid the malloc for non-readable images?

…implement readable attribute for 3d textures
@MoritzBrueckner
Copy link
Contributor Author

Yeah, we can avoid the malloc for non-readable images, I just realized Kode/Krom does the same. I now pushed another commit to implement this that also adds an image field at position 1 of the value returned by krom_create_texture_from_bytes3d() in order to actually use the readable attribute in this function. This way, Krom.getTexturePixels() and Image.getPixels() work for 3d textures created by this function which would previously fail here.

@luboslenco luboslenco merged commit 8ffff1b into armory3d:main Jul 10, 2023
4 checks passed
@luboslenco
Copy link
Member

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants