From 22f52f37003d6d27301ff05b31ea69450a72af04 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Theys?= <seb@odoo.com>
Date: Thu, 2 Jun 2022 15:56:05 +0000
Subject: [PATCH] [IMP] mail: avoid storing model manager in scope that are not
 cleaned
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This will reduce memory leak size in tests. But the proper fix should be lower
level in the code (probably in Owl according to the stack) to ensure unnecessary
references aren't kept anymore.

Part of task-2850875

closes odoo/odoo#90934

Signed-off-by: Alexandre Kühn (aku) <aku@odoo.com>
---
 .../static/src/component_hooks/use_models.js  | 18 ++-------
 .../component_hooks/use_rendered_values.js    |  7 ++--
 .../static/src/component_hooks/use_update.js  | 13 ++-----
 .../chat_window_manager_container.js          |  4 ++
 .../chat_window_manager_container.xml         |  2 +-
 .../dialog_manager_container.js               |  4 ++
 .../dialog_manager_container.xml              |  2 +-
 .../discuss_container/discuss_container.js    |  4 ++
 .../discuss_public_view_container.js          |  6 ++-
 .../discuss_public_view_container.xml         |  4 +-
 .../messaging_menu_container.js               |  3 ++
 addons/mail/static/src/model/model_manager.js | 23 ++++++++++-
 addons/mail/static/src/models/record.js       |  5 ++-
 .../static/src/utils/messaging_component.js   |  3 ++
 .../mail/static/tests/helpers/test_utils.js   | 38 +++++++++----------
 15 files changed, 81 insertions(+), 55 deletions(-)

diff --git a/addons/mail/static/src/component_hooks/use_models.js b/addons/mail/static/src/component_hooks/use_models.js
index ca2e48c2845..82f93ff307e 100644
--- a/addons/mail/static/src/component_hooks/use_models.js
+++ b/addons/mail/static/src/component_hooks/use_models.js
@@ -15,31 +15,21 @@ const { onRendered, onWillDestroy, onWillRender, useComponent } = owl;
  */
 export function useModels() {
     const component = useComponent();
-    const { modelManager } = component.env.services.messaging;
-    Object.defineProperty(component, 'messaging', {
-        get: () => modelManager.messaging,
-    });
     const listener = new Listener({
         isLocking: false, // unfortunately __render has side effects such as children components updating their reference to their corresponding model
         name: `useModels() of ${component}`,
         onChange: () => component.render(),
     });
     onWillRender(() => {
-        if (modelManager) {
-            modelManager.startListening(listener);
-        }
+        component.env.services.messaging.modelManager.startListening(listener);
     });
     onRendered(() => {
-        if (modelManager) {
-            modelManager.stopListening(listener);
-        }
+        component.env.services.messaging.modelManager.stopListening(listener);
     });
     onWillDestroy(() => {
-        if (modelManager) {
-            modelManager.removeListener(listener);
-        }
+        component.env.services.messaging.modelManager.removeListener(listener);
     });
-    modelManager.messagingCreatedPromise.then(() => {
+    component.env.services.messaging.modelManager.messagingCreatedPromise.then(() => {
         component.render();
     });
 }
diff --git a/addons/mail/static/src/component_hooks/use_rendered_values.js b/addons/mail/static/src/component_hooks/use_rendered_values.js
index c8840e31f15..e0fe62179e3 100644
--- a/addons/mail/static/src/component_hooks/use_rendered_values.js
+++ b/addons/mail/static/src/component_hooks/use_rendered_values.js
@@ -17,21 +17,20 @@ export function useRenderedValues(selector) {
     const component = useComponent();
     let renderedValues;
     let patchedValues;
-    const { modelManager } = component.env.services.messaging;
     const listener = new Listener({
         name: `useRenderedValues() of ${component}`,
         onChange: () => component.render(),
     });
     onWillRender(() => {
-        modelManager.startListening(listener);
+        component.env.services.messaging.modelManager.startListening(listener);
         renderedValues = selector();
-        modelManager.stopListening(listener);
+        component.env.services.messaging.modelManager.stopListening(listener);
     });
     onMounted(onUpdate);
     onPatched(onUpdate);
     function onUpdate() {
         patchedValues = renderedValues;
     }
-    onWillDestroy(() => modelManager.removeListener(listener));
+    onWillDestroy(() => component.env.services.messaging.modelManager.removeListener(listener));
     return () => patchedValues;
 }
diff --git a/addons/mail/static/src/component_hooks/use_update.js b/addons/mail/static/src/component_hooks/use_update.js
index 5a06ee1b26f..85034868620 100644
--- a/addons/mail/static/src/component_hooks/use_update.js
+++ b/addons/mail/static/src/component_hooks/use_update.js
@@ -12,26 +12,19 @@ const { onMounted, onPatched, onWillDestroy, useComponent } = owl;
  */
 export function useUpdate({ func }) {
     const component = useComponent();
-    const { modelManager } = component.env.services.messaging;
     const listener = new Listener({
         isLocking: false, // unfortunately onUpdate methods often have side effect
         name: `useUpdate() of ${component}`,
         onChange: () => component.render(),
     });
     function onUpdate() {
-        if (modelManager) {
-            modelManager.startListening(listener);
-        }
+        component.env.services.messaging.modelManager.startListening(listener);
         func();
-        if (modelManager) {
-            modelManager.stopListening(listener);
-        }
+        component.env.services.messaging.modelManager.stopListening(listener);
     }
     onMounted(onUpdate);
     onPatched(onUpdate);
     onWillDestroy(() => {
-        if (modelManager) {
-            modelManager.removeListener(listener);
-        }
+        component.env.services.messaging.modelManager.removeListener(listener);
     });
 }
diff --git a/addons/mail/static/src/components/chat_window_manager_container/chat_window_manager_container.js b/addons/mail/static/src/components/chat_window_manager_container/chat_window_manager_container.js
index c3e33d48183..e5f5b64ba43 100644
--- a/addons/mail/static/src/components/chat_window_manager_container/chat_window_manager_container.js
+++ b/addons/mail/static/src/components/chat_window_manager_container/chat_window_manager_container.js
@@ -16,6 +16,10 @@ export class ChatWindowManagerContainer extends Component {
         useModels();
         super.setup();
     }
+
+    get messaging() {
+        return this.env.services.messaging.modelManager.messaging;
+    }
 }
 
 Object.assign(ChatWindowManagerContainer, {
diff --git a/addons/mail/static/src/components/chat_window_manager_container/chat_window_manager_container.xml b/addons/mail/static/src/components/chat_window_manager_container/chat_window_manager_container.xml
index a7ddd7d94d1..390f2ccf5b7 100644
--- a/addons/mail/static/src/components/chat_window_manager_container/chat_window_manager_container.xml
+++ b/addons/mail/static/src/components/chat_window_manager_container/chat_window_manager_container.xml
@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <templates xml:space="preserve">
     <t t-name="mail.ChatWindowManagerContainer" owl="1">
-        <ChatWindowManager t-if="messaging and messaging.chatWindowManager" record="messaging.chatWindowManager"/>     
+        <ChatWindowManager t-if="messaging and messaging.chatWindowManager" record="messaging.chatWindowManager"/>
     </t>
 </templates>
diff --git a/addons/mail/static/src/components/dialog_manager_container/dialog_manager_container.js b/addons/mail/static/src/components/dialog_manager_container/dialog_manager_container.js
index 95c30e326f8..2024aef1cd0 100644
--- a/addons/mail/static/src/components/dialog_manager_container/dialog_manager_container.js
+++ b/addons/mail/static/src/components/dialog_manager_container/dialog_manager_container.js
@@ -16,6 +16,10 @@ export class DialogManagerContainer extends Component {
         useModels();
         super.setup();
     }
+
+    get messaging() {
+        return this.env.services.messaging.modelManager.messaging;
+    }
 }
 
 Object.assign(DialogManagerContainer, {
diff --git a/addons/mail/static/src/components/dialog_manager_container/dialog_manager_container.xml b/addons/mail/static/src/components/dialog_manager_container/dialog_manager_container.xml
index 1f730a30ae0..47e27238e44 100644
--- a/addons/mail/static/src/components/dialog_manager_container/dialog_manager_container.xml
+++ b/addons/mail/static/src/components/dialog_manager_container/dialog_manager_container.xml
@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <templates xml:space="preserve">
     <t t-name="mail.DialogManagerContainer" owl="1">
-        <DialogManager t-if="messaging and messaging.dialogManager" record="messaging.dialogManager"/>     
+        <DialogManager t-if="messaging and messaging.dialogManager" record="messaging.dialogManager"/>
     </t>
 </templates>
diff --git a/addons/mail/static/src/components/discuss_container/discuss_container.js b/addons/mail/static/src/components/discuss_container/discuss_container.js
index f2dd2503384..bd506cbaa15 100644
--- a/addons/mail/static/src/components/discuss_container/discuss_container.js
+++ b/addons/mail/static/src/components/discuss_container/discuss_container.js
@@ -53,6 +53,10 @@ export class DiscussContainer extends Component {
         DiscussContainer.currentInstance = this;
     }
 
+    get messaging() {
+        return this.env.services.messaging.modelManager.messaging;
+    }
+
     _willDestroy() {
         if (this.discuss && DiscussContainer.currentInstance === this) {
             this.discuss.close();
diff --git a/addons/mail/static/src/components/discuss_public_view_container/discuss_public_view_container.js b/addons/mail/static/src/components/discuss_public_view_container/discuss_public_view_container.js
index 3c232df6b3f..d2e4c2e3e7e 100644
--- a/addons/mail/static/src/components/discuss_public_view_container/discuss_public_view_container.js
+++ b/addons/mail/static/src/components/discuss_public_view_container/discuss_public_view_container.js
@@ -18,7 +18,11 @@ export class DiscussPublicViewContainer extends Component {
     }
 
     get discussPublicView() {
-        return this.messaging.models['DiscussPublicView'].findFromIdentifyingData(this.messaging);
+        return this.messaging && this.messaging.models['DiscussPublicView'].findFromIdentifyingData(this.messaging);
+    }
+
+    get messaging() {
+        return this.env.services.messaging.modelManager.messaging;
     }
 
 }
diff --git a/addons/mail/static/src/components/discuss_public_view_container/discuss_public_view_container.xml b/addons/mail/static/src/components/discuss_public_view_container/discuss_public_view_container.xml
index 690bb3bc247..03099cdd2e7 100644
--- a/addons/mail/static/src/components/discuss_public_view_container/discuss_public_view_container.xml
+++ b/addons/mail/static/src/components/discuss_public_view_container/discuss_public_view_container.xml
@@ -2,10 +2,10 @@
 <templates xml:space="preserve">
     <t t-name="mail.DiscussPublicViewContainer" owl="1">
         <div class="o_DiscussPublicViewContainer position-absolute h-100 w-100">
-            <DiscussPublicView t-if="messaging and messaging.isInitialized" record="discussPublicView"/>
+            <DiscussPublicView t-if="discussPublicView" record="discussPublicView"/>
             <div t-else="" class="h-100 d-flex justify-content-center align-items-center">
                 <i class="o_DiscussPublicViewContainer_spinner fa fa-circle-o-notch fa-spin mx-2" title="Please wait..."/>
-            </div>        
+            </div>
         </div>
     </t>
 </templates>
diff --git a/addons/mail/static/src/components/messaging_menu_container/messaging_menu_container.js b/addons/mail/static/src/components/messaging_menu_container/messaging_menu_container.js
index efa9ab2064d..5a45b20a226 100644
--- a/addons/mail/static/src/components/messaging_menu_container/messaging_menu_container.js
+++ b/addons/mail/static/src/components/messaging_menu_container/messaging_menu_container.js
@@ -17,6 +17,9 @@ export class MessagingMenuContainer extends Component {
         super.setup();
     }
 
+    get messaging() {
+        return this.env.services.messaging.modelManager.messaging;
+    }
 }
 
 Object.assign(MessagingMenuContainer, {
diff --git a/addons/mail/static/src/model/model_manager.js b/addons/mail/static/src/model/model_manager.js
index 0613ea38c88..1750a585feb 100644
--- a/addons/mail/static/src/model/model_manager.js
+++ b/addons/mail/static/src/model/model_manager.js
@@ -195,6 +195,24 @@ export class ModelManager {
         this._flushUpdateCycle();
     }
 
+    /**
+     * Destroys this model manager, which consists of cleaning all possible
+     * references in order to avoid memory leaks.
+     */
+    destroy() {
+        this.messaging.delete();
+        for (const model of Object.values(this.models)) {
+            delete model.__fieldList;
+            delete model.__fieldMap;
+            delete model.__identifyingFields;
+            delete model.__identifyingFieldsFlattened;
+            delete model.__records;
+            delete model.__requiredFieldsList;
+            delete model.fields;
+            delete model.modelManager;
+        }
+    }
+
     /**
      * Returns whether the given record still exists.
      *
@@ -1216,13 +1234,14 @@ export class ModelManager {
                 field => field.required
             );
             // Add field accessors.
-            for (const field of model.__fieldList) {
-                Object.defineProperty(model.prototype, field.fieldName, {
+            for (const fieldName of Object.keys(model.__fieldMap)) {
+                Object.defineProperty(model.prototype, fieldName, {
                     get: function getFieldValue() { // this is bound to record
                         if (!this.exists()) {
                             // Deprecated, allows reading fields on deleted records. Use exists() instead.
                             return undefined;
                         }
+                        const field = model.__fieldMap[fieldName];
                         if (this.modelManager._listeners.size) {
                             if (!this.modelManager._listenersObservingLocalId.has(this.localId)) {
                                 this.modelManager._listenersObservingLocalId.set(this.localId, new Map());
diff --git a/addons/mail/static/src/models/record.js b/addons/mail/static/src/models/record.js
index 2c1ad806f1f..4a1dd387806 100644
--- a/addons/mail/static/src/models/record.js
+++ b/addons/mail/static/src/models/record.js
@@ -125,7 +125,7 @@ registerModel({
          * @returns {Messaging}
          */
         messaging() {
-            return this.modelManager.messaging;
+            return this.modelManager && this.modelManager.messaging;
         },
         /**
          * Returns all existing models.
@@ -149,6 +149,9 @@ registerModel({
          * @returns {boolean}
          */
         exists() {
+            if (!this.modelManager) {
+                return false;
+            }
             return this.modelManager.exists(this.constructor, this);
         },
         /**
diff --git a/addons/mail/static/src/utils/messaging_component.js b/addons/mail/static/src/utils/messaging_component.js
index 6d0129d9741..97e5e0f8b9d 100644
--- a/addons/mail/static/src/utils/messaging_component.js
+++ b/addons/mail/static/src/utils/messaging_component.js
@@ -44,6 +44,9 @@ export function registerMessagingComponent(ComponentClass) {
             }
             return res;
         }
+        get messaging() {
+            return this.env.services.messaging.modelManager.messaging;
+        }
         /**
          * @returns {string}
          */
diff --git a/addons/mail/static/tests/helpers/test_utils.js b/addons/mail/static/tests/helpers/test_utils.js
index 6767b490c5c..6f8806526df 100644
--- a/addons/mail/static/tests/helpers/test_utils.js
+++ b/addons/mail/static/tests/helpers/test_utils.js
@@ -406,9 +406,9 @@ function getCreateChatterContainerComponent({ afterEvent, env, target }) {
     };
 }
 
-function getCreateComposerComponent({ env, modelManager, target }) {
+function getCreateComposerComponent({ env, target }) {
     return async function createComposerComponent(composer, props) {
-        const composerView = modelManager.messaging.models['ComposerView'].create({
+        const composerView = env.services.messaging.modelManager.messaging.models['ComposerView'].create({
             qunitTest: insertAndReplace({
                 composer: replace(composer),
             }),
@@ -420,9 +420,9 @@ function getCreateComposerComponent({ env, modelManager, target }) {
     };
 }
 
-function getCreateComposerSuggestionComponent({ env, modelManager, target }) {
+function getCreateComposerSuggestionComponent({ env, target }) {
     return async function createComposerSuggestionComponent(composer, props) {
-        const composerView = modelManager.messaging.models['ComposerView'].create({
+        const composerView = env.services.messaging.modelManager.messaging.models['ComposerView'].create({
             qunitTest: insertAndReplace({
                 composer: replace(composer),
             }),
@@ -434,9 +434,9 @@ function getCreateComposerSuggestionComponent({ env, modelManager, target }) {
     };
 }
 
-function getCreateMessageComponent({ env, modelManager, target }) {
+function getCreateMessageComponent({ env, target }) {
     return async function createMessageComponent(message) {
-        const messageView = modelManager.messaging.models['MessageView'].create({
+        const messageView = env.services.messaging.modelManager.messaging.models['MessageView'].create({
             message: replace(message),
             qunitTest: insertAndReplace(),
         });
@@ -453,9 +453,9 @@ function getCreateMessagingMenuComponent({ env, target }) {
     };
 }
 
-function getCreateNotificationListComponent({ env, modelManager, target }) {
+function getCreateNotificationListComponent({ env, target }) {
     return async function createNotificationListComponent({ filter = 'all' } = {}) {
-        const notificationListView = modelManager.messaging.models['NotificationListView'].create({
+        const notificationListView = env.services.messaging.modelManager.messaging.models['NotificationListView'].create({
             filter,
             qunitTestOwner: insertAndReplace(),
         });
@@ -595,10 +595,10 @@ async function start(param0 = {}) {
     param0.serverData.views = { ...pyEnv.getViews(), ...param0.serverData.views };
     const webClient = await getWebClientReady({ ...param0, messagingBus, testSetupDoneDeferred });
 
-    const { modelManager } = webClient.env.services.messaging;
+    webClient.env.services.messaging.modelManager;
     registerCleanup(async () => {
-        await modelManager.messagingInitializedPromise;
-        modelManager.messaging.delete();
+        await webClient.env.services.messaging.modelManager.messagingInitializedPromise;
+        webClient.env.services.messaging.modelManager.destroy();
         webClient.env.services.legacy_bus_service.destroy();
         delete webClient.env.services.messaging;
         delete owl.Component.env.services.messaging;
@@ -606,11 +606,11 @@ async function start(param0 = {}) {
         delete owl.Component.env;
     });
     if (waitUntilMessagingCondition === 'created') {
-        await modelManager.messagingCreatedPromise;
+        await webClient.env.services.messaging.modelManager.messagingCreatedPromise;
     }
     if (waitUntilMessagingCondition === 'initialized') {
-        await modelManager.messagingCreatedPromise;
-        await modelManager.messagingInitializedPromise;
+        await webClient.env.services.messaging.modelManager.messagingCreatedPromise;
+        await webClient.env.services.messaging.modelManager.messagingInitializedPromise;
     }
     const openDiscuss = getOpenDiscuss(webClient, discuss);
     if (autoOpenDiscuss) {
@@ -629,16 +629,16 @@ async function start(param0 = {}) {
         afterNextRender,
         click: getClick({ afterNextRender }),
         createChatterContainerComponent: getCreateChatterContainerComponent({ afterEvent, env: webClient.env, target }),
-        createComposerComponent: getCreateComposerComponent({ env: webClient.env, modelManager, target }),
-        createComposerSuggestionComponent: getCreateComposerSuggestionComponent({ env: webClient.env, modelManager, target }),
-        createMessageComponent: getCreateMessageComponent({ env: webClient.env, modelManager, target }),
+        createComposerComponent: getCreateComposerComponent({ env: webClient.env, target }),
+        createComposerSuggestionComponent: getCreateComposerSuggestionComponent({ env: webClient.env, target }),
+        createMessageComponent: getCreateMessageComponent({ env: webClient.env, target }),
         createMessagingMenuComponent: getCreateMessagingMenuComponent({ env: webClient.env, target }),
-        createNotificationListComponent: getCreateNotificationListComponent({ env: webClient.env, modelManager, target }),
+        createNotificationListComponent: getCreateNotificationListComponent({ env: webClient.env, target }),
         createRootMessagingComponent: (componentName, props) => createRootMessagingComponent(webClient.env, componentName, { props, target }),
         createThreadViewComponent: getCreateThreadViewComponent({ afterEvent, env: webClient.env, target }),
         env: webClient.env,
         insertText,
-        messaging: modelManager.messaging,
+        messaging: webClient.env.services.messaging.modelManager.messaging,
         openDiscuss,
         openView,
         pyEnv,