From 90e298a00f9dd83d21500224261f3097f35b391c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9mi=20Rahir=20=28rar=29?= <rar@odoo.com>
Date: Thu, 25 Apr 2024 14:35:23 +0000
Subject: [PATCH] [FIX] ChartJs: Properly destroy chartJs object on component
 wrapper deletion
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

How to reproduce:

- Make a chartjs chart (line/bar/pie)
- Mousedown on a datapoint/bar/part of the pie
- move your mouse
- mouseup (lift your finger) while still moving your mouse
-> crash

We were not properly destroying the chart js item and their linked
eventListeners persisted. Specifically, the eventHandler of the tooltip
plugin would still try to handle the mousemove event while its internal
state was partially invalidated.

closes odoo/o-spreadsheet#4168

Task: 3777754
X-original-commit: 670d87dbaf7df0098f7764d8a28490a6174b117a
Signed-off-by: Lucas Lefèvre (lul) <lul@odoo.com>
Signed-off-by: Rémi Rahir (rar) <rar@odoo.com>
---
 src/components/figures/chart/chartJs/chartjs.ts |  3 ++-
 tests/figures/chart/charts_component.test.ts    | 10 ++++++++++
 tests/test_helpers/helpers.ts                   |  2 +-
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/components/figures/chart/chartJs/chartjs.ts b/src/components/figures/chart/chartJs/chartjs.ts
index dbc5bba9b..fe85f3928 100644
--- a/src/components/figures/chart/chartJs/chartjs.ts
+++ b/src/components/figures/chart/chartJs/chartjs.ts
@@ -1,4 +1,4 @@
-import { Component, onMounted, useEffect, useRef } from "@odoo/owl";
+import { Component, onMounted, onWillUnmount, useEffect, useRef } from "@odoo/owl";
 import type { Chart, ChartConfiguration } from "chart.js";
 import { deepCopy, deepEquals } from "../../../../helpers";
 import { Figure, SpreadsheetChildEnv } from "../../../../types";
@@ -45,6 +45,7 @@ export class ChartJsComponent extends Component<Props, SpreadsheetChildEnv> {
       // Note: chartJS modify the runtime in place, so it's important to give it a copy
       this.createChart(deepCopy(runtime.chartJsConfig));
     });
+    onWillUnmount(() => this.chart?.destroy());
     useEffect(() => {
       const runtime = this.chartRuntime;
       if (!deepEquals(runtime, this.currentRuntime, "ignoreFunctions")) {
diff --git a/tests/figures/chart/charts_component.test.ts b/tests/figures/chart/charts_component.test.ts
index 2d10edb1c..bed3d7a43 100644
--- a/tests/figures/chart/charts_component.test.ts
+++ b/tests/figures/chart/charts_component.test.ts
@@ -1306,3 +1306,13 @@ describe("Default background on runtime tests", () => {
     expect(model.getters.getChartRuntime("1").background).toBe("#FA0000");
   });
 });
+
+test("ChartJS charts are correctly destroyed on chart deletion", async () => {
+  ({ parent, fixture, model } = await mountSpreadsheet({ model: new Model() }));
+  createChart(model, { dataSets: ["A1"], type: "bar" }, "1");
+  await nextTick();
+  const spyDelete = jest.spyOn((window as any).Chart.prototype, "destroy");
+  model.dispatch("DELETE_FIGURE", { id: "1", sheetId: model.getters.getActiveSheetId() });
+  await nextTick();
+  expect(spyDelete).toHaveBeenCalled();
+});
diff --git a/tests/test_helpers/helpers.ts b/tests/test_helpers/helpers.ts
index bfd586e49..a52d14009 100644
--- a/tests/test_helpers/helpers.ts
+++ b/tests/test_helpers/helpers.ts
@@ -718,7 +718,7 @@ export const mockChart = () => {
       return mockChartData.data;
     }
     toBase64Image = () => "data:image/png;base64,randomDataThatIsActuallyABase64Image";
-    destroy = () => {};
+    destroy() {}
     update() {}
     options = mockChartData.options;
     config = mockChartData;