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

fix: angular wrapped mutationobserver detection #1597

Merged
merged 7 commits into from
Dec 6, 2024

Conversation

pauldambra
Copy link
Contributor

@pauldambra pauldambra commented Nov 19, 2024

When Angular wraps MutationObserver and rrweb tracks mutations you get trapped in a fight with Angular's changedetection and performance suffers. We had multiple reports at posthog of performance degrading with recording active - even to the point of freezing the page entirely

We discovered that the checks for whether to load mutation observer from an iFrame were not correctly detecting the "taint" of angular wrapping the mutation observer.

It turns out detecting whether Zone is on the global object is a good enough proxy for Angular having tainted something

(there is some hand-waving here since you can configure zone.js to use a different global object name, but my guess is most people don't, and anyone with enough tech savvy to do that on purpose can also run the recorder outside of the angular zone)

This is confirmed as fixing performance issues in posthog-js v 1.187.1

You can see a wrapped MutationObserver from angular in this screenshot

Screenshot 2024-11-28 at 19 05 39

a posthog customer provided this repro https://github.com/typedb/angular-posthog-freeze
with that when session recording started the page froze immediately
with this fix it does not

Copy link

changeset-bot bot commented Nov 19, 2024

🦋 Changeset detected

Latest commit: 7336ce9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 19 packages
Name Type
@rrweb/record Patch
rrweb Patch
rrweb-snapshot Patch
rrdom Patch
rrdom-nodejs Patch
rrweb-player Patch
@rrweb/all Patch
@rrweb/replay Patch
@rrweb/types Patch
@rrweb/packer Patch
@rrweb/utils Patch
@rrweb/web-extension Patch
rrvideo Patch
@rrweb/rrweb-plugin-console-record Patch
@rrweb/rrweb-plugin-console-replay Patch
@rrweb/rrweb-plugin-sequential-id-record Patch
@rrweb/rrweb-plugin-sequential-id-replay Patch
@rrweb/rrweb-plugin-canvas-webrtc-record Patch
@rrweb/rrweb-plugin-canvas-webrtc-replay Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pauldambra pauldambra closed this Nov 19, 2024
@pauldambra pauldambra reopened this Nov 19, 2024
@Juice10 Juice10 self-requested a review November 29, 2024 09:05
Comment on lines 43 to 48
if (!isFunction(x)) {
return false;
}
const prototypeKeys = Object.getOwnPropertyNames(x.prototype || {});
return prototypeKeys.some((key) => key.indexOf('__zone'));
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, no, i've just had to change this in prod at posthog

will wait for a few more customers to report it works now

turns out I'm still an idiot, -1 is truthy, and this always returns true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interestingly, that meant that all browsers and sessions were treating all things as tainted and loading them from an iframe

in safari and mobile safari particularly this meant often there was no mutation observer

which suggests that the iframe loading code is bugged in some way (or just not guaranteed maybe)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, lots of reports that this change has fixed things for folk who aren't using Angular
And for folk using Angular there's no more reports of perfomance issues

But I still see folk where they are using Angular and they can't get untainted things from an iframe when using Safari

@Juice10 Juice10 merged commit bd9eb70 into rrweb-io:master Dec 6, 2024
6 checks passed
guntherjh pushed a commit to pendo-io/rrweb that referenced this pull request Dec 18, 2024
* fix: angular wrapped mutationobserver detection

* add change set

* fix

* prettier

* following posthog prod

* manually prettier

* Update .changeset/moody-experts-build.md

Co-authored-by: Justin Halsall <[email protected]>

---------

Co-authored-by: Justin Halsall <[email protected]>
guntherjh pushed a commit to pendo-io/rrweb that referenced this pull request Jan 18, 2025
* fix: angular wrapped mutationobserver detection

* add change set

* fix

* prettier

* following posthog prod

* manually prettier

* Update .changeset/moody-experts-build.md

Co-authored-by: Justin Halsall <[email protected]>

---------

Co-authored-by: Justin Halsall <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants