From 1a0254bfc59d328dff7f672c4d3ffaf652cce5bd 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 16:35:23 +0200
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#4137

Task: 3777754
Signed-off-by: Lucas Lefèvre (lul) <lul@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 fcab8915b..fe36dfdb5 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";
@@ -42,6 +42,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 e7cd328aa..32a5f1a27 100644
--- a/tests/figures/chart/charts_component.test.ts
+++ b/tests/figures/chart/charts_component.test.ts
@@ -1224,3 +1224,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 37cc29fd0..a015c5915 100644
--- a/tests/test_helpers/helpers.ts
+++ b/tests/test_helpers/helpers.ts
@@ -624,7 +624,7 @@ export const mockChart = () => {
       return mockChartData.data;
     }
     toBase64Image = () => "";
-    destroy = () => {};
+    destroy() {}
     update() {}
     options = mockChartData.options;
     config = mockChartData;