delete crash

[fluxionary]

using the most recent version (fab6f1a)

2023-06-04 19:03:45: ERROR[Main]: ServerError: AsyncErr: Lua: Runtime error from mod 'mail' in callback on_playerReceiveFields(): .../mt/5.7.0/Minetest_test/bin/../mods/mail_mod/storage.lua:119: attempt to index a nil value
2023-06-04 19:03:45: ERROR[Main]: stack traceback:
2023-06-04 19:03:45: ERROR[Main]: 	.../mt/5.7.0/Minetest_test/bin/../mods/mail_mod/storage.lua:119: in function 'delete_mail'
2023-06-04 19:03:45: ERROR[Main]: 	...t/5.7.0/Minetest_test/bin/../mods/mail_mod/ui/events.lua:190: in function 'func'
2023-06-04 19:03:45: ERROR[Main]: 	...inetest_test/bin/../builtin/profiler/instrumentation.lua:108: in function <...inetest_test/bin/../builtin/profiler/instrumentation.lua:101>
2023-06-04 19:03:45: ERROR[Main]: 	.../mt/5.7.0/Minetest_test/bin/../builtin/game/register.lua:446: in function <.../mt/5.7.0/Minetest_test/bin/../builtin/game/register.lua:432>

the user reports:

• What I was trying to do is to delete all mails. "Select all" confuses me (do selected mails have darker or lighter color?), so I just selected them all with shift+arrows, then pressed "delete" and nothing happened. After that I pressed "Select all" again and pressed "delete" again and it crashed.

[BuckarooBanzay]

Hm, this looks like the player doesn't have mails in the inbox to iterate through:

mail/storage.lua

Line 119 in fab6f1a

if entry.inbox[i].id == deleted_msg then

😕
(or maybe they are deleted already...)

EDIT: you might have to add a break here:

	for i = #entry.inbox, 1, -1 do
		for _, deleted_msg in ipairs(msg_ids) do
			if entry.inbox[i].id == deleted_msg then
				table.remove(entry.inbox, i)
				break -- <<<<
			end
		end
	end

otherwise it might remove multiple messages in a single pass of the outer loop (#entry.inbox, 1, -1) and the i index points to outside of the inbox-list

See #98 for a partial fix

[Athozus]

@BuckarooBanzay you're removing the multiple deletion system by adding a break. I just added a verification of entry lengths, and it is fixed.

[BuckarooBanzay]

you're removing the multiple deletion system by adding a break

yeah, a break from the inner loop, the outer still loops and removes all selected messages...

[S-S-X]

For me it seems like this again should have been pull request instead direct commits. Just noticed it is, I just didn't see it for some reason...

Anyway, what @Athozus added does not actually do anything. For loop mechanisms already work as same expressions: Loop is never executed if table is empty.

On the other hand @BuckarooBanzay's commit does actually fix issue which basically is preventing from continuing operation on thing that is already removed in inner loop entry.outbox[i].

[Athozus]

yeah, a break from the inner loop, the outer still loops and removes all selected messages...

Sorry, my bad, I was too fast.

@S-S-X I do not think it does anything, when you have a 0 length it does a weird thing.

[S-S-X]

@S-S-X I do not think it does anything, when you have a 0 length it does a weird thing.

Just try it yourself, it is basic Lua and nothing else. Loop block is either executed or not and if table is empty it is not executed, if #t > 0 then is completely unnecessary because for i=#t,1,-1 do already does exactly same.

If you do not believe your eyes then just try it. Reason for crash is in inner loop which attempts to index past outer loop table after first removal.

(best thing to do would be to implement filter-by-shift method discussed on Discord but above stands if going to patch current methods)

[BuckarooBanzay]

should be fixed with #99 otherwise reopen please