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: reset fns when when stopping record #962

Merged
merged 1 commit into from
Sep 17, 2022

Conversation

wfk007
Copy link
Contributor

@wfk007 wfk007 commented Aug 18, 2022

reset wrappedEmit and takeFullSnapshot when stopping record

record.addCustomEvent = <T>(tag: string, payload: T) => {

record.takeFullSnapshot = (isCheckout?: boolean) => {

Comment on lines +522 to +523
(wrappedEmit as unknown) = undefined;
(takeFullSnapshot as unknown) = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't reset the fns to their original state, this removes the functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't reset the fns to their original state, this removes the functions

These two fns(wrappedEmit and takeFullSnapshot ) are undefined when record is not invoked

image

In record.addCustomEvent and record.takeFullSnapshot:
Determine whether it is recording by judging whether wrappedEmit or takeFullSnapshot is falsy

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the below case:

import { record } from "rrweb"

// start record
const stopRecord = record({
  emit: xxx,
})

// stopRecord
stopRecord()

// still invoke 
record.addCustomEvent()
record.takeFullSnapshot()

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch @wfk007, thanks for finding this and clarifying!

Comment on lines +522 to +523
(wrappedEmit as unknown) = undefined;
(takeFullSnapshot as unknown) = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch @wfk007, thanks for finding this and clarifying!

@Yuyz0112 Yuyz0112 merged commit 6007266 into rrweb-io:master Sep 17, 2022
@Yuyz0112
Copy link
Member

Thanks!

@Vadman97
Copy link

Hi @wfk007 ! After this change, we've noticed that when calling the stop recording callback, observers are not immediately stopped throw the following error:
image

return () => {
handlers.forEach((h) => h());
// reset init fns when stopping record
(wrappedEmit as unknown) = undefined;
(takeFullSnapshot as unknown) = undefined;
};

Could the mousemoveCb call from the mouse / viewport observers be trying to call wrappedEmit which is set undefined by this PR?

Vadman97 added a commit to highlight/highlight that referenced this pull request Oct 18, 2022
## Summary

update to include HIG-2959 fix as well as updating rrweb fork. 
see highlight/rrweb#94 and highlight/rrweb#93

See #3200 

Fixed by reverting rrweb-io/rrweb#962 which causes problems. Will be pointing out the observers issue there.

This also cleans up some client state logic which should make sure we do not call `rrweb.addCustomEvent` which we are not recording

## How did you test this change?

Before: 
![image](https://user-images.githubusercontent.com/1351531/196545769-12c0ff1f-44a6-40a5-8457-9f224785a7e3.png)

After:
![image](https://user-images.githubusercontent.com/1351531/196560173-8b550e2e-1b4d-48c8-a00e-04f69f62197f.png)


## Are there any deployment considerations?

Bumped client version.
@wfk007
Copy link
Contributor Author

wfk007 commented Oct 25, 2022

Hi @wfk007 ! After this change, we've noticed that when calling the stop recording callback, observers are not immediately stopped throw the following error: image

return () => {
handlers.forEach((h) => h());
// reset init fns when stopping record
(wrappedEmit as unknown) = undefined;
(takeFullSnapshot as unknown) = undefined;
};

Could the mousemoveCb call from the mouse / viewport observers be trying to call wrappedEmit which is set undefined by this PR?

@Vadman97 thx for your reply. I encounterred this error too.
It is because that the MoveObserver uses double throttle to track mouseMove events
The first one is updatePosition with options trailing false
the second one is wrappedCb WITHOUT options trailing false
which means that the second throttle will be triggerred async
when stopping record, wrappedEmit will be reset to undefined, but wrappedCb will still execute because of trailing: true

@YunFeng0817
Copy link
Member

@wfk007 I encountered this error as well. Do you have any idea or plan to fix this?

@wfk007
Copy link
Contributor Author

wfk007 commented Oct 25, 2022

I have come up with three ways to fix this uncaught error

  1. rollback this PR but we can not catch record.addCustomEvent() after stopping record
  2. using wrappedCb with trailing: false but it will lose last mouseMove positions after stopping record
  3. add falsy check for wrappedEmit but it seems stupid

@Yuyz0112 @Juice10 @Mark-Fenng Hi all, any ideas for this uncaught error

@wfk007
Copy link
Contributor Author

wfk007 commented Oct 26, 2022

@wfk007 I encountered this error as well. Do you have any idea or plan to fix this?

@Mark-Fenng @Yuyz0112 @Juice10 @Vadman97 I opened a PR to fix this error: #1034

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

Successfully merging this pull request may close these issues.

5 participants