Electron ipcRenderer.sendSync() with 'is-mutation-observer-enabled' channel without listeners.

[kylegundersen]

I have found that if you start with enableBlockingInSession using the default session. Then you try to disable those Ads using disableBlockingInSession. It will throw an error and make any associated webContents become unable to receive input.

WebContents #24 (which is the ID) called ipcRenderer.sendSync() with 'is-mutation-observer-enabled' channel without listeners.

Seems that there is no listener present, I am not 100% sure for what is-mutation-observer-enabled is used for considering it looks to be a one-time configuration. (didn't have time to dig for that long.)

By removing it from the preload script everything works as expected.

My question is what is the use case for is-mutation-observer-enabled?

[remusao]

Hi @kylegundersen,

Thanks for reaching out. This is-mutation-observer-enabled message is used to know if the page should create an instance of MutationObserver in the frame to keep track of changes and potentially block/hide ads that are loaded later on.

The code that sets this listener is here: https://github.com/ghostery/adblocker/blob/master/packages/adblocker-electron/adblocker.ts#L87

And the code that removes it is here: https://github.com/ghostery/adblocker/blob/master/packages/adblocker-electron/adblocker.ts#L113

In principle, on unload we also remove the preload script so there should not be any such message sent after the unload.

[kylegundersen]

Ok so based on your feedback and if anyone else is running into this issue, you just need to set loadCosmeticFilters: false when using ElectronBlocker.fromLists.

Thanks @remusao for the quick and detailed reply and all the work yo do on this open source masterpiece.

FYI I am running the latest version of Electron. I am not sure if this issue would be considered closed, I just kind of danced around it with the conditional. Is there any disadvantage to just removing line 113 that removes that listener? (Assumptions: If I am not mistaken it will get overwritten anyways if a new one is spawned with the same channel name, and I am pretty sure they don't tie up much resources.)

[remusao]

Ok so based on your feedback and if anyone else is running into this issue, you just need to set loadCosmeticFilters: false when using ElectronBlocker.fromLists.

If you do so, you will disable part of the adblocking capability. Everything but the network blocking code, which might not be desirable.

Thanks @remusao for the quick and detailed reply and all the work yo do on this open source masterpiece.

Thank you very much for the kind words.

Regarding your last remark, I am not sure how removing the line would solve the warning you observed. Do you have some minimum test case to reproduce the issue? Is it consistently happening when disabling blocking?

[kylegundersen]

Ad-blocker-freeze-example.zip
I slightly modified the example project this repo provides.
Just run the npm scripts init, build, and start
Then use f10 to toggle between enabling and disabling the ad blocker.

You will notice this is a very simple Electron project and it does happen every time.

1. Once you click on the page to focus.
2. Then use f10 the first time the adblocker will become disabled.
3. On the next interaction (search in Google or page refresh) that message will pop in the VSCode console and the app will become unusable.

Thanks, hopefully this helps.

[kylegundersen]

I investigated it more thoroughly this morning.

So the issue is that the event is-mutation-observer-enabled is set on the preload. Then when Ads are disabled it removes the listener. The preload however knows nothing of this operation and will call the is-mutation-observer-enabled event regardless. Normally this wouldn't be a problem but the root of the problem lies in the fact that it uses sendSync, this will freeze the app until it receives a response. So if it never receives a response then it will freeze the app indefinitely.

Two possible solutions:

1. If you want I can PR the removal of line 113 as previously suggested. This is my recommended for lowest complexity and impact.
2. We could reengineer it to use sync instead of sendSync which would also make it asynchronous, but I can foresee this creating unwanted side effects.

Let me know your thoughts?

[remusao]

Thanks for investigating. I do not understand why the preload script is not removed when the adblocker is disabled? My understanding is that the call to disable blocking in session should also remove the custom preload script, which means the is-mutation-observer-enabled event should neither be emitted nor listed to.

[kylegundersen]

Yea I checked the code and it seems to give me the expected session.preload value, but your right it doesn't seem to update what is happening within the currently running webContents. I wonder if we should escalate to the Electron team. At least they can confirm if this is expected behavior or if there an issue.

[remusao]

Good idea, it is not clear to me if this is a new behavior because at the time this code was written I am pretty sure it was pretty extensively tested. Maybe @sentialx has some insight into this behavior. Do you remember if disabling blocking was also impacting existing preload scripts?

@kylegundersen What I find weird is that even if the existing preload script are still running, the is-mutation-observer-enabled event should be emitted only once at the beginning. So disabling blocking in session after that should not cause any more events to be emitted. Except, if new frames in the same page still get the preload script injected even after blocking was disabled. Any chance you could check if that is the case?

[kylegundersen]

@remusao I performed a loadURL directly after setting it to disabled and it worked as expected. So it must be continuing to publish events to is-mutation-observer-enabled in the current frame. That being said I tried reload and reloadIgnoreCache and it still froze the app. So it looks like only certain functions trigger refreshing the preload scripts.

[kylegundersen]

@remusao this library is broken for Electron as of now, do you want me to PR the removal of that line until someone has time to look into this deeper?

[remusao]

If you have a partial fix then yes. So the quick fix is to never remove the listener so that we do not get the error right? Since I am not actively using the adblocker in electron I would appreciate some help if you have time to get to the bottom of this. I'm happy to help.

@sentialx Is it also broken for your project? (If you are still using the library)

[kylegundersen]

Yes that is the only way in my mind. We could change send to sendSync to send but in my mind that could miss ads/junk that triggers very quickly on a page. Which I am assuming is the reason sendSync was used in the first place. I think this might just be the best fix period. The only other way is to block page loading a different way and use send which would require a lot of testing and might not be as future proof. Submitted PR #2449

[remusao]

This is still not so clear to me why removing the preload script is not enough. In all the existing frames where the preload script was already injected then the event should have been fired already, and all new frames should not get the preload script after disableBlockingInSession is called. So why do we still have preload script trying to send this sync event after the unload?

[kylegundersen]

@remusao yea its not clear to me either, I can only speculate that there could be some caching going on behind the scenes. The more I think about it , not removing that listener is probably the best long-term solution. Unsubscribing from any sendSync listener is a dangerous game. Your basically relying on the Electron team to not change anything in their session process, which from the looks of it, they did. I am just as curious as you, but I think we can close this issue.

[sentialx]

@remusao Sorry, I no longer use Electron for my browser. I didn't really use disableBlockingInSession, I made my own function to disable the adblocker. But this warning means that there is no listener for such channel in the main process.

[remusao]

Thank you for taking the time to comment here @sentialx.