Skip to content
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

Node events memory leak fixes #16169

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
10 changes: 7 additions & 3 deletions src/controls/DynDiv.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import { extend } from '../util/ExtensionProvider';
import * as React from 'react';
import { IExtensibleProps } from '../types/IExtensionProvider';

import ErrorBoundary from '../controls/ErrorBoundary';

interface IDynDivDefinition {
component: React.ComponentClass<any>;
options: IDynDivOptions;
Expand Down Expand Up @@ -41,9 +43,11 @@ class DynDiv extends React.Component<IProps, {}> {
}

return (
<div id={group} className={classes.join(' ')}>
{visible.map((comp, idx) => this.renderComponent(comp, idx))}
</div>
<ErrorBoundary key={`dynamic-div-${group}`}>
<div id={group} className={classes.join(' ')}>
{visible.map((comp, idx) => this.renderComponent(comp, idx))}
</div>
</ErrorBoundary>
);
}

Expand Down
14 changes: 11 additions & 3 deletions src/controls/ErrorBoundary.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export interface IErrorBoundaryProps extends WithTranslation {
}

interface IErrorBoundaryState {
hasError: boolean;
error: Error;
errorInfo?: React.ErrorInfo;
}
Expand All @@ -42,6 +43,7 @@ class ErrorBoundary extends ComponentEx<IErrorBoundaryProps, IErrorBoundaryState
this.state = {
error: undefined,
errorInfo: undefined,
hasError: false,
};

this.mErrContext = {
Expand All @@ -57,7 +59,13 @@ class ErrorBoundary extends ComponentEx<IErrorBoundaryProps, IErrorBoundaryState
};
}

static getDerivedStateFromError(error) {
// Called before the render
return { hasError: true, error };
}

public componentDidCatch(error: Error, errorInfo: React.ErrorInfo) {
// Called after the render
if (this.props.canDisplayError === false) {
this.context.api.sendNotification({
type: 'error',
Expand Down Expand Up @@ -88,9 +96,9 @@ class ErrorBoundary extends ComponentEx<IErrorBoundaryProps, IErrorBoundaryState

public render(): React.ReactNode {
const { t, className, canDisplayError, onHide, visible } = this.props;
const { error } = this.state;
const { hasError } = this.state;

if (error === undefined) {
if (!hasError) {
return (
<ErrorContext.Provider value={this.mErrContext}>
{React.Children.only(this.props.children)}
Expand Down Expand Up @@ -153,7 +161,7 @@ class ErrorBoundary extends ComponentEx<IErrorBoundaryProps, IErrorBoundaryState
}

private retryRender = () => {
this.setState({ error: undefined, errorInfo: undefined });
this.setState({ hasError: false, error: undefined, errorInfo: undefined });
}
}

Expand Down
3 changes: 1 addition & 2 deletions src/extensions/mod_management/util/deploy.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { startActivity, stopActivity } from '../../../actions/session';
import { IDeployedFile, IDeploymentMethod, IExtensionApi } from '../../../types/IExtensionContext';
import { IGame } from '../../../types/IGame';
import { INotification } from '../../../types/INotification';
import { IProfile } from '../../../types/IState';
import { ProcessCanceled, TemporaryError } from '../../../util/CustomErrors';
import { log } from '../../../util/log';
Expand All @@ -10,7 +9,7 @@ import { getSafe } from '../../../util/storeHelper';
import { truthy } from '../../../util/util';
import { IModType } from '../../gamemode_management/types/IModType';
import { getGame } from '../../gamemode_management/util/getGame';
import { installPath, installPathForGame } from '../selectors';
import { installPathForGame } from '../selectors';
import { IMod } from '../types/IMod';
import { fallbackPurgeType, getManifest, loadActivation, saveActivation, withActivationLock } from './activationStore';
import { getActivator, getCurrentActivator } from './deploymentMethods';
Expand Down
20 changes: 10 additions & 10 deletions src/extensions/mod_management/views/ExternalChangeDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -231,17 +231,17 @@ class ExternalChangeDialog extends ComponentEx<IProps, IComponentState> {
return (
<div style={{ flex: '1 1 0', display: 'flex', flexDirection: 'column' }}>
{text}
<p>{actions.map(action => (
<>
<a
key={action.key}
onClick={this.setAll[type]}
href={'#' + action.key}
>{t(action.allText)}
</a><span className='link-action-seperator'>&nbsp; | &nbsp;</span>
</>
<div>{actions.map(action => (
<div key={action.key}>
<a
key={action.key}
onClick={this.setAll[type]}
href={'#' + action.key}
>{t(action.allText)}
</a><span className='link-action-seperator'>&nbsp; | &nbsp;</span>
</div>
))
}</p>
}</div>
<div style={{ overflowY: 'auto', flex: '1 1 0' }}>
<Table
tableId={`external-change-${type}`}
Expand Down
8 changes: 7 additions & 1 deletion src/extensions/mod_management/views/ModList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1076,7 +1076,13 @@ class ModList extends ComponentEx<IProps, IComponentState> {
}

private updateModGrouping(modsWithState) {
const modList = Object.keys(modsWithState).map(key => modsWithState[key]);
const modList = Object.keys(modsWithState).reduce((accum, key) => {
const mod = modsWithState[key];
if (mod) {
accum.push(mod);
}
return accum;
}, []);
const grouped = groupMods(modList, { groupBy: 'file', multipleEnabled: false });

const groupedMods = grouped.reduce((prev: { [id: string]: IModWithState[] }, value) => {
Expand Down
49 changes: 30 additions & 19 deletions src/extensions/symlink_activator_elevate/index.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
/* eslint-disable */
import { clearUIBlocker, setUIBlocker } from '../../actions';
import {IExtensionApi, IExtensionContext} from '../../types/IExtensionContext';
import { IGame } from '../../types/IGame';
import { IState } from '../../types/IState';
import {ProcessCanceled, TemporaryError, UserCanceled} from '../../util/CustomErrors';
import {ProcessCanceled, UserCanceled} from '../../util/CustomErrors';
import * as fs from '../../util/fs';
import { Normalize } from '../../util/getNormalizeFunc';
import getVortexPath from '../../util/getVortexPath';
Expand Down Expand Up @@ -101,7 +102,7 @@ class DeploymentMethod extends LinkingDeployment {
private mOpenRequests: { [num: number]: { resolve: () => void, reject: (err: Error) => void } };
private mIPCServer: net.Server;
private mDone: () => void;
private mWaitForUser: () => Promise<void>;
// private mWaitForUser: () => Promise<void>;
private mOnReport: (report: string) => void;
private mTmpFilePath: string;

Expand All @@ -114,18 +115,18 @@ class DeploymentMethod extends LinkingDeployment {
api);
this.mElevatedClient = null;

this.mWaitForUser = () => new Promise<void>((resolve, reject) => api.sendNotification({
type: 'info',
message: 'Deployment requires elevation',
noDismiss: true,
actions: [{
title: 'Elevate',
action: dismiss => { dismiss(); resolve(); },
}, {
title: 'Cancel',
action: dismiss => { dismiss(); reject(new UserCanceled()); },
}],
}));
// this.mWaitForUser = () => new Promise<void>((resolve, reject) => api.sendNotification({
// type: 'info',
// message: 'Deployment requires elevation',
// noDismiss: true,
// actions: [{
// title: 'Elevate',
// action: dismiss => { dismiss(); resolve(); },
// }, {
// title: 'Cancel',
// action: dismiss => { dismiss(); reject(new UserCanceled()); },
// }],
// }));

let lastReport: string;
this.mOnReport = (report: string) => {
Expand Down Expand Up @@ -173,11 +174,21 @@ class DeploymentMethod extends LinkingDeployment {
}

public userGate(): Promise<void> {
const state: IState = this.api.store.getState();

return state.settings.workarounds.userSymlinks
? Promise.resolve()
: this.mWaitForUser();
// In the past, we used to block the user from deploying/purging his mods
// until he would give us consent to elevate permissions to do so.
// That is a redundant anti-pattern as the elevation UI itself will already inform the user
// of this requirement and give him the opportunity to cancel or proceed with the deployment!
//
// Additionally - blocking the deployment behind a collapsible notification is extremely bad UX
// as it is not necessarily obvious to the user that we require him to click on the notification.
// Finally, this will block the user from switching to other games while Vortex awaits for elevation
// causing the "Are you stuck?" overlay to appear and remain there, waiting for the user to click on an
// invisible notification button.
//
// I could add a Promise.race([this.waitForUser(), this.waitForElevation()]) to replace the mWaitForUser
// functor - but what's the point - if the user clicked deploy, he surely wants to elevate his instance
// as well. (And if not, he can always cancel the Windows API dialog!)
return Promise.resolve();
}

public prepare(dataPath: string, clean: boolean, lastActivation: IDeployedFile[],
Expand Down
33 changes: 18 additions & 15 deletions src/views/MainWindow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -150,20 +150,6 @@ export class MainWindow extends React.Component<IProps, IMainWindowState> {
};

this.applicationButtons = [];

this.props.api.events.on('show-main-page', pageId => {
this.setMainPage(pageId, false);
});

this.props.api.events.on('refresh-main-page', () => {
this.forceUpdate();
});

this.props.api.events.on('show-modal', id => {
this.updateState({
showLayer: { $set: id },
});
});
}

public getChildContext(): IComponentContext {
Expand All @@ -183,6 +169,20 @@ export class MainWindow extends React.Component<IProps, IMainWindowState> {

this.updateSize();

this.props.api.events.on('show-main-page', pageId => {
this.setMainPage(pageId, false);
});

this.props.api.events.on('refresh-main-page', () => {
this.forceUpdate();
});

this.props.api.events.on('show-modal', id => {
this.updateState({
showLayer: { $set: id },
});
});

window.addEventListener('resize', this.updateSize);
window.addEventListener('keydown', this.updateModifiers);
window.addEventListener('keyup', this.updateModifiers);
Expand All @@ -191,6 +191,9 @@ export class MainWindow extends React.Component<IProps, IMainWindowState> {
}

public componentWillUnmount() {
this.props.api.events.removeAllListeners('show-main-page');
this.props.api.events.removeAllListeners('refresh-main-page');
this.props.api.events.removeAllListeners('show-modal');
window.removeEventListener('resize', this.updateSize);
window.removeEventListener('keydown', this.updateModifiers);
window.removeEventListener('keyup', this.updateModifiers);
Expand Down Expand Up @@ -436,7 +439,7 @@ export class MainWindow extends React.Component<IProps, IMainWindowState> {
const state = this.props.api.getState();
const profile = profileById(state, this.props.activeProfileId);
const game = profile !== undefined ? getGame(profile.gameId) : undefined;
const gameName = game?.name || 'Mods';
const gameName = game?.shortName || game?.name || 'Mods';
const pageGroups = [
{ title: undefined, key: 'dashboard' },
{ title: 'General', key: 'global' },
Expand Down
7 changes: 6 additions & 1 deletion src/views/QuickLauncher.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,22 +60,27 @@ interface IComponentState {
}

class QuickLauncher extends ComponentEx<IProps, IComponentState> {
private mMounted = false;
private mCacheDebouncer: Debouncer = new Debouncer(() => {
if (!this.mMounted) { return Promise.resolve(); }
this.nextState.gameIconCache = this.genGameIconCache();
return Promise.resolve();
}, 100);
}, 250);

constructor(props: IProps) {
super(props);
this.initState({ starter: this.makeStarter(props), gameIconCache: this.genGameIconCache() });
}

public componentDidMount() {
this.mMounted = true;
this.context.api.events.on('quick-launch', this.start);
}

public componentWillUnmount() {
this.mMounted = false;
this.context.api.events.removeListener('quick-launch', this.start);
this.mCacheDebouncer.clear();
}

public UNSAFE_componentWillReceiveProps(nextProps: IProps) {
Expand Down