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

Return false from the handler, as it doesn't use sendResponse argument #47

Closed
wants to merge 1 commit into from

Conversation

smalluban
Copy link

Fixes ghostery/ghostery-extension#806.

We are not using the sendResponse third argument in the handleRequest function, so we should always return false. For now, it returns a promise, so the API holds for using the sendResponse, which never happens. As the result, the error message is thrown in the console.

@smalluban smalluban added bug Something isn't working patch Increment the patch version when merged labels Jul 20, 2022
@@ -36,7 +36,9 @@ function handleRequest(message, sender) {
return undefined;
}

return inject.module(module).action(action, ...args, sender);
inject.module(module).action(action, ...args, sender);
Copy link
Contributor

@remusao remusao Jul 20, 2022

Choose a reason for hiding this comment

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

How is the content script getting the response back if we do not return the promise?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch! For instance, adblocking fails then on https://spiegel.de/.

The warning does originate from this function, so it is the right place. But it is correct that returning the promise is important.

Copy link
Author

Choose a reason for hiding this comment

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

Right, i missed, that returned promise value works like sendResponse.. but what then it means that some of the sendMessage origin does not wait for the response. I suppose, that they don't care about it, but the above code sens a promise always, which causes the issue

Copy link
Member

Choose a reason for hiding this comment

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

Observation: in the debugger, I see that we are using the polyfill:

https://github.com/mozilla/webextension-polyfill/blob/780518ed1d9b05e6b31c4067d4db29927779abf3/src/browser-polyfill.js#L426

My guess is that we are using the API correctly, but the version of the polyfill creates issues on modern Chrome version. (?)

@smalluban
Copy link
Author

smalluban commented Jul 22, 2022

The core problem lies elsewhere. Chrome introduced a new error message lately if the listener of the message returns a promise, which is not resolved. What I found, the webextension-polyfill have mapping for those messages. Then instead of rejecting with an error, it resolves (I am not sure if this is a good practice, but Firefox does it, so the polyfill tries to mimic that behavior). More info here: mozilla/webextension-polyfill#384

I think I found the place, where we don't resolve a promise in the background listener in @cliqz/adblocker-webextension when the content scripts send a message for cosmetic filters: https://github.com/ghostery/adblocker/blob/master/packages/adblocker-webextension/adblocker.ts#L398

The sendResponse (the resolve method of the promise created in place) is not always called, but the listener always returns a new promise. As far as I tested it is safe to put sendResponse() (without arguments) at the end of the function, after line 408. The content script resolves result in the way result = result || {}.

I am closing the PR here, as the change must be done in the above package, and then reinstalling the ghostery-common package should use the latest version of the adblocker-webextension.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in console: Uncaught (in promise)
3 participants