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

Error: Cannot read property 'implementation' of null #548

Closed
Juice10 opened this issue Apr 23, 2021 · 3 comments
Closed

Error: Cannot read property 'implementation' of null #548

Juice10 opened this issue Apr 23, 2021 · 3 comments

Comments

@Juice10
Copy link
Contributor

Juice10 commented Apr 23, 2021

The Error

The current master version of rrweb triggers a "Cannot read property 'implementation' of null" error in some situations.

The Bug

I did some digging and it turns out iframes inside of a DocumentFragment don't have a iframe.contentDocument.

rrweb/src/replay/index.ts

Lines 625 to 631 in 2b96a68

private attachDocumentToIframe(
mutation: addedNodeMutation,
iframeEl: HTMLIFrameElement,
) {
const collected: AppendedIframe[] = [];
buildNodeWithSN(mutation.node, {
doc: iframeEl.contentDocument!,

Whenever applyMutation gets called with useVirtualParent: true and a mutation containing an iframe, rrweb ends up calling buildNodeWithSN with an empty doc property which ends up triggering the error in buildNode on: Source

      return doc.implementation.createDocument(null, '', null);
//            ^--- null

Fixing this line and most other lines in buildNode should be quite easy, just replacing doc with document should do the trick.

For buildNodeWithSN however we need a different approach: Source

  if (n.type === NodeType.Document) {
    // close before open to make sure document was closed
    doc.close();
//   ^--- null
    doc.open();
    node = doc;
//   ^-- node is extensively used throughout buildNodeWithSN
  }

I wasn't really able to go beyond this unfortunately.

The Test (to reproduce the bug)

Add to test/replayer.test.ts:

import iframeEvents from './events/iframe-events';
  it('can fast forward past iframe changes on virtual elements', async () => {
    await this.page.evaluate(`events = ${JSON.stringify(iframeEvents)}`);
    const actionLength = await this.page.evaluate(`
      const { Replayer } = rrweb;
      const replayer = new Replayer(events);
      replayer.play(3000);
      replayer['timer']['actions'].length;
    `);
    expect(actionLength).to.equal(
      iframeEvents.filter(
        (e) => e.timestamp - iframeEvents[0].timestamp >= 3000,
      ).length,
    );
  });

Contents of test/events/iframe-events.ts: iframe-events.ts.zip

The Version

I'm using rrweb's master branch

@Yuyz0112
Copy link
Member

Yes. I have a discussion with Mark today and we will update our solution ASAP.

@Juice10
Copy link
Contributor Author

Juice10 commented Apr 23, 2021

Thanks @Yuyz0112 I tried to make it as easy as possible for someone to work on. Let me know if there is anything else I can do to help.

@Yuyz0112
Copy link
Member

@Juice10 Sure, let me explain the issue more.

In our current replay implementation, we use the "virtual parent", which is a fragment element to batch the DOM operations during apply mutations. This is a performance improvement because it reduces the amount of reflow.

The implementation is currently not quite perfect because we are moving some existing DOM nodes into the virtual parent, which causes them to drop from the document.
This causes several problems including drop stylesheet state, scroll state, and iframe content document. For the first two states, we already implement a state map to store and restore them. But for iframe content document, a map may not be a suitable solution. And we are also wondering how many potential problems are not discovered.

So @Mark-Fenng and I had an on-site discussion about the solution:

  1. Do not use virtual parent optimization if there are iframe mutations in the events. This will cause some performance regression but will be easy to add and ship.
  2. Implement a more accurate algorithm that can calculate the DOM diff without real DOM operations, like what virtual DOMs do. So we will not touch the DOM nodes that are not mutated during replay, which will keep their states better.

@Juice10 Juice10 closed this as completed Apr 28, 2021
Yuyz0112 pushed a commit that referenced this issue May 2, 2021
1. Do not use virtual parent optimization if the mutation targets have iframe elements as children. This will cause some performance regression but will be easy to add and ship.
2. If an iframe element has already been a child of a virtual parent, add the virtual parent back to the dom.
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

No branches or pull requests

2 participants