Skip to content

Protect against ReferenceError when creating SharedWorker #20575

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Protect against ReferenceError when creating SharedWorker #20575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 12 additions & 4 deletions web_src/js/features/eventsource.sharedworker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,18 @@ self.addEventListener('connect', (e) => {
}
}
// Create a new Source
source = new Source(url);
source.register(port);
sourcesByUrl[url] = source;
sourcesByPort[port] = source;
try {
source = new Source(url);
source.register(port);
sourcesByUrl[url] = source;
sourcesByPort[port] = source;
} catch (error) {
port.postMessage({
type: 'error',
message: `unable to create Source: ${error}`,
});
port.close();
}
} else if (event.data.type === 'listen') {
const source = sourcesByPort[port];
source.listen(event.data.eventType);
Expand Down
41 changes: 29 additions & 12 deletions web_src/js/features/notification.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,28 @@ export function initNotificationCount() {
return;
}

const poller = () => {
const pollerFn = (timeout, lastCount) => {
if (timeout <= 0) {
return;
}
setTimeout(() => {
const _promise = updateNotificationCountWithCallback(pollerFn, timeout, lastCount);
}, timeout);
};

pollerFn(notificationSettings.MinTimeout, notificationCount.text());
Comment on lines +53 to +62
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const pollerFn = (timeout, lastCount) => {
if (timeout <= 0) {
return;
}
setTimeout(() => {
const _promise = updateNotificationCountWithCallback(pollerFn, timeout, lastCount);
}, timeout);
};
pollerFn(notificationSettings.MinTimeout, notificationCount.text());
const timeout = notificationSettings.MinTimeout;
const lastCount = notificationCount.text();
if (timeout <= 0) {
return;
}
setTimeout(() => {
const _promise = updateNotificationCountWithCallback(pollerFn, timeout, lastCount);
}, timeout);
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You've got rid of the definition of the pollerfn in this suggestion and then you use it later.

What's the point of this change? Why are you redefining as a constant?

It's deliberately structured the way it's structured.

};

if (notificationSettings.EventSourceUpdateTime > 0 && !!window.EventSource && window.SharedWorker) {
// Try to connect to the event source via the shared worker first
const worker = new SharedWorker(`${__webpack_public_path__}js/eventsource.sharedworker.js`, 'notification-worker');
worker.addEventListener('error', (event) => {
console.error(event);
worker.addEventListener('error', (error) => {
if (error.message && error.message === 'ReferenceError: EventSource is not defined') {
Copy link
Member

Choose a reason for hiding this comment

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

I'm absolutely not a fan of comparing against error messages, and even less of doing some strange behavior in that case.
At least add a comment, if comparing against a specific error is impossible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, it's not necessary to use the error message if the error can be detected by a stable method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the specific error? Where is the specific error defined in the specs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment #20575 (comment)

poller();
return;
}
console.error(error);
});
worker.port.addEventListener('messageerror', () => {
console.error('Unable to deserialize message');
Expand All @@ -70,6 +87,10 @@ export function initNotificationCount() {
if (event.data.type === 'notification-count') {
const _promise = receiveUpdateCount(event.data);
} else if (event.data.type === 'error') {
if (event.data.message === 'unable to create Source: ReferenceError: EventSource is not defined') {
Copy link
Member

Choose a reason for hiding this comment

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

See above.

poller();
return;
}
console.error(event.data);
} else if (event.data.type === 'logout') {
if (event.data.data !== 'here') {
Expand All @@ -87,8 +108,11 @@ export function initNotificationCount() {
worker.port.close();
}
});
worker.port.addEventListener('error', (e) => {
console.error(e);
worker.port.addEventListener('error', (error) => {
if (error.message && error.message === 'unable to create Source: ReferenceError: EventSource is not defined') {
Copy link
Member

Choose a reason for hiding this comment

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

See above.

return;
}
console.error(error);
});
worker.port.start();
window.addEventListener('beforeunload', () => {
Expand All @@ -97,21 +121,14 @@ export function initNotificationCount() {
});
worker.port.close();
});

return;
}

if (notificationSettings.MinTimeout <= 0) {
return;
}

const fn = (timeout, lastCount) => {
setTimeout(() => {
const _promise = updateNotificationCountWithCallback(fn, timeout, lastCount);
}, timeout);
};

fn(notificationSettings.MinTimeout, notificationCount.text());
poller();
}

async function updateNotificationCountWithCallback(callback, timeout, lastCount) {
Expand Down
51 changes: 35 additions & 16 deletions web_src/js/features/stopwatch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,34 @@ export function initStopwatch() {
$(this).parent().trigger('submit');
});

const poller = () => {
const fn = (timeout) => {
if (timeout <= 0) {
return;
}
setTimeout(() => {
const _promise = updateStopwatchWithCallback(fn, timeout);
}, timeout);
};

fn(notificationSettings.MinTimeout);
Comment on lines +30 to +39
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const fn = (timeout) => {
if (timeout <= 0) {
return;
}
setTimeout(() => {
const _promise = updateStopwatchWithCallback(fn, timeout);
}, timeout);
};
fn(notificationSettings.MinTimeout);
const timeout = notificationSettings.MinTimeout;
if (timeout <= 0) {
return;
}
setTimeout(() => {
const _promise = updateStopwatchWithCallback(fn, timeout);
}, timeout);
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this suggestion is wrong. You have dropped the definition of the fn that is being called.


const currSeconds = $('.stopwatch-time').data('seconds');
if (currSeconds) {
updateTimeInterval = updateStopwatchTime(currSeconds);
}
};


if (notificationSettings.EventSourceUpdateTime > 0 && !!window.EventSource && window.SharedWorker) {
// Try to connect to the event source via the shared worker first
const worker = new SharedWorker(`${__webpack_public_path__}js/eventsource.sharedworker.js`, 'notification-worker');
worker.addEventListener('error', (event) => {
console.error(event);
worker.addEventListener('error', (error) => {
if (error.message && error.message === 'ReferenceError: EventSource is not defined') {
Copy link
Member

Choose a reason for hiding this comment

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

See above.

poller();
return;
}
console.error(error);
});
worker.port.addEventListener('messageerror', () => {
console.error('Unable to deserialize message');
Expand All @@ -47,6 +70,10 @@ export function initStopwatch() {
if (event.data.type === 'stopwatches') {
updateStopwatchData(JSON.parse(event.data.data));
} else if (event.data.type === 'error') {
if (event.data.message === 'unable to create Source: ReferenceError: EventSource is not defined') {
Copy link
Member

Choose a reason for hiding this comment

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

See above.

poller();
return;
}
console.error(event.data);
} else if (event.data.type === 'logout') {
if (event.data.data !== 'here') {
Expand All @@ -64,8 +91,11 @@ export function initStopwatch() {
worker.port.close();
}
});
worker.port.addEventListener('error', (e) => {
console.error(e);
worker.port.addEventListener('error', (error) => {
if (error.message && error.message === 'ReferenceError: EventSource is not defined') {
Copy link
Member

Choose a reason for hiding this comment

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

See above.

return;
}
console.error(error);
});
worker.port.start();
window.addEventListener('beforeunload', () => {
Expand All @@ -82,18 +112,7 @@ export function initStopwatch() {
return;
}

const fn = (timeout) => {
setTimeout(() => {
const _promise = updateStopwatchWithCallback(fn, timeout);
}, timeout);
};

fn(notificationSettings.MinTimeout);

const currSeconds = $('.stopwatch-time').data('seconds');
if (currSeconds) {
updateTimeInterval = updateStopwatchTime(currSeconds);
}
poller();
}

async function updateStopwatchWithCallback(callback, timeout) {
Expand Down