Fix mutation edge case when blocked class gets unblocked #867

rahulrelicx · 2022-03-31T16:31:00Z

Context:

RRweb uses MutationObserver feature of browser to fetch 100s of mutation records.
Processes all these mutation records to generate adds, removes, attributes data of mutation event in a minimal fashion.

One of those optimizations is to ensure that rrweb doesn't generate adds data for a node if its parent is anyways being removed in another sibling mutation record.

Detailed Description of Edge case:

However, there is an edge case bug in this optimization exercise when we use the blockClass feature.
If an element is blocked, we don't serialize and record it.

Let's take a following scenario where we have some elements which are blocked from recording and we see a bunch of mutation records with following actions:

The element with blocked class name is changed to a different class name such that it is not blocked
Some child element is removed from the previously blocked class
Adds couple of element nodes to some other class such that there is at-least one element with a sibling

Since an element is removed (Action-2), we add it's nodeId to the removed set.
However since this node was not serialized (as it was part of a block class), we shouldn't track it's nodeId in the removed set. RRweb is smart and does check if a node was part of blockClass before adding it to the removed set. But in this case the element which was blocked has it's class name changed to an unblocked class (Action-1). So the check that the node was part of blockClass fails and we track an unserialized node in the removed set. For unserialized nodes, we track the id by value of -1.

Now coming to Action-3 where lots of element nodes are added to an unrelated element, some of them could have their parentId defaulted to -1 while being added to the DOM (since the order of serialization is not always from parent to child). So RRweb's optimization logic thinks that there is no need to add this element to the mutation event as it's parent -1 is part of the removed set.

How to repro?

Run rrweb replayer on this html document and click on both the Mutate buttons.
In the original session, each Mutate button will create two new buttons.
In the replay, each Mutate button will create only one new button.

<head>
  <title>Uber Application for Codegen Testing</title>

</head>

<body>
  <script>
      function mutate1() {
        const bClassDiv = document.getElementById("b-class");
        bClassDiv.className = "notB";

        const removeBlockedButton = document.getElementById("remove");
        removeBlockedButton.remove();

        const visibleCollection = document.getElementsByClassName("visible");
        const i1Div = document.createElement("div");
        const i1i1Div = document.createElement("div");
        const i1i2Div = document.createElement("div");

        const i1i1Button = document.createElement("button");
        i1i1Button.innerHTML = "I1I1 VISIBLE";
        i1i1Div.appendChild(i1i1Button);

        const i1i2Button = document.createElement("button");
        i1i2Button.innerHTML = "I1I2 VISIBLE";
        i1i2Div.appendChild(i1i2Button);

        i1Div.appendChild(i1i1Div);
        i1Div.appendChild(i1i2Div);
        visibleCollection[0].appendChild(i1Div);
      }
      function mutate2() {
        const bClassDiv = document.getElementById("b-class-2");
        bClassDiv.className = "notB";

        const removeBlockedButton = document.getElementById("remove2");
        const innerButton = document.createElement("button");
        innerButton.innerHTML = "INNER BLOCKED";
        removeBlockedButton.appendChild(innerButton)
        removeBlockedButton.remove();

        const visibleCollection = document.getElementsByClassName("visible2");
        const i1Div = document.createElement("div");
        const i1i1Div = document.createElement("div");
        const i1i2Div = document.createElement("div");

        const i1i1Button = document.createElement("button");
        i1i1Button.innerHTML = "I1I1 VISIBLE";
        i1i1Div.appendChild(i1i1Button);

        const i1i2Button = document.createElement("button");
        i1i2Button.innerHTML = "I1I2 VISIBLE";
        i1i2Div.appendChild(i1i2Button);

        i1Div.appendChild(i1i1Div);
        i1Div.appendChild(i1i2Div);
        visibleCollection[0].appendChild(i1Div);
      }
  </script>
  <br/>
  <h1>
      Verify that block class bugs are fixed
  </h1>
  <br/>
  <div class="first">
    <div class="visible">
      <button>VISIBLE</button>
    </div>
    <br/><br/><br/>
    <div class="rr-block" id="b-class">
      <button id="remove">BLOCKED</button>
    </div>
    <br/><br/><br/>
    <button onclick="mutate1()">MUTATE</button>
  </div>
  <br/><br/><br/>
  <div class="second">
    <div class="visible2">
      <button>VISIBLE</button>
    </div>
    <br/><br/><br/>
    <div class="rr-block" id="b-class-2">
      <button id="remove2">BLOCKED</button>
    </div>
    <br/><br/><br/>
    <button onclick="mutate2()">MUTATE</button>
  </div>
</body>

How did I test?

Ran the repl command with my fix and verified that both the new buttons appear per each click on Mutate button.

Yuyz0112 · 2022-04-03T08:42:05Z

LGTM!

Juice10 · 2022-04-03T15:06:08Z

@rahulrelicx I'd like to thank you for an excellent pull request with such a detailed description! You made it very clear as to what is going on and it's clear you really put some effort into this figuring out and coming up with a great solution.
I'm currently in the process of overhauling the whole serialization process and doing away with the __sn property and I'd hate for the edge cases you found to pop up again just because of my refactor.
So I'd like to ask you if you could turn the cases you described into tests.
You can look at the "Dom Node movement" for inspiration

rahulrelicx · 2022-04-14T08:22:39Z

@Juice10 @Yuyz0112
Please re-check my PR.
I have added an integration test and updated the isSerialized logic.

Thanks!

Juice10

LGTM, thanks for adding the test and fixing the isSerialized logic so it works with the new main branch @rahulrelicx!

Fix mutation edge case when blocked class gets unblocked

7adc4ca

rahulrelicx added 3 commits April 14, 2022 12:44

Add integration test

b798019

Rebase and merge

627d205

Update isSerialized logic

d1b0d22

Juice10 approved these changes Apr 14, 2022

View reviewed changes

Yuyz0112 merged commit 93fec1f into rrweb-io:master Apr 15, 2022

Juice10 added the 2.0 label Dec 1, 2022

This was referenced Jul 6, 2023

add sentry v1 tests getsentry/rrweb#93

Closed

feat mask all text pr getsentry/rrweb#94

Closed

feat add custom attribute transform getsentry/rrweb#96

Closed

billyvg mentioned this pull request Jul 17, 2023

feat pass element to maskTextFn getsentry/rrweb#99

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix mutation edge case when blocked class gets unblocked #867

Fix mutation edge case when blocked class gets unblocked #867

Uh oh!

rahulrelicx commented Mar 31, 2022 •

edited

Loading

Uh oh!

Yuyz0112 commented Apr 3, 2022

Uh oh!

Juice10 commented Apr 3, 2022

Uh oh!

rahulrelicx commented Apr 14, 2022

Uh oh!

Juice10 left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Fix mutation edge case when blocked class gets unblocked #867

Fix mutation edge case when blocked class gets unblocked #867

Uh oh!

Conversation

rahulrelicx commented Mar 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yuyz0112 commented Apr 3, 2022

Uh oh!

Juice10 commented Apr 3, 2022

Uh oh!

rahulrelicx commented Apr 14, 2022

Uh oh!

Juice10 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rahulrelicx commented Mar 31, 2022 •

edited

Loading

Juice10 left a comment •

edited

Loading