Significant performance impact when large amount of DOM elements is created

[Celsiusss]

Steps To Reproduce

When website that modifies or creates large amount of elements (large tables or lists), the Bitwarden extensions causes a large performance impact.

I created a simple demo here: https://jsfiddle.net/agos6npw/18/ Open the console to see the time it takes to update in ms. Click the update button.

Expected Result

When Bitwarden is enabled, it should not cause a significant impact on rendering.

Actual Result

Using the provided demo, each update on my computer takes around 220 to 250ms. Without Bitwarden it is a consistent 40-50ms. That is over 6x as long.

Screenshots or Videos

A screenshot from the Firefox performance profiler, notice how the Bitwarden extension has a major running time, and 70% of the samples. This is caused by the bootstrap-autofill-overlay.js script.

Additional Context

This causes a great deal of extra lag on a few web apps I use.

As a workaround, it would be nice to be able to disable the overlay script.

Operating System

Linux

Operating System Version

No response

Web Browser

Firefox

Browser Version

124.0.2 (64-bit)

Build Version

2024.2.1

Issue Tracking Info

• [X] I understand that work is tracked outside of Github. A PR will be linked to this issue should one be opened to address it, but Bitwarden doesn't use fields like "assigned", "milestone", or "project" to track progress.

[cagonzalezcs]

The performance issues that have been recently reported for the extension relate to the ShadowDOM fix that was introduced back in December. This fix introduced an exhaustive approach to handle identification of form field elements within potentially nested Shadow Roots.

Along with this change, we introduced a set of reactions to updates within the DOM to ensure we didn't have to unnecessarily re-query a web page for page details that are used in autofilling credentials. This was done through a mutation observer, but it definitely has a cost associated with its beahvior.

We've introduced a set of changes to resolve this. We're moving away entirely from the TreeWalker API approach we used previously to query ShadowDOM elements and instead handling querying those elements in a recursive manner using a shadowRoot.querySelectorAll(":defined"); call. This appears to be much quicker than walking through the entire DOM using the TreeWalker API.

The reasoning for this improvement isn't completely obvious, as other arbitrary tests shows that the TreeWalker API outperforms a querySelectorAll call for all elements on a page. However, it seems to work better for our use case so we're moving forward with that choice.

The second change that we're introducing here is a change to how we are handling updates to the DOM through our mutation observers. The process we undergo for updating our collection of page details anytime a DOM element is modified can be pretty expensive. We essentially have to query any potential form field elements within an inserted node. We also need to query potential shadow roots in those inserted nodes.

To resolve these issues in our mutation observer, we are delaying acting on any introduced mutation records using a requestIdleCallback call. In doing this, we delay processing of added nodes until the execution thread is in an idle state. This allows us to avoid introducing a large amount of jank/stuttering to a web page that is attempting to insert a large amount of DOM nodes.

I'd like to keep this thread open until these changes are release. While these changes do provide an improvement, I want to keep a discussion on this topic alive for now to facilitate suggestions for further improvements in relation to the autofill feature and collection of page details that facilitate autofill. Once we have resolve the largest issues, I'll consider closing the thread.

[wooque]

Performance impact is noticeable even if vault is locked. I think analyzing DOM/listening to DOM changes should be deferred when vault is unlocked, or maybe even when user interacts with Bitwarden (opening extension pop up or triggering keyboard shortcut to autofill credentials), otherwise it should not do basically anything.

[Celsiusss]

Hi @cagonzalezcs

Thank you for addressing this! The new approach seems to be a very good improvement, with little to no impact on performance.

Didn't know about the :defined pseudoclass, cool!

The benchmark in the SO post you linked returns every element in the DOM tree, but in our case we need to find a few specific elements in a potentially large DOM tree. The benchmark also shifts the entire array on the querySelector tests.

[cagonzalezcs]

@Celsiusss

Hm, yeah the unshift to insert the root at the front of the array of elements does explain the performance difference in that SO example.

My guess is that the implementation used under the hood when querying the DOM using querySelector improves performance in some manner, whereas iterating over a TreeWalker is very much a brute-force approach. It made sense originally to go with that approach due to its exhaustive nature, and also because it involved only querying the DOM once. This new approach requires re-querying the DOM a couple of times to achieve the same result, but that doesn't seem to be a huge performance cost.

@wooque

Thank you for the suggestion, I think it's a good idea. We recently implemented a methodology that allows us to dynamically register the content scripts used for Passkeys based on user settings. This allows us to hold off entirely on injecting those scripts when a user has the featured turned off, and when switching the setting, the scripts dynamically are registered and injected into all tabs (or de-registered and destroyed if the feature is turned off).

I'd like to implement something similar to that for autofill's content scripts, but it might take us some time to do so as we have a couple of other larger concerns to address now that we're getting manifest v3 sorted out.

[cagonzalezcs]

I'm reopening this ticket as the performance issues experienced will persist with the latest extension release.

The performance improvements detailed above function well in the context of a DOM that has no ShadowDOM elements or shallowly nested ShadowDOM elements. Querying elements using a standard document.querySelector, and then recursively filtering ShadowDOM roots seems to provide an adequate bump in performance.

The same cannot be said when a web page contains several deeply nested ShadowDOM elements in its structure. In effect, the fixes applied can break functionality entirely on websites such as this one.

We've fallen back to the TreeWalker strategy for now which has its hangups, but on average functions in a relatively performant manner for the majority of use cases.

[ChiragMoradiya]

We noticed that our App (https://a.kerika.com/) is being affected by this issue, as the app has been built using Web Components using Shadow DOMs heavily.

Users on 2024.4.0 version of Chrome Extension are not affected. But, Users on 2024.6.0 are affected.

[ChiragMoradiya]

oh! Sorry! I realised that the issue has been fixed in the latest version 2024.6.1. Thank You!

[theofficialgman]

Would like this resolved as well. Seeing anywhere from a 15-45% performance hit (in load times http://www.raymondhill.net/ublock/pageloadspeed.html , try any site basically) with bitwarden installed and enabled (chromium browser) depending on the site and device in use. chromium's builtin password manager does not seem to be affected by this issue. what sort of functionality do browsers need to provide so you can take advantage of their fast builtin parsing methods that I assume their native password managers use?

[richhollis]

Same here - the performance hit is considerable in an app that I'm developing - it does add/remove elements when changing views. I am not using Shadow DOM. I was hoping that this might have been fixed by now. Each yellow stack area in this trace is BW doing it's mutations thing:

Would it make any sense to be able to mark parts of the DOM that will NEVER require BW, so as to hint to BW that it does not need to deep walk that part of the tree? In my app, this would the highest percentage of the DOM. e.g. data-bw-exclude-node=true or provide a way for the mutations to check specific selectors maybe?

[Tiedye]

Any solution in the works? This is brutal on some sites

[creekorful]

Hello there,

We are facing huge performance issue on a ERP we are developing.

The extension add us ~4sec of loading time on some pages. Disabling the extension make the page indeed load faster.

Any fix planned?

[layus]

This is probably not trivial to fix, and there will always exist websites that break the form detection logic one way or another. I tried to blacklist the sites that BW slows significantly, but blacklisting only applies to the pop-ups, not the form detection. A simple (transient ?) workaround could be to extend that blacklist to DOM inspection too.

[cagonzalezcs]

Thank you everyone for your patience regarding this issue, I know that it has been an annoying element of our autofill feature.

I've implemented a set of fixes that I think should help resolve the problems experienced.

https://github.com/bitwarden/clients/pull/10816

These fixes incorporate logic that further defers the mutation observer logic that we use to attempt to re-capture and cache DOM mutations that occur within a page. Each mutation we process is fairly taxing to re-assess, so we're deferring each of those to the next available tick within the JS event loop.

The fix also contains logic that will attempt to assert whether to use the TreeWalker API or the querySelectorAll API if a website does or does not contain shadow DOM elements. From my testing, the TreeWalker API is much more efficient when attempting to query elements within a large DOM that contains ShadowRoot elements. However, querySelectorAll works more efficiently for smaller datasets, and we have a recursive method that allows for querying deeply into nested ShadowRoots using this query method.

That said, if the depth of a DOM tree with ShadowRoots is very large, we encounter significant performance concerns with a deep querySelectorAll approach. So to counter that, when we do use querySelectorAll to identify elements in a DOM tree, if we hit a ShadowRoot depth of 4, we fallback to the TreeWalker API.

In practice, I'm seeing this methodology work well for improving performance on websites that contain known issues. I'm also seeing performance improvements on websites that do not use a Shadow DOM implementation at all.

With all of this said, I'd definitely appreciate any feedback from the community on their experiences with the fixes I've implemented. If you're open to it, you can sideload these changes by loading the PR's extension artifact from here: https://github.com/bitwarden/clients/actions/runs/10634576590?pr=10816

Download any of the dist-<browser>-35719bf.zip builds to test this behavior.

I'd also like to mention that I think the idea of an "exclusion list" for websites for autofill behavior/features is a good one. I'll be bringing that up to the team and try to get some traction for how we can incorporate that idea.

[layus]

@cagonzalezcs It seems to have fixed the slowdown on the website where I noticed the issue. Thanks :tada:

[cagonzalezcs]

That's fantastic to hear, thank you for checking it out.

We'll be moving to get this work into the next release of the extension.

[richhollis]

Hello @cagonzalezcs

Thank you for looking into this.

I've just loaded the firefox extension from the debug page (about:debugging#/runtime/this-firefox) - I'm assuming that's the correct approach.

Unfortunately, I'm not really seeing any real improvement for my app:

My app has a fairly large grid/table of DIVs, some of which get updated with each navigation in the app and that's where bitwarden literally saturates the CPU. It's fine when it's run with BW not loaded:

As I previously suggested, may if you could mark a page or root level elements that shouldn't ever be walked, maybe that's the answer? In my example, I wouldn't ever want BW to run on this page, it's a total waste of CPU. On the login page, sure. Perhaps it's an issue with my app in the way it's doing the DOM manipulations?

Let me know if you need any more info.

Thanks

[cagonzalezcs]

Hmm... implementing an attribute that indicates to Bitwarden to ignore all children of a parent element would be easier to incorporate than an exclusion list for a given cipher/domain.

Let me put something together in the implementation. I'll set up the query to ignore/skip querying into DOM elements that contain an attribute of data-bwignore. Currently we ignore certain input elements in that manner, but I'll set something up that ignores all nodes contained within a node that has that attribute.

Will report back on this soon.

[cagonzalezcs]

@richhollis

I don't want to impose, but would it be possible to get access to some form of testing environment for your application? If not, that's cool... we'll try to work through a solution based on the details you've provided.

If so, since Github doesn't allow for private messages, could you please reach out my user on the Bitwarden community forums through a private message?

[theofficialgman]

This hasn't hit any current tags yet. For other users who want to track this issue's progress in making it into a "Browser" tag release the only way is to refresh this page https://github.com/bitwarden/clients/commit/b0e0e71974d93b16df9f06fd5daf37e8470219f8 and eventually when it the "Browser" branch diverges from a head position above that commit and a new tag gets made there will be a tag there.