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

CORE: prevent unbound growth of suspendedTimeouts and possible NaN values #12059

Merged
merged 3 commits into from
Jul 30, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
23 changes: 17 additions & 6 deletions src/utils/focusTimeout.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,27 @@
let outOfFocusStart;
let outOfFocusStart = null; // enforce null otherwise it could be undefined and the callback wouldn't execute
let timeOutOfFocus = 0;
let suspendedTimeouts = [];

document.addEventListener('visibilitychange', () => {
function trackTimeOutOfFocus() {
if (document.hidden) {
outOfFocusStart = Date.now()
} else {
timeOutOfFocus += Date.now() - outOfFocusStart
suspendedTimeouts.forEach(({ callback, startTime, setTimerId }) => setTimerId(setFocusTimeout(callback, timeOutOfFocus - startTime)()))
timeOutOfFocus += Date.now() - (outOfFocusStart ?? 0); // when the page is loaded in hidden state outOfFocusStart is undefined, which results in timeoutOffset being NaN
outOfFocusStart = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You had a comment here calling this to be set before looping on suspendedTimeouts - I removed it as I don't think the order matters, it's checked from within a setTimeout callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes you're right, it shouldn't matter.

suspendedTimeouts.forEach(({ callback, startTime, setTimerId }) => setTimerId(setFocusTimeout(callback, timeOutOfFocus - startTime)()));
suspendedTimeouts = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

all the suspended timeouts should be dealt with here, I think the list can just be cleared instead of filtering it during the loop

Copy link
Contributor Author

@olafbuitelaar olafbuitelaar Jul 30, 2024

Choose a reason for hiding this comment

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

i agree, i wasn't sure about it, so took the safer route by just filtering on the callback. but prefer this if this is a safe operation

}
});
}

document.addEventListener('visibilitychange', trackTimeOutOfFocus);

export function reset() {
outOfFocusStart = null;
timeOutOfFocus = 0;
suspendedTimeouts = [];
document.removeEventListener('visibilitychange', trackTimeOutOfFocus);
document.addEventListener('visibilitychange', trackTimeOutOfFocus);
}

/**
* Wraps native setTimeout function in order to count time only when page is focused
Expand All @@ -19,7 +30,7 @@ document.addEventListener('visibilitychange', () => {
* @param {number} [milliseconds] - Minimum duration (in milliseconds) that the callback will be executed after
* @returns {function(*): (number)} - Getter function for current timer id
*/
export default function setFocusTimeout(callback, milliseconds) {
export function setFocusTimeout(callback, milliseconds) {
const startTime = timeOutOfFocus;
let timerId = setTimeout(() => {
if (timeOutOfFocus === startTime && outOfFocusStart == null) {
Expand Down
2 changes: 1 addition & 1 deletion src/utils/ttlCollection.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {GreedyPromise} from './promise.js';
import {binarySearch, logError, timestamp} from '../utils.js';
import setFocusTimeout from './focusTimeout.js';
import {setFocusTimeout} from './focusTimeout.js';

/**
* Create a set-like collection that automatically forgets items after a certain time.
Expand Down
39 changes: 33 additions & 6 deletions test/spec/unit/utils/focusTimeout_spec.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import setFocusTimeout from '../../../../src/utils/focusTimeout';
import {setFocusTimeout, reset} from '../../../../src/utils/focusTimeout';

export const setDocumentHidden = (hidden) => {
Object.defineProperty(document, 'hidden', {
Expand All @@ -9,33 +9,41 @@ export const setDocumentHidden = (hidden) => {
};

describe('focusTimeout', () => {
let clock;
let clock, callback;

beforeEach(() => {
reset()
clock = sinon.useFakeTimers();
callback = sinon.spy();
});

afterEach(() => {
clock.restore();
})

it('should invoke callback when page is visible', () => {
let callback = sinon.stub();
setFocusTimeout(callback, 2000);
clock.tick(2000);
expect(callback.called).to.be.true;
});

it('should not choke if page starts hidden', () => {
setDocumentHidden(true);
reset();
setDocumentHidden(false);
setFocusTimeout(callback, 1000);
clock.tick(1000);
sinon.assert.called(callback);
})

it('should not invoke callback if page was hidden', () => {
let callback = sinon.stub();
setFocusTimeout(callback, 2000);
setDocumentHidden(true);
clock.tick(3000);
expect(callback.called).to.be.false;
});

it('should defer callback execution when page is hidden', () => {
let callback = sinon.stub();
setFocusTimeout(callback, 4000);
clock.tick(2000);
setDocumentHidden(true);
Expand All @@ -46,8 +54,27 @@ describe('focusTimeout', () => {
expect(callback.called).to.be.true;
});

it('should not execute deferred callbacks again', () => {
setDocumentHidden(true);
setFocusTimeout(callback, 1000);
clock.tick(2000);
[false, true, false].forEach(setDocumentHidden);
clock.tick(2000);
sinon.assert.calledOnce(callback);
});

it('should run callbacks that expire while page is hidden', () => {
setFocusTimeout(callback, 1000);
clock.tick(500);
setDocumentHidden(true);
clock.tick(1000);
setDocumentHidden(false);
sinon.assert.notCalled(callback);
clock.tick(1000);
sinon.assert.called(callback);
})

it('should return updated timerId after page was showed again', () => {
let callback = sinon.stub();
const getCurrentTimerId = setFocusTimeout(callback, 4000);
const oldTimerId = getCurrentTimerId();
clock.tick(2000);
Expand Down