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: Recursive logging bug with console recording #1136

Merged
merged 4 commits into from
Feb 16, 2023

Conversation

benjackwhite
Copy link
Contributor

@benjackwhite benjackwhite commented Feb 15, 2023

Originally reported here

Problem

Thanks to some debugging with a customer, we found an interesting "edge" case that can trigger an infinite loop via console logs. The context is this was a Vue.js app that was logging some warnings which contained objects within that warning with Proxied methods that triggered logs themselves.

The situation is like this (WARNING: If you run this with console log recording enabled your browser will freeze).

let target = {
  foo: "bar"
};

const handler = {
  get(target, prop, receiver) {
    if (prop === "foo") {
      console.warn("foo was accessed. This shouldn't be done... Context:", target)
    }
    return Reflect.get(...arguments);
  },
};

const proxy = new Proxy(target, handler);
target.proxy = proxy

console.log(proxy); 
  1. We log the proxy object
  2. The console log is intercepted by our code
  3. It creates a "copy" of the object via stringifying
  4. This triggers the proxied get call which itself is logging a warning
  5. ... which triggers Step 2 and now we have a loop 🙈

Solution

  • Flag when we enter the block to stringify and callback the code. If any console logs occur in that time, we don't capture them to avoid infinite loops

@changeset-bot
Copy link

changeset-bot bot commented Feb 15, 2023

🦋 Changeset detected

Latest commit: cdec9e6

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

This PR includes changesets to release 7 packages
Name Type
rrweb Patch
rrweb-snapshot Patch
rrdom Patch
rrdom-nodejs Patch
rrweb-player Patch
@rrweb/types Patch
@rrweb/web-extension 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

Copy link
Member

@YunFeng0817 YunFeng0817 left a comment

Choose a reason for hiding this comment

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

This a very interesting edge case. Thanks for your great description and fix.

@YunFeng0817
Copy link
Member

YunFeng0817 commented Feb 16, 2023

Could you please help us add the change log? Seems like I don't have permission to add it to your branch.

@benjackwhite
Copy link
Contributor Author

Happily! (How do I do that? 😅 ) Perhaps something to add to the PR template?

@benjackwhite
Copy link
Contributor Author

@YunFeng0817 I think I did it? (I just clicked the link in the bots comment but please double check I got that right 🙈 )

@YunFeng0817
Copy link
Member

@benjackwhite Yes, you did. I will merge it once CI passes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants