getShadowRoot Browser hangups

jasonparallel commented 1 month ago

Steps To Reproduce

The removal of the || node.childNodes.length !== 0) check in getShadowRoot made here https://github.com/bitwarden/clients/commit/bbf14730222408292a2e00dd15c7b4ccc74cba8d#diff-8ff1ef69519807ae86fc872374b223656c9459d1b8b0304e8a2b602182b68f98 is causing 1 sec+ browser hangups when it runs on a select box with a large number of select options.

Also the comment on the method is not longer accurate. "Will return null if the node is not an HTMLElement or if the node has child nodes."

Expected Result

Don't hang up the browser

Actual Result

The browser hangs up

Screenshots or Videos

No response

Additional Context

No response

Operating System

macOS

Operating System Version

No response

Web Browser

Chrome

Browser Version

No response

Build Version

6.1 (did not happen in 5.2)

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.

Neonwarden commented 1 month ago

Hi there,

I am unable to reproduce this issue, it has been escalated for further investigation. If you have more information that can help us, please add it below.

Thanks!

jasonparallel commented 1 month ago

I'll see if I can create a small example page.

jasonparallel commented 1 month ago

I modified the bitwarden plugin (pulled from github) to add timing around chrome.dom.openOrClosedShadowRoot(node) open https://codepen.io/jasonparallel/pen/oNrbWod with bitwarden thinking it has a password for codepen.io (not sure if this is needed) I got a 4174 ms lock up on openOrClosedShadowRoot

Prior to the referenced change, nodes like this would not have openOrClosedShadowRoot run on them.

jasonparallel commented 1 month ago

@Neonwarden Was that helpful? @cagonzalezcs Did you change intent to no longer exclude nodes with child nodes?

cagonzalezcs commented 1 month ago

@jasonparallel

So the reversal on checking for child nodes when asserting a ShadowDOM element is indeed purposefully done, and the behavior you're describing was resolved with the deepQuery methodology introduced with https://github.com/bitwarden/clients/pull/9063

If you look at the thread mentions at the bottom of the PR, however, we ran into some heavy issues on websites that deeply nest ShadowDOM elements. As a result, we've hardcoded a feature flag that facilitates a reversal of the reworked implementation.

The reason why we're once again checking for child nodes is because of examples such as the one referenced in this Github issue - https://github.com/bitwarden/clients/issues/9566

The form reference in that thread has a custom web component that contains both a shadow root and a regular node. In effect, discarding nodes that contain a child breaks that form's autofill behavior.

I'll be revisiting the performance improvements soon. When a PR for that comes into play, I'll try to remember to post a link to it here.

bitwarden / clients