narrow: Fix messages being cached without flags set.

f0c680e9c0 introduced a call to
message_helper.process_new_message without first calling
message_store.set_message_flags on the message.

This resulted in it being possible as a race, when loading the Zulip
app to a stream/topic/near narrow, for a message to have the
`historical` flag be undefined due to not being initialized.

That invalid state, in turn, resulted in the message_list_view code
path for rendering the message feed incorrectly displaying additional
recipient bars around the message.

We could fix this by just calling message_store.set_message_booleans
in this code path. However, this bug exposes the fact that it's very
fragile to expect every code path to call that function before
message_helper.process_new_message.

So we instead fix this by moving message_store.set_message_booleans
inside message_helper.process_new_message.

One call point of concern in this change is maybe_add_narrow_messages,
which could theoretically reintroduce the double set_message_flags
bugs detailed in 9729b1a4ad. However, I
believe that to not be possible, because that call should never
experience a cache miss.

The other existing code paths were already calling
set_message_booleans immediately before
message_helper.process_new_message. They are still changing here, in
that we now do a cache lookup before attempting to call
set_message_booleans. Because the message booleans do not affect the
cache lookup and the local message object is discarded in case of a
cache hit, this should have no functional impact.

Because I found the existing comment at that call site confusing and
almost proposed removing it as pointless, extend the block comment to
explicitly mention that the purpose is refreshing our object.

Fixes #21503.

frontend_tests/node_tests/example2.js

@@ -64,14 +64,13 @@ run_test("message_store", () => {
     const in_message = {...messages.isaac_to_denmark_stream};
 
     assert.equal(in_message.alerted, undefined);
-    message_store.set_message_booleans(in_message);
-    assert.equal(in_message.alerted, true);
 
     // Let's add a message into our message_store via
     // message_helper.process_new_message.
     assert.equal(message_store.get(in_message.id), undefined);
     message_helper.process_new_message(in_message);
     const message = message_store.get(in_message.id);
+    assert.equal(in_message.alerted, true);
     assert.equal(message, in_message);
 
     // There are more side effects.

frontend_tests/node_tests/message_events.js

@@ -20,7 +20,6 @@ message_lists.current = {};
 const people = zrequire("people");
 const message_events = zrequire("message_events");
 const message_helper = zrequire("message_helper");
-const message_store = zrequire("message_store");
 const stream_data = zrequire("stream_data");
 const stream_topic_history = zrequire("stream_topic_history");
 const unread = zrequire("unread");
@@ -70,7 +69,6 @@ run_test("update_messages", () => {
     };
 
     message_helper.process_new_message(original_message);
-    message_store.set_message_booleans(original_message);
 
     assert.equal(original_message.mentioned, true);
     assert.equal(original_message.unread, true);

frontend_tests/node_tests/message_fetch.js

@@ -31,7 +31,6 @@ mock_esm("../../static/js/ui_report", {
 const channel = mock_esm("../../static/js/channel");
 const message_helper = mock_esm("../../static/js/message_helper");
 const message_lists = mock_esm("../../static/js/message_lists");
-const message_store = mock_esm("../../static/js/message_store");
 const message_util = mock_esm("../../static/js/message_util");
 const pm_list = mock_esm("../../static/js/pm_list");
 const stream_list = mock_esm("../../static/js/stream_list", {
@@ -119,14 +118,13 @@ function config_fake_channel(conf) {
 function config_process_results(messages) {
     const self = {};
 
-    const messages_processed_for_bools = [];
+    const messages_processed_for_new = [];
 
-    message_store.set_message_booleans = (message) => {
-        messages_processed_for_bools.push(message);
+    message_helper.process_new_message = (message) => {
+        messages_processed_for_new.push(message);
+        return message;
     };
 
-    message_helper.process_new_message = (message) => message;
-
     message_util.do_unread_count_updates = (arg) => {
         assert.deepEqual(arg, messages);
     };
@@ -141,7 +139,7 @@ function config_process_results(messages) {
     pm_list.update_private_messages = noop;
 
     self.verify = () => {
-        assert.deepEqual(messages_processed_for_bools, messages);
+        assert.deepEqual(messages_processed_for_new, messages);
     };
 
     return self;

frontend_tests/node_tests/message_store.js

@@ -110,7 +110,6 @@ test("process_new_message", () => {
         is_me_message: false,
         id: 2067,
     };
-    message_store.set_message_booleans(message);
     message_helper.process_new_message(message);
 
     assert.deepEqual(message_user_ids.user_ids().sort(), [me.user_id, bob.user_id, cindy.user_id]);
@@ -153,7 +152,6 @@ test("process_new_message", () => {
         id: 2068,
     };
 
-    message_store.set_message_booleans(message);
     message_helper.process_new_message(message);
     assert.deepEqual(message.stream, message.display_recipient);
     assert.equal(message.reply_to, "denise@example.com");
@@ -307,7 +305,6 @@ test("update_property", () => {
         id: 101,
     };
     for (const message of [message1, message2]) {
-        message_store.set_message_booleans(message);
         message_helper.process_new_message(message);
     }
 

static/js/echo.js

@@ -193,7 +193,6 @@ export function insert_local_message(message_request, local_id_float) {
 
     message.display_recipient = build_display_recipient(message);
 
-    message_store.set_message_booleans(message);
     insert_message(message);
     return message;
 }

static/js/message_events.js

@@ -64,11 +64,13 @@ function maybe_add_narrowed_messages(messages, msg_list, callback) {
             }
 
             // This second call to process_new_message in the
-            // insert_new_messages code path helps in very rare race
+            // insert_new_messages code path is designed to replace
+            // our slightly stale message object with the latest copy
+            // from the message_store. This helps in very rare race
             // conditions, where e.g. the current user's name was
             // edited in between when they sent the message and when
             // we hear back from the server and can echo the new
-            // message.  Arguably, it's counterproductive complexity.
+            // message.
             new_messages = new_messages.map((message) =>
                 message_helper.process_new_message(message),
             );

static/js/message_fetch.js

@@ -8,7 +8,6 @@ import * as message_helper from "./message_helper";
 import * as message_list from "./message_list";
 import * as message_lists from "./message_lists";
 import * as message_scroll from "./message_scroll";
-import * as message_store from "./message_store";
 import * as message_util from "./message_util";
 import * as narrow_banner from "./narrow_banner";
 import {page_params} from "./page_params";
@@ -50,10 +49,7 @@ function process_result(data, opts) {
         narrow_banner.show_empty_narrow_message();
     }
 
-    messages = messages.map((message) => {
-        message_store.set_message_booleans(message);
-        return message_helper.process_new_message(message);
-    });
+    messages = messages.map((message) => message_helper.process_new_message(message));
 
     // In case any of the newly fetched messages are new, add them to
     // our unread data structures.  It's important that this run even

static/js/message_helper.js

@@ -23,6 +23,7 @@ export function process_new_message(message) {
         return cached_msg;
     }
 
+    message_store.set_message_booleans(message);
     message.sent_by_me = people.is_current_user(message.sender_email);
 
     people.extract_people_from_message(message);

static/js/server_events.js

@@ -6,7 +6,6 @@ import * as channel from "./channel";
 import * as echo from "./echo";
 import * as message_events from "./message_events";
 import * as message_lists from "./message_lists";
-import * as message_store from "./message_store";
 import {page_params} from "./page_params";
 import * as reload from "./reload";
 import * as reload_state from "./reload_state";
@@ -110,10 +109,6 @@ function get_events_success(events) {
         try {
             messages = echo.process_from_server(messages);
             if (messages.length > 0) {
-                for (const message of messages) {
-                    message_store.set_message_booleans(message);
-                }
-
                 const sent_by_this_client = messages.some((msg) =>
                     sent_messages.messages.has(msg.local_id),
                 );