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

Prevent webview from freezing due to error in _runHook #1301

Closed
wants to merge 1 commit into from

Conversation

hikaru-y
Copy link
Contributor

Fixes https://forums.ankiweb.net/t/2-1-45-release-candidate/11362/20 .

I know that add-on authors and users should make sure that there are no errors in the hook functions, but I created this PR because the clayout's webview also freezes while editing a template.

@hgiesel
Copy link
Contributor

hgiesel commented Jul 18, 2021

This will start all promises sequentially, instead of running them in parallel. The catch should be on the Promise.all() call.

@hikaru-y
Copy link
Contributor Author

hikaru-y commented Jul 18, 2021

@hgiesel Thanks for pointing that out.

The catch should be on the Promise.all() call.

This means something like the following?

--- a/ts/reviewer/index.ts
+++ b/ts/reviewer/index.ts
@@ -27,14 +27,14 @@ export function getTypedAnswer(): string | null {
     return typeans?.value ?? null;
 }

-function _runHook(arr: Array<Callback>): Promise<void[]> {
+function _runHook(arr: Array<Callback>): Promise<void[] | void> {
     const promises: (Promise<void> | void)[] = [];

     for (let i = 0; i < arr.length; i++) {
         promises.push(arr[i]());
     }

-    return Promise.all(promises);
+    return Promise.all(promises).catch((e) => console.error(e));
 }

 let _updatingQueue: Promise<void> = Promise.resolve();

I tried this, but it did not prevent the webview from freezing.

As you said, in this PR, hooks are executed sequentially. However, considering the case where there are multiple hooks that operate on the same DOM, I thought sequential execution would be more appropriate for this function than parallel execution, so that's what I did. I should have written the reason why I did it that way, though.

For example, if there are multiple hooks for manipulating images with querySelectorAll("img"), I think that parallel execution would not guarantee consistency of the results. Am I wrong?


Edit: If sequential execution is not appropriate, I will update the PR soon.

@hikaru-y
Copy link
Contributor Author

Closed this PR. See #1302 (comment) .

@hikaru-y hikaru-y closed this Jul 19, 2021
@hikaru-y hikaru-y deleted the fix-error-handling-in-hook branch March 25, 2022 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants