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.

Sign up for GitHub

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

Jump to bottom

Ignore firstFullSnapshot once only after initial 'poster' build #608

Merged
merged 3 commits into from
Jul 6, 2021
Merged

Ignore firstFullSnapshot once only after initial 'poster' build #608

merged 3 commits into from
Jul 6, 2021

Conversation

eoghanmurray
Copy link
Contributor

@eoghanmurray eoghanmurray commented Jun 30, 2021 •
edited
Loading

I believe the Timer was immediately started and reached the snapshot before the setTimeout returned.

Actually that probably wasn't the case, but this includes a guard against that which prevents the 'poster' build if any FullSnapshot has already been built.

@eoghanmurray
Encountered a bug where firstFullSnapshot was played twice because ti…
0bc5bc3 
…mer was immediately started and reached the snapshot before the setTimeout returned
@eoghanmurray eoghanmurray mentioned this pull request Jun 30, 2021
Closed
@eoghanmurray
Ignoring a FullSnapshot needs to be a one-time only thing, as otherwi…
ff65e4c 
…se we'll ignore it after scrubbing (restarting play head at a particular time). This is a problem if mutations have altered the player state, and we try to replay those mutations, so we e.g. try to remove an element that has already been removed because we haven't reset the FullSnapshot state
@eoghanmurray
Copy link
Contributor Author

Ok, now I've got the real fix to my issue included in the second commit.

Scenario was:

  • player built the first FullSnapshot 'poster' before starting the Timer
  • timer starts, correctly doesn't replay first FullSnapshot, but applies some destructive mutations (remove a node)
  • premature finish: playback was FINISHed as we are lazy loading events over the network
  • new events came in so we restarted the player using .play(current_time)
  • player requeued up all the already replayed events to get state up to current_time (neededEvents); however the first FullSnapshot was incorrectly ignored this time round
  • during this process, the destructive mutations were replayed, however this time they failed, as the node was no longer there to be removed.

@eoghanmurray eoghanmurray changed the title (2) Encountered a bug where firstFullSnapshot was played twice Ignore firstFullSnapshot once only after initial 'poster' build Jun 30, 2021
Juice10
Juice10 approved these changes Jul 3, 2021
View reviewed changes
Copy link
Contributor

@Juice10 Juice10 left a comment

Choose a reason for hiding this comment

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

I've been bumping in to this issue as well. I think this would solve it indeed

@Juice10
Copy link
Contributor

Juice10 commented Jul 3, 2021

I tried this PR out and it seems to solve my issues. Looks good to me!

@Yuyz0112 Yuyz0112 merged commit f99b00e into rrweb-io:master Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Juice10 Juice10 Juice10 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eoghanmurray @Juice10 @Yuyz0112