Skip to content

Misc memory leak fixes #7224

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/plugins/LADTable/LADTableView.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,5 +79,6 @@ export default class LADTableView {
if (this._destroy) {
this._destroy();
}
this.component = null;
}
}
1 change: 1 addition & 0 deletions src/plugins/condition/ConditionSetViewProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ export default class ConditionSetViewProvider {
if (_destroy) {
_destroy();
}
component = null;
}
};
}
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/displayLayout/plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ class DisplayLayoutView {
destroy() {
if (this._destroy) {
this._destroy();
this.component = undefined;
this.component = null;
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/plugins/plot/MctPlot.vue
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,13 @@
></span>
</div>

<mct-ticks
<MctTicks
v-show="gridLines && !options.compact"
:axis-type="'xAxis'"
:position="'right'"
/>

<mct-ticks
<MctTicks
v-for="(yAxis, index) in yAxesIds"
v-show="gridLines"
:key="`yAxis-gridlines-${index}`"
Expand Down
14 changes: 8 additions & 6 deletions src/plugins/plot/MctTicks.vue
Original file line number Diff line number Diff line change
Expand Up @@ -273,12 +273,14 @@ export default {
)
);

this.tickWidth = tickWidth;
this.$emit('plot-tick-width', {
width: tickWidth,
yAxisId: this.axisType === 'yAxis' ? this.axisId : ''
});
this.shouldCheckWidth = false;
if (this.tickWidth !== tickWidth) {
this.tickWidth = tickWidth;
this.$emit('plot-tick-width', {
width: tickWidth,
yAxisId: this.axisType === 'yAxis' ? this.axisId : ''
});
this.shouldCheckWidth = false;
}
}
}

Expand Down
6 changes: 1 addition & 5 deletions src/plugins/plot/axis/XAxis.vue
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

<template>
<div v-if="loaded" class="gl-plot-axis-area gl-plot-x has-local-controls">
<mct-ticks :axis-type="'xAxis'" :position="'left'" @plot-tick-width="onTickWidthChange" />
<MctTicks :axis-type="'xAxis'" :position="'left'" />

<div class="gl-plot-label gl-plot-x-label" :class="{ 'icon-gear': isEnabledXKeyToggle() }">
{{ xAxisLabel }}
Expand Down Expand Up @@ -59,7 +59,6 @@ export default {
}
}
},
emits: ['plot-x-tick-width'],
data() {
return {
selectedXKeyOptionKey: '',
Expand Down Expand Up @@ -137,9 +136,6 @@ export default {
this.xAxisLabel = this.xAxis.get('label');
this.selectedXKeyOptionKey =
this.xKeyOptions.length > 0 ? this.getXKeyOption(xAxisKey).key : xAxisKey;
},
onTickWidthChange(width) {
this.$emit('plot-x-tick-width', width);
}
}
};
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/plot/axis/YAxis.vue
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
</option>
</select>

<mct-ticks
<MctTicks
:axis-id="id"
:axis-type="'yAxis'"
class="gl-plot-ticks"
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/plot/inspector/PlotOptionsBrowse.vue
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
<div v-if="loaded" class="js-plot-options-browse">
<ul v-if="!isStackedPlotObject" class="c-tree" aria-label="Plot Series Properties">
<h2 class="--first" title="Plot series display properties in this object">Plot Series</h2>
<plot-options-item v-for="series in plotSeries" :key="series.key" :series="series" />
<PlotOptionsItem v-for="series in plotSeries" :key="series.keyString" :series="series" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oof, nice catch

</ul>
<div v-if="plotSeries.length && !isStackedPlotObject" class="grid-properties">
<ul
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/plot/inspector/PlotOptionsEdit.vue
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
<div v-if="loaded" class="js-plot-options-edit">
<ul v-if="!isStackedPlotObject" class="c-tree" aria-label="Plot Series Properties">
<h2 class="--first" title="Display properties for this object">Plot Series</h2>
<li v-for="series in plotSeries" :key="series.key">
<li v-for="series in plotSeries" :key="series.keyString">
<series-form :series="series" @series-updated="updateSeriesConfigForObject" />
</li>
</ul>
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/plot/inspector/PlotOptionsItem.vue
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ export default {
return this.series.get('color').asHexString();
}
},
mounted() {
created() {
this.status = this.openmct.status.get(this.series.domainObject.identifier);
this.removeStatusListener = this.openmct.status.observe(
this.series.domainObject.identifier,
Expand Down
4 changes: 2 additions & 2 deletions src/plugins/plot/inspector/forms/SeriesForm.vue
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ export default {
return this.series.get('color').asHexString();
}
},
mounted() {
created() {
this.initialize();

this.status = this.openmct.status.get(this.series.domainObject.identifier);
Expand All @@ -206,7 +206,7 @@ export default {
}
},
methods: {
initialize: function () {
initialize() {
this.fields = [
{
modelProp: 'yKey',
Expand Down
4 changes: 4 additions & 0 deletions src/plugins/plot/stackedPlot/mixins/objectStyles-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ export default {
this.stopListeningStyles();
}

if (this.stopListeningFontStyles) {
this.stopListeningFontStyles();
}

if (this.styleRuleManager) {
this.styleRuleManager.destroy();
}
Expand Down
1 change: 1 addition & 0 deletions src/plugins/tabs/tabs.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export default class Tabs {
if (this.destroy) {
this.destroy();
}
component = null;
}
};
}
Expand Down
1 change: 1 addition & 0 deletions src/plugins/telemetryTable/TelemetryTableView.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export default class TelemetryTableView {
if (this._destroy) {
this._destroy();
}
this.component = null;
}

show(element, editMode, { renderWhenVisible }) {
Expand Down
5 changes: 4 additions & 1 deletion src/plugins/viewLargeAction/viewLargeAction.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,10 @@ export default class ViewLargeAction {
}
);
this.preview = vNode.componentInstance;
this.destroy = destroy;
this.destroy = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really applicable to this PR, but it looks like PreviewContainer throws a Vue warning when reusing an existing component (e.g., using viewLargeAction in a DisplayLayout):

[Vue warn]: Maximum recursive updates exceeded. This means you have a reactive effect that is mutating its own dependencies and thus recursively triggering itself. Possible sources include component template, render function, updated hook or watcher source function.

... repeating a zillion times ...
flushJobs @ runtime-core.esm-bundler.js:372
flushJobs @ runtime-core.esm-bundler.js:372
flushJobs @ runtime-core.esm-bundler.js:372
flushJobs @ runtime-core.esm-bundler.js:372
flushJobs @ runtime-core.esm-bundler.js:372
flushJobs @ runtime-core.esm-bundler.js:372
flushJobs @ runtime-core.esm-bundler.js:372
flushJobs @ runtime-core.esm-bundler.js:372
flushJobs @ runtime-core.esm-bundler.js:372
flushJobs @ runtime-core.esm-bundler.js:372
flushJobs @ runtime-core.esm-bundler.js:372
flushJobs @ runtime-core.esm-bundler.js:372
flushJobs @ runtime-core.esm-bundler.js:372
flushJobs @ runtime-core.esm-bundler.js:372
flushJobs @ runtime-core.esm-bundler.js:372
Promise.then (async)
queueFlush @ runtime-core.esm-bundler.js:275
queuePostFlushCb @ runtime-core.esm-bundler.js:295
queueEffectWithSuspense @ runtime-core.esm-bundler.js:1665
componentUpdateFn @ runtime-core.esm-bundler.js:5770
run @ reactivity.esm-bundler.js:178
instance.update @ runtime-core.esm-bundler.js:5861
setupRenderEffect @ runtime-core.esm-bundler.js:5869
mountComponent @ runtime-core.esm-bundler.js:5659
processComponent @ runtime-core.esm-bundler.js:5612
patch @ runtime-core.esm-bundler.js:5087
mountChildren @ runtime-core.esm-bundler.js:5331
mountElement @ runtime-core.esm-bundler.js:5238
processElement @ runtime-core.esm-bundler.js:5203
patch @ runtime-core.esm-bundler.js:5075
mountChildren @ runtime-core.esm-bundler.js:5331
mountElement @ runtime-core.esm-bundler.js:5238
processElement @ runtime-core.esm-bundler.js:5203
patch @ runtime-core.esm-bundler.js:5075
componentUpdateFn @ runtime-core.esm-bundler.js:5755
run @ reactivity.esm-bundler.js:178
instance.update @ runtime-core.esm-bundler.js:5861
setupRenderEffect @ runtime-core.esm-bundler.js:5869
mountComponent @ runtime-core.esm-bundler.js:5659
processComponent @ runtime-core.esm-bundler.js:5612
patch @ runtime-core.esm-bundler.js:5087
mountChildren @ runtime-core.esm-bundler.js:5331
mountElement @ runtime-core.esm-bundler.js:5238
processElement @ runtime-core.esm-bundler.js:5203
patch @ runtime-core.esm-bundler.js:5075
componentUpdateFn @ runtime-core.esm-bundler.js:5755
run @ reactivity.esm-bundler.js:178
instance.update @ runtime-core.esm-bundler.js:5861
setupRenderEffect @ runtime-core.esm-bundler.js:5869
mountComponent @ runtime-core.esm-bundler.js:5659
processComponent @ runtime-core.esm-bundler.js:5612
patch @ runtime-core.esm-bundler.js:5087
componentUpdateFn @ runtime-core.esm-bundler.js:5755
run @ reactivity.esm-bundler.js:178
instance.update @ runtime-core.esm-bundler.js:5861
setupRenderEffect @ runtime-core.esm-bundler.js:5869
mountComponent @ runtime-core.esm-bundler.js:5659
processComponent @ runtime-core.esm-bundler.js:5612
patch @ runtime-core.esm-bundler.js:5087
render @ runtime-core.esm-bundler.js:6379
mount @ runtime-core.esm-bundler.js:3832
app.mount @ runtime-dom.esm-bundler.js:1449
mount @ mount.js:10
_getPreview @ viewLargeAction.js:81
_expand @ viewLargeAction.js:65
invoke @ viewLargeAction.js:51
action.onItemClicked @ MenuAPI.js:81
callWithErrorHandling @ runtime-core.esm-bundler.js:158
callWithAsyncErrorHandling @ runtime-core.esm-bundler.js:166
invoker @ runtime-dom.esm-bundler.js:595
eventHelpers.js:38 [Violation] Added non-passive event listener to a scroll-blocking 'wheel' event. Consider marking event handler as 'passive' to make the page more responsive. See https://www.chromestatus.com/feature/5745543795965952
listenTo @ eventHelpers.js:38
initCanvas @ MctPlot.vue:885
callWithErrorHandling @ runtime-core.esm-bundler.js:158
callWithAsyncErrorHandling @ runtime-core.esm-bundler.js:166
emit @ runtime-core.esm-bundler.js:669
visibilityChanged @ MctChart.vue:300

Seems to happen with any type of component in a DisplayLayout, so probably an issue with PreviewContainer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

destroy();
this.preview = null;
};

return this.preview.$el;
}
Expand Down