Protect against ReferenceError when creating SharedWorker #20575

zeripath · 2022-07-31T19:44:41Z

Some browsers do not have EventSource within the SharedWorker context even though
it is available in the normal context. This leads to a global error percolating
up to the main page.

Here we simply wrap the new Source in a try/catch and catch the error falling back
to a poller if so.

Fix #20572

Signed-off-by: Andrew Thornton [email protected]

Some browsers do not have EventSource within the SharedWorker context even though it is available in the normal context. This leads to a global error percolating up to the main page. Here we simply wrap the new Source in a try/catch and catch the error falling back to a poller if so. Fix go-gitea#20572 Signed-off-by: Andrew Thornton <[email protected]>

delvh · 2022-07-31T19:56:21Z

web_src/js/features/notification.js

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


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);

};

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.

delvh · 2022-07-31T20:00:42Z

web_src/js/features/notification.js

-    worker.addEventListener('error', (event) => {
-      console.error(event);
+    worker.addEventListener('error', (error) => {
+      if (error.message && error.message === 'ReferenceError: EventSource is not defined') {


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.

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

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

Comment #20575 (comment)

delvh · 2022-07-31T20:00:52Z

web_src/js/features/notification.js

@@ -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') {


delvh · 2022-07-31T20:01:03Z

web_src/js/features/notification.js

-    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') {


delvh · 2022-07-31T20:02:32Z

web_src/js/features/stopwatch.js

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


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);

};

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

delvh · 2022-07-31T20:03:03Z

web_src/js/features/stopwatch.js

-    worker.addEventListener('error', (event) => {
-      console.error(event);
+    worker.addEventListener('error', (error) => {
+      if (error.message && error.message === 'ReferenceError: EventSource is not defined') {


delvh · 2022-07-31T20:03:13Z

web_src/js/features/stopwatch.js

@@ -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') {


delvh · 2022-07-31T20:03:23Z

web_src/js/features/stopwatch.js

-    worker.port.addEventListener('error', (e) => {
-      console.error(e);
+    worker.port.addEventListener('error', (error) => {
+      if (error.message && error.message === 'ReferenceError: EventSource is not defined') {


wxiaoguang · 2022-08-01T04:50:57Z

IMO the root problem here is that the SharedWorker doesn't have EventSource support.

Since the EventSource is only used by Source class, which is created by self.addEventListener('connect',

So, maybe there could be a one-line fix for it (then no need to poll in real-time since the browser is non-standard? and the real-time polling never worked before for such non-standard browsers?).

self.addEventListener('connect', (e) => {
  // return early if there is no EventSource in SharedWorker
  if (!self.EventSource) return;
  ...
}

Or:

  port.addEventListener('message', (event) => {
    ...
    ...
    if (!self.EventSource) {
      // Tell the caller to use poller clearly, but it's still more complex than before
      // I still prefer the one-line fix above, because the real-time polling never worked before 😆 
      port.postMessage({type: 'use-poller'});
    } else {
       source = new Source(url);
       ...
    }
  }

zeripath · 2022-08-01T05:35:48Z

@wxiaoguang I think you have misunderstood what the purpose of the pollerfn is.

Here the error is such that the source will not work so the pollerfn is called to replace update notifications instead. There is no polling to check if the source is working.

wxiaoguang · 2022-08-01T05:38:17Z

I understand it correctly. So the second suggestion is to make a port.postMessage({type: 'use-poller'}); call to tell the caller to use the poller fn (instead of the confusing error message)

zeripath · 2022-08-01T05:39:01Z

(!self.EventSource) won't work.

You need to be in the shared worker context to check if the eventsource is available.

wxiaoguang · 2022-08-01T05:39:47Z

It will work, it's already in the context.

The context self is SharedWorkerGlobalScope class.

I have tested

zeripath · 2022-08-01T05:42:10Z

Fine do it then.

wxiaoguang · 2022-08-01T05:43:05Z

I thought that's your honor to do so ........

zeripath added type/bug backport/v1.17 labels Jul 31, 2022

zeripath added this to the 1.18.0 milestone Jul 31, 2022

delvh reviewed Jul 31, 2022

View reviewed changes

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 31, 2022

zeripath closed this Aug 1, 2022

zeripath deleted the fix-20572-protect-against-error-sharedworker branch August 5, 2022 02:12

zeripath removed type/bug lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. backport/v1.17 labels Aug 21, 2022

lunny removed this from the 1.18.0 milestone Dec 20, 2022

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protect against ReferenceError when creating SharedWorker #20575

Protect against ReferenceError when creating SharedWorker #20575

zeripath commented Jul 31, 2022

delvh Jul 31, 2022

zeripath Aug 1, 2022

delvh Jul 31, 2022

wxiaoguang Aug 1, 2022

zeripath Aug 1, 2022

wxiaoguang Aug 1, 2022

delvh Jul 31, 2022

delvh Jul 31, 2022

delvh Jul 31, 2022

zeripath Aug 1, 2022

delvh Jul 31, 2022

delvh Jul 31, 2022

delvh Jul 31, 2022

wxiaoguang commented Aug 1, 2022 •

edited

Loading

zeripath commented Aug 1, 2022

wxiaoguang commented Aug 1, 2022 •

edited

Loading

zeripath commented Aug 1, 2022

wxiaoguang commented Aug 1, 2022

zeripath commented Aug 1, 2022

wxiaoguang commented Aug 1, 2022

Protect against ReferenceError when creating SharedWorker #20575

Protect against ReferenceError when creating SharedWorker #20575

Conversation

zeripath commented Jul 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wxiaoguang commented Aug 1, 2022 • edited Loading

zeripath commented Aug 1, 2022

wxiaoguang commented Aug 1, 2022 • edited Loading

zeripath commented Aug 1, 2022

wxiaoguang commented Aug 1, 2022

zeripath commented Aug 1, 2022

wxiaoguang commented Aug 1, 2022

wxiaoguang commented Aug 1, 2022 •

edited

Loading

wxiaoguang commented Aug 1, 2022 •

edited

Loading