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

High CPU usage due to iframe workaround #179

Closed
ghost opened this issue May 6, 2021 · 26 comments
Closed

High CPU usage due to iframe workaround #179

ghost opened this issue May 6, 2021 · 26 comments
Assignees
Labels
bug Something isn't working electron issue Issue of the Electron project, unsolvable in Vieb

Comments

@ghost
Copy link

ghost commented May 6, 2021

Hello Everyone,

I discovered Vieb 2 days ago, and I realy need to say that its a very cool browser, wouldn't there be this one issue.
Okay, the browser is working pretty fine, but the cpu usage is extremly high, even by only watching the youtube start page:
https://ibb.co/jRg08Rp
(I've made that screenshot after about 30 secs being on the site, you can see that one thread is over 70%, coused on the browser.)
Hopefully someone can help me.

Why is that a problem?
Couse I have a laptop, and It would be cool if the cpu usage isn't that high so I can use it longer.

Hardware/Software
ThinkPad E15 Gen 2
Intel i7-1165G7
Intel Iris Xe Graphics
16gb ram

Arch Linux x86_64
Open source Intel video drivers (xf86-video-intel, mesa)
Window Manager: awesome
Xorg, no display manager

What I've tried

  • reinstalling my video drivers
  • running it with --software-only (is there no hardware acceleration for me?)
  • use the Appimage (same issue there)

When I've tried other stuff, I will tell you about that here. hopefully I don't raste to much of your time.

@Jelmerro Jelmerro pinned this issue May 6, 2021
@Jelmerro Jelmerro added the question Further information is requested label May 6, 2021
@Jelmerro
Copy link
Owner

Jelmerro commented May 6, 2021

I don't understand what makes you think youtube is a light website, it's actually one of the heaviest and least optimized pages out there, usually by design.

Hardware rendering is enabled by default for supported systems, and you can check if it's enabled at chrome://gpu/. If not, that explains the high CPU usage, but also means there is not much we can do to optimize it. If it is enabled, it would help to find other pages where the cpu usage is that high, but for now I suspect it's just youtube being youtube. Please let me know your results after checking the gpu page and doing some further testing.

@Jelmerro Jelmerro unpinned this issue May 6, 2021
@ghost
Copy link
Author

ghost commented May 6, 2021

hardware accalaration is enabled.
Opening the webseite with firefox will use about 4% of the cpu, vieb about 12%, but with one thread at 100%.
If you don't have any clue about how to continue, that very sad, but okay, then I will probatly not be able to use that.
Anyways, its a great project anyways!
Btw. its the same on every webseite with more then 2 images (for example twitter)

@Jelmerro Jelmerro added bug Something isn't working electron issue Issue of the Electron project, unsolvable in Vieb and removed question Further information is requested labels May 6, 2021
@Jelmerro
Copy link
Owner

Jelmerro commented May 6, 2021

I just visited youtube and ran some performance tests, I think I have actually located the function that's responsible, though in my case the performance isn't nearly as bad as in your case. Sadly that is code we can't get rid of or change much, as it's essential code to make any interaction in iframes possible. This is needed because of a missing Electron feature. Basically, it's a loop that sets the right navigator properties and APIs inside of the iframes to achieve any kind of interaction, but also to block access to certain sensitive APIs. It's also needed to make screensharing work for requests made from within iframes. Also, it doesn't seem like the Electron team will pick this up any time soon, so I'm sorry to say that this will probably remain an issue for a while.

@errge errge mentioned this issue May 31, 2021
3 tasks
@errge
Copy link
Contributor

errge commented May 31, 2021

@Jelmerro could you provide a description of the function that you found as responsible and maybe a hacky branch where that is disabled (even at the expense of that branch being incorrect regarding iframes/follow/security)? This would help, so that I could try out Vieb performance now assuming the future when this bug is fixed in electrum.

@Jelmerro
Copy link
Owner

This is the problematic loop, but it's required to keep the following working in iframes:

  • Security fixes such as navigator overrides
  • Screensharing
  • Context menu and other click related functionality

The reason for the infinite loop is because event listeners for iframes will be removed if the frame is updated in certain ways by the webpage, which means we need to reapply the listener as frequently as possible. This is a hacky workaround for this missing feature of electron. Once that is fixed, we can properly apply the fixes inside the iframe itself before it loads, but until then this loop is required to keep the feature list above working inside iframes. Running the loop on a slower interval is possible, but will compromise those features up to a maximum of the interval you choose to use.

@errge
Copy link
Contributor

errge commented May 31, 2021

Thank you for your kind and detailed explanation. I understand that increasing the interval would make the APIs exposed for that amount of time at the start of an iframe/new page load. But, is the security/privacy issue present even with setInterval(0), if somebody is intentionally attacking it (in another busy-loop)? I mean, what gives you the guarantee, that if the attacker webpage trying to access the API in a setInterval(create_iframe && attack, 0) loop, will always be scheduled after your setInterval(protect_us, 0) loop?

If the missing nodeIntegrationInSubFrames feature is implemented in electron, will our code always run before the guest code of the webpage?

Also: for features that we completely don't need in the whole of Vieb (e.g. battery, graphics card information, experimental keyboard API, connection information, plugins), wouldn't it make more sense to ship with an electron, where those features are patched out completely? Is that too hard? (Sorry, I've never patched/read electron's source code.)

And when @gebner is saying "cleaning up using event handlers", is that not possible at all without the electron bugfix?

@Jelmerro
Copy link
Owner

Jelmerro commented May 31, 2021

setInterval(0) is the closest we can currently get without patching Electron. I have never written C++, and most times I check how something works in the source code it takes me quite a while to even understand what is happening. We need a preload for iframes, and the nodeIntegrationInSubFrames has two issues:

  • Firstly it doesn't actually work for iframes anymore, ever since this weird PR, which literally breaks the documented feature (that is STILL documented in the same way as before the PR. It claims to run in all subframes, even iframes, but it obviously doesn't do that since this PR)
  • Secondly, it will enable node integration for the webpage, which we don't even want, we just want to run some kind of code in the iframe before it loads the page. Providing sites with node integration is a big red flag, we don't want to do that, even if the feature would work as documented.

I am all ears to make this work any better than it currently does, and it would solve a lot of iframe related issues, but currently Electron doesn't offer any kind of solution, and I couldn't figure out how to patch it myself. In the meantime, we need this hacky workaround to at least try to patch this as soon as the iframe loads, hopefully before any code from the page tries to reach those APIs. It's definitely not waterproof, and to even make this work we need to disable strict site isolation by default as well. It's a mess to be honest, but I can promise that I'm trying my best to patch this missing Electron feature. It's just that I haven't found any way to do that yet (half the issues of Vieb flagged as "upstream issues" are in a way related to this missing feature).

@errge
Copy link
Contributor

errge commented May 31, 2021

Thank you @Jelmerro . I read a bit of frustration in your writing, and I just wanted to assure you, that in my opinion you are doing the absolute best that is possible in this ugly world of web browsers, web engines and web development in general.

I think this is now discussed enough and clear enough for people (including myself) to follow up, if our talent/time allows. It definitely helped me to understand the issue and provides me good pointers if I want to apply myself trying to come up with something for this. So thank you!

One last idea: shouldn't we maybe change the title of the bug to remove the reference to the graphics card and to show more clearly that this is an issue, where future development and hacking in C++ would be very valuable?

@Jelmerro Jelmerro changed the title High CPU usage on Arch Linux w. Intel Iris Xe Graphics High CPU usage due to iframe workaround May 31, 2021
@Jelmerro
Copy link
Owner

At times it can be very frustrating to know the issue, but not being able to fix it myself. If I had even the slightest idea of C++ knowledge I would, but it's just not something that I'll be able to soon. What boggles my mind is how issues like this can exist in such a big project like Electron for years, without anyone knowing how to fix it (including myself). My only explanation is severe understaffing of the Electron team, as this is far from the only issue that gets overlooked or buried by hundreds of more recent ones. I definitely don't want to make this sound like a rant against Electron, but sometimes it's just really frustrating to know that a big issue like this exists, and not being able to fix it or get anyone with the right knowledge to look into it. A recent example being that maximize events just don't work at all. I just don't understand how nobody knows how to fix (or can be bothered to fix) such a big issue that affects literally every project using Electron with a GUI (BrowserWindow is pretty much the basis of any Electron window). To anyone bothering to read this far, don't let this discourage you from using Electron, the team seems to be doing whatever they can to keep the project afloat, but please help them out if you can write C++ by contributing some PRs for long-running issues like these that really need fixing I think.

@danrobi11
Copy link

don't let this discourage you from using Electron

I have high cpu usage here too. The Min browser is also Electron based. However, min does not have high CPU usage. Therefore, im thinking, it should be possible to reduce the high CPU usage with Vieb as well (non-technical user here just sharing his thought). I love Vieb :)

@Jelmerro
Copy link
Owner

Min uses browserviews instead of webviews, which come with a whole range of different issues. I also don't know how well they are attempting to protect the Electron useragent and navigator properties. For Vieb to resolve the high cpu usage, Electron needs to solve the iframe preload ticket, regardless of whether min even attempts to solve this.

@KunaPrime
Copy link

I also have the same issue, about 50% on idle instance (only help page opened). And I have also noticed that htop shows 20.5G in VIRT field. I don't think it is concern but it is way higher than anything else on my system (by the order of magnitude at least) and i'm wandering is it related to this issue?

P.S. this project is great! It is everything I would like modal browser to be.

@Jelmerro
Copy link
Owner

Jelmerro commented Sep 3, 2021

Most definitely that this is causing your high CPU usage, sadly we can't really do without it until Electron adds the iframe preload feature.

@KunaPrime
Copy link

i was wandering is it also the cause of very high VIRT(20.5G) nunmbers?

@errge
Copy link
Contributor

errge commented Nov 29, 2021

Dear @Jelmerro et al,

I've posted a bounty on this issue on bountysource: https://www.bountysource.com/issues/98778088-high-cpu-usage-due-to-iframe-workaround

Naturally, to solve this, the electron issue has to be solved first, so I consider that part of the bounty, I just wanted to make sure, that whoever starts working on this is aware of the goal, not just the electron issue.

I also wanted to say, that if other backers join, that would be great, solving the CPU issue would be a big improvement for Vieb in my opinion. On the other hand, if @Jelmerro you think the whole bounty+money thingie is a very bad idea, then please say so, and I will contact bountysource to cancel and my apologies for not discussing this beforehand.

I hope this helps.

@Jelmerro
Copy link
Owner

If you want to pay somebody for implementing this feature, that's totally fine. I'm not involved in this deal, and will be reviewing the PR as usual without any bias. Let's just hope it helps motivate people to implement the required Electron feature to fix this.

@ayushkumar63123
Copy link

I have a workaround Just make a free google cloud account, get free credits by adding a credit card for verification, then use the browser on a cloud rdp for free!

@ayushkumar63123

This comment has been minimized.

@errge
Copy link
Contributor

errge commented Nov 30, 2021

@ayushkumar63123 Dear Ayush, thank you for your suggestion, but obviously we want to run the browser on our own computers, and not remotely. The bounty is about fixing the underlying issue, not moving the heating of the universe from my flat to a data center. Therefore I will keep the bounty open for people who want to work on the real underlying issue in electrum (and then adopting the fix in Vieb).

@Jelmerro
Copy link
Owner

Jelmerro commented Jun 8, 2022

Fixed in the new 8.0.0 release!

@Jelmerro Jelmerro closed this as completed Jun 8, 2022
@errge
Copy link
Contributor

errge commented Jun 8, 2022

Hey, can you please provide a bit of technical description on what happened and how future proof the solution is? Is Vieb now not consuming CPU when idle? I'm about to click accept on the bounty page, but I don't have too much time now to read code, and I would like to evaluate, before I press it. Thanks, much appreciated!

@Jelmerro
Copy link
Owner

Jelmerro commented Jun 8, 2022

Basically, we run iframes in a separate process, with it's own preload, so the previous workaround infinite loop is no longer needed. This was achieved by disabling all workarounds and enabling two webpreferences of Electron to also run preloads in subframes and in workers. This required quite a rework to follow mode and pointer mode in particular. These Electron options/preferences have weird names, as they are called nodeIntegrationInSubFrames and nodeIntegrationInWorker, for which you can find more info on these links: electron/electron#22582 and electron/electron#28620. Because of this naming, I did not realize that enabling these preferences in combination with NOT enabling nodeIntegration, would still allow preloads to run inside subframes and workers, while not having node integration. It took me way longer than I would have liked to realize this, and my previous comments about these preferences have inconsistencies in them, that I now realize are not always how I understood them at the time.

As for how future proof this solution is, there are currently no active plans to remove node integration completely nor these additional options, there are however tickets to rename these preferences to reflect their true purpose, which is precisely how we are using them: run preloads inside iframes. I'm no Electron developer, and with this ticket it has become clear how big the dependency on the project is, but I hope that even if these options are ever removed from Electron we will by then have other ways to run preloads in iframes, for which there are currently issues in the Electron bugtracker to make it happen. I can't give any guarantees or dates on how future proof this is, but what I do know is that currently the options are used exactly as documented, I just wasn't familiar with how to use them and Vieb needed quite some work to account for previous assumptions and preload handling choices.

I hope this explains a bit of the process and steps taken, but I do recommend to check to code as well, especially so because it's quite a big diff and I don't mind you taking the time to evaluate this properly. In all fairness, most of this mess was caused by the incorrect names that these options currently have, and by myself for misunderstanding them and failing to make them work as described due to a variety of quirks related to activating them, such as:

  • Requiring ipcRenderer.send instead of ipcRenderer.sendToHost, the former sends it to the main process, the latter to the "host", which would be the web page preload (if code would run iframes), and NOT the renderer process, this means that we would not receive messages from the iframes, even if code was actually running there with nodeIntegrationInSubFrames enabled.
  • This also meant that any actions sending things to the frames now need a way to send things to ALL frames in a page, which becomes especially tricky if this data is location related. This is mostly why the diff is so big and why it mostly affects follow and pointer mode.
  • Some iframes do not get their own process even when these options are enabled, this is the case for some same domain iframes. These don't get a separate process, but the positive side is that they don't need to as all the properties that we would want to override are taken over by the window

Anyway, glad that this is now behind, as for the new cpu usage stats, they are impressive if I say so myself:

$ ps -C vieb -o %cpu
%CPU
 0.3
 0.1
 1.4
 0.0
 0.0
 0.0
 1.4
 0.3
 3.5
 0.0
 0.0
 0.0
 0.0
 0.0
 0.3
 0.2
 0.3
 0.0
 0.0
 0.2
 0.5
 0.0
 0.3
 0.0
 0.0
 0.0
 0.3
 0.1
 0.1
 0.0
 0.0
 0.0
 0.0
 0.3
 0.3
 0.3
 0.3
 0.4
 0.0
 0.1
 4.6
 0.4
 0.3
 4.1
 2.8

This is with 19 tabs open across two different instances/datafolders. Total usage from this command ranks up to ~23% but that's split across 16 threads, so the total usage per core is around 1.4% at the moment, which equates to below 0.1% of cpu usage per tab. You can also see in the screenshot below, where my CPU graph is now finally at rest (this is with more open that these two Vieb instances, but oh well):
image
Memory totals at 2.5GB for the same 19 tabs in 2 instances, averaging around ~130MB per tab, not that great, but also not affected much by this change I think.

@KunaPrime
Copy link

Thank you for your hard work on this!!! it is a huge thing and finally, I can set Vieb as my default browser.

@herrsimon
Copy link

I was suffering from the same issue and just reinstalled vieb due to the good news. However, while excessive cpu usage in normal mode indeed seems to be gone, it spikes enormously when entering visual mode (pressing v with the default keybindings). Tested this on various websites, as soon as visual mode is entered all my cores almost max out.

Should I open a new issue for this or is it related to the current one?

@Jelmerro
Copy link
Owner

@herrsimon That's a different issue, please open a new one with steps to reproduce.

@herrsimon
Copy link

Ok, will do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working electron issue Issue of the Electron project, unsolvable in Vieb
Projects
None yet
Development

No branches or pull requests

6 participants