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.

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

Fix resolving the promise if there is no scripts to inject #2724

Closed
wants to merge 1 commit into from
Closed

Fix resolving the promise if there is no scripts to inject #2724

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
2 changes: 2 additions & 0 deletions packages/adblocker-webextension/adblocker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,10 +402,12 @@ export class WebExtensionBlocker extends FiltersEngine {
scripts,
styles: '',
});
return;
}
}

await Promise.all(promises);
sendResponse();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that there are other cases above in the code that short-circuit the execution of the function with return; without calling sendResponse. Would it make sense to also call sendResponse on all of them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tried it locally and found cases where this change causes leads to exceptions raised: https://github.com/ghostery/adblocker/blob/master/packages/adblocker-webextension-cosmetics/adblocker.ts#L210

This function above is given the response as is and if it is undefined then an exception is raised.

image

Copy link
Member

Choose a reason for hiding this comment

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

I have been thinking about it again. Adding a sendResponse call to all exit paths like Remi suggested, would make sense IMHO; arguably, using sendResponse({}) instead of sendResponse() to avoid the errors with code that is not prepared to handled undefined. I have not tested it, but I would expect it to work.

};

/**
Expand Down