Fix resolving the promise if there is no scripts to inject

[smalluban]

Fixes ghostery/ghostery-extension#806. Related to try of fix in ghostery-common in ghostery/common#47.

The PR fixes a case, where the promise is not resolved nor rejected. I think it was not intentional, as the last line of the function contains await Promise.all(... which is useless as the returned promise from the function is not used, but the promise generated by hand).

However, the PR changes the logic, so the promise (and returned response from the event) is now always resolved after those promises resolve. @remusao please take a look, as you make the last changes in that part.

[smalluban]

@remusao @philipp-classen I made a tiny change to bring back the original flow, but at the same time, fix the issue. I recommend merging that PR, as I see here and there some comments over the internet that Ghostery is responsible for ugly error messages in the console.

[philipp-classen]

My main problem is that I still get the error with these changes in Ghostery. I used "yarn link" for all adblocker modules in common and ghostery-extension, and I see the modified code in the debugger:

Not sure what I'm doing wrong. Maybe the errors are coming from different parts, or the change is not working.

Also, note that the polyfill library has the following semantic:

• If the callback was fired, then nothing will be done
• If the callback was not fired but a promise was returned - which is always the case for us, since our function is async - it will send the response back.

So, the content script should receive a response (resolving to undefined) if no scripts are injected. I assume the reason for the warning is somewhere inside the library code.

What does fix the error message is to manually apply the proposed changes in the polyfill library: mozilla/webextension-polyfill#385 That makes the errors go away for me.

[smalluban]

@philipp-classen When I'll have some more time, I will try to test it out again if it is sufficient, but on the sites, I was testing, the message is gone with my simple fix. Nevertheless, the bug is there, so I strongly advise just merging the PR. It helps in many cases. Even if it would not help, the code is currently wrong (never resolving promise).

[smalluban]

@philipp-classen @remusao I made another approach to the problem. You might be right @philipp-classen about that the last change did not help. It looks like the adblocker is initialized in ghostery-common, where it runs this class method expecting old API:
https://github.com/ghostery/common/blob/main/modules/adblocker/sources/background.es#L247

I pushed the change, so the promise is resolved, but the old API still works. From my understanding, the logic is still the same.

If the fix does not help with the console error message, I found a place where every call to the background touches one place, so I used it (writing a promise wrapper with timeout), to check if there are some other actions, which are never resolved:
https://github.com/ghostery/common/blob/main/platforms/webextension/content-communication-manager.es#L39

[philipp-classen]

Also sharing here: I'm fine with merging if it improves the readability. I still think, given the semantic of the polyfill-library, the library will in any case trigger the callback:

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

"common" is still my best guess of the place where the hanging promise comes from, especially this part, which can result in a promise that will never resolve, nor reject:

https://github.com/ghostery/common/blob/f1d4bcf465540c879185ba9c512291c11382bc53/modules/adblocker/sources/background.es#L246

But if the code becomes clearer with the PR and there is less magic needed to understand that the polyfill provides, we can merge. I just think that the hanging promise is when the adblocker engine is used with the common wrapper. That is also in line with the observation that the warning can not be reproduced with the standalone example webextension in the adblocker, but only with Ghostery.

[smalluban]

About the polyfill, we are always returning the promise, so the check does not do the trick for us ;)

And about the common, this is the place where uses adblocker-extension in a way that it must always run the fourth argument, otherwise it can hang.

so.. what about the PR, is there anything else we can do better? The bug is obvious, we have to resolve the promise, the flow is fine, and it does not break anything. Can we merge it?

[remusao]

Please have a look at the issue in comments. Let's do a little more testing locally before merging since tests and type checker are not enough to uncover all potential breakages.

[remusao]

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?

[remusao]

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.

[philipp-classen]

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.

[chrmod]

Following suggestion of @philipp-classen - this PR #2980 should ensure that sendResponse is always called.

[philipp-classen]

Fixed by #2980