[PATCH] Lobby message timestamps improperly stored in GUI code

[elexis]

When playing a long game and returning to the lobby, lobby.js polls all chatmessages received during that time.

These chatmessages don't contain a timestamp.

The spamfilter is applied to all of them and then naturally assumes that everyone spammed who sent more than 3 messages.

So don't apply the spamfilter when returning from the lobby and/or preferably fix the timestamps in XmppClient.cpp.

[Josh]

Hmmm, that's an odd bug. I'm surprised that we lose the timestamps somewhere along the way (as they're maintained server-side).

I'm on break right now, so I'll take a quick look....

[Josh]

This turned out to be a lot more complicated than I imagined. The server only sends timestamps for delayed messages, so we have to locally track timestamps for normal messages. It was being done in lobby.js, but lobby.js must be stateless because it is dynamically loaded/unloaded.

The attached patch fixes this by adding timestamps (in seconds since the GMT epoch) to every GUI message.

[elexis]

Thanks for coming up with a patch so quickly! :)

The patch works for me - the timestamps are correct (besides timezone) and the spam monitor doesn't filter messages incorrectly when returning from a game while still filtering current spammers.

Since the patch doesn't introduce any regressions and implements correct message timestamps, I'd be in favor of committing it (the coding style issues should be fixed). Details:

Logic:

• timezone doesn't work for me, shows me UTC timestamps while it should be UTC+1 (See #3350 and linked stuff).
• Why that TODO for addChatMessage, isn't it general enough?
• That other TODO in the spam-monitor: Can't we remove the early return and check for msg.time instead of Date.now()? (Also it looks like 30 seconds, not 5) IMO the spam monitor behavior is weird anyway and could be fixed in a separate ticket

Coding Style:

• time_t -> std::time_t
• time -> std::time
• std::time(NULL) -> std::time(nullptr) ? (as the function wants a pointer)
• L942 missing newline before {
• L114 L117 addChatMessage -> would be nice to have every property in a single line imo (otherwise missing space before })

[Josh]

Thanks for the prompt and detailed review!

Replying to elexis:

• timezone doesn't work for me, shows me UTC timestamps while it should be UTC+1 (See #3350 and linked stuff).

This is the expected behavior. I'm converting it to GMT from localtime, otherwise things become inconsistent. I have updated the comments to be clearer in the updated patch. The idea is that internal representations should be locale-agnostic.

• Why that TODO for addChatMessage, isn't it general enough?

Almost every message from the XMPP backend gets converted to a visible chat message. IMO, there should be some way to simplify the mapping (eg. by directly calling addChatMessage with the message). However, my comment was too vague and has been removed in the updated patch.

• That other TODO in the spam-monitor: Can't we remove the early return and check for msg.time instead of Date.now()? (Also it looks like 30 seconds, not 5) IMO the spam monitor behavior is weird anyway and could be fixed in a separate ticket

msg.time exists on all messages now, so an explicit check is required. The incorrect number of seconds is fixed in the updated patch. Also, the TODO refers to how the spam monitor doesn't properly handle historical messages from a compromised client. The comment has been clarified in my updated patch.

• time_t -> std::time_t
• time -> std::time
• std::time(NULL) -> std::time(nullptr) ? (as the function wants a pointer)

I have been writing a lot of C lately, so this seems rather foreign to me. If it's not too much trouble, could you elaborate on why this is convention?

• L942 missing newline before {
• L114 L117 addChatMessage -> would be nice to have every property in a single line imo (otherwise missing space before })

Fixed point 2 in updated patch, point 1 would be inconsistent with the style used in g_NetMessageTypes for example.

[Josh]

Rework lobby message timestamp handling (v2)

[elexis]

Whitespace:

• Still missing space (or newlines) in L122
• Still missing newline in L943 (XmppClient::ComputeTimestamp, how is that related to 'g_NetMessageTypes` ?)
• ComputeTimestamp could use newlines before the comments imo

Logic:

• if you'd remove that 5 second check, woudln't it just work too for historical messages?

Random ignorable comments:

• Removed TODO: there are only three GUIMessage types which show a chat message without condition, so not sure if its worth it to add a new message queue for chatmessages

• since the code used std::string and std::wstring everywhere, I thought it would be more coherent to also use std::time() and std::size_t, but apparently thats not always the case in the code, so whatever

• ok to have internal timestamps in GMT, but displayed ones should be in the local timezone, so in that case we should fix that GUI function maybe (#3350)

[elexis]

In 17928:

Add timestamps to the GUI/chat-notifications sent from XmppClient, even if it's not a historical message.
Correct spammonitor behavior for historical messages and when returning to the lobby from a game.
Patch by Josh, fixes #3832.

[elexis]

Thanks for looking at the Xmpp backend and writing, testing a patch that quickly!

The spammonitor was already correctly working with your patch, just changed the number 5 to that spamblock-constant, so that rejoining the lobby doesn't unblock spammers. Also fixed the whitespace issues.

The displaying of timestamps in a wrong timezone was addressed in r17929 / #3350.

NOTICE: The removal of L894-905 should have been committed in r17928 instead of r17929.

[elexis]

The patch doesn't compile on windows as strptime isn't resolved. Apparently wposix implements it, but I don't know yet how to acccess it.

[Josh]

Should work if you make sure that source/lib/sysdep/os/win/wposix/wtime.cpp is included.

[Itms]

In 17941:

Revert r17928.
It breaks the Windows build by using time_t and it changes too many things to the way lobby messages are processed to be entirely safe to commit now.
Refs #3832.

As a side effect it reverts r17929, refs #3350.

[Itms]

Attaching a patch including the reverted code and some improvements. It's still needed to make that return call in ComputeTimestamp nicer so we make clear that we return a UTC timestamp.

[elexis]

As far as I can see, the only reason why the post-revert patch isn't committed is #3848. If u64 can be used, we don't need a CTime as proposed in comment:5:ticket:3350.

[elexis]

Backlogging due to lack of progress.

[elexis]

treegb2: ?
treegb2: who is pinging me?
treegb2: some history message is lost when I get in lobby again

​Phab:D512 proposes to remove the broken spamfilter entirely becaue it should be filtered on the ejabberd + moderator level to begin with.

Postponed is not abandoned. ​Phab:D514 proposes to fix this by reverting the revert, as the patch was correct besides missing a #include "lib/sysdep/os/win/wposix/wtime.h" and using std::time_t (see ​Phab:D205).

[elexis]

In 19633:

Delete variously broken lobby spamfilter, which did not filter spammers but filtered non-spammers, conceptually can't filter in a way that all clients see the same chat messages and should instead be implemented on a serverside or moderation level.

Differential Revision: ​https://code.wildfiregames.com/D512
Refs #3832
Accepted By: scythetwirler

[elexis]

In 19801:

Correct lobby chat timestamps.

Only historic messages were timestamped in C++ prior.
Other messages showed the timestamp when they were displayed in JS, which is wrong in case of returning to the lobby from a game.

Differential Revision: ​https://code.wildfiregames.com/D514
Fixes #3832.
Reverts the revert of rP17928 in rP17941.
Patch By: Josh
Tested By: Itms

[elexis]

In 19947:

Fix BSD and OSX build that don't provide the AT&T version of timezone used by rP19801. Refs #3832, D514.

Mostly reviewed by: leper
Tested on Windows by: Stan

[elexis]

In 20032:

Enhance lobby / XMPP timestamp parsing documentation.

Refs #3832, rP19947 / D760, rP19801 / D514
Reviewed By: leper