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

Memory Leak in Preview Component #3822

Closed
jvilk opened this Issue Aug 30, 2017 · 6 comments

Comments

Projects
None yet
4 participants
@jvilk

jvilk commented Aug 30, 2017

The Preview component adds four event listeners to CodeMirror that remain after it unmounts.

class Preview extends PureComponent {
  // (snip)
  componentDidMount() {
    const { codeMirror } = this.props.editor;
    const codeMirrorWrapper = codeMirror.getWrapperElement();

    codeMirror.on("scroll", this.onScroll);
    codeMirrorWrapper.addEventListener("mouseover", this._onMouseOver);
    codeMirrorWrapper.addEventListener("mouseup", this._onMouseUp);
    codeMirrorWrapper.addEventListener("mousedown", this._onMouseDown);
  }
 // (snip)
}

This leak adds around 500KB to debugger.html's heap size every time a source document is opened and closed.

Forgive me for using a competing browser for capturing the heap; it's the setup I'm currently working with!

Experiment Setup

  • Fresh installation from master (last commit: 91f5c63).
  • Running via yarn start
  • Connected to an instance of Firefox with one tab pointing to the SensorWeb debugging example.

Methodology

Perform the following tasks with an unmodified version of debugger.html, and a version of debugger.html with the leaks fixed:

  • Navigate to http://localhost:8000/?firefox-tab=child1/tab1
  • Do the following 5 times within one session:
    • Click on main.js to open it in the source viewer.
    • Once loaded, click on the x to close main.js.
    • Take a heap snapshot.
  • For each snapshot:
    • Click on the dropdown and select the statistics view.
    • Subtract System Objects from the Total; this is debugger.html's controllable heap size.
      • (As far as I can tell, System Objects correspond to internal Chrome state with no clear direct link to your application's behavior. They can add considerable noise to the graphs.)

Result

As you can see in the image below, debugger.html adds about 500KB to its heap every time main.js is opened and closed. After this leak is fixed, debugger.html grows normally (the remaining heap growth, which stabilizes after a few iterations, is likely Chrome's JIT compiler in action).

firefox_devtools

Suggested Actions

Save a reference to the used event handlers on the Preview object, and add a componentWillUnmount function that cleans up the handlers.

class Preview extends PureComponent {
  constructor() {
    super();

    const self = this;
    self.onScroll = this.onScroll.bind(this);
    self.onMouseOver = debounce(this.onMouseOver, 40);
    self._onMouseOver = e => this.onMouseOver(e);
    self._onMouseUp = e => this.onMouseUp(e);
    self._onMouseDown = e => this.onMouseDown(e);
  }

  componentDidMount() {
    const { codeMirror } = this.props.editor;
    const codeMirrorWrapper = codeMirror.getWrapperElement();

    codeMirror.on("scroll", this.onScroll);
    codeMirrorWrapper.addEventListener("mouseover", this._onMouseOver);
    codeMirrorWrapper.addEventListener("mouseup", this._onMouseUp);
    codeMirrorWrapper.addEventListener("mousedown", this._onMouseDown);
  }

  componentWillUnmount() {
    const codeMirror = this.props.editor.codeMirror;
    const codeMirrorWrapper = codeMirror.getWrapperElement();
    codeMirrorWrapper.removeEventListener("mouseover", this._onMouseOver);
    codeMirrorWrapper.removeEventListener("mouseup", this._onMouseUp);
    codeMirrorWrapper.removeEventListener("mousedown", this._onMouseDown);
    codeMirror.off("scroll", this.onScroll);
  }
}
@jasonLaster

This comment has been minimized.

Contributor

jasonLaster commented Aug 30, 2017

@jvilk that looks like a great fix. Would you like to open the PR?

Perhaps one tweak would be to reference the underlying source documents like we do here
https://github.com/devtools-html/debugger.html/blob/master/src/components/Editor/Breakpoint.js#L104-L108

Stop by and say hello in our slack
https://devtools-html-slack.herokuapp.com/

I'd love to chat source maps & debuggers with you some time.
e.g you might like this work: #3817

@jasonLaster jasonLaster added the bug label Aug 30, 2017

@jasonLaster

This comment has been minimized.

Contributor

jasonLaster commented Aug 30, 2017

Are you involved in the source map visualization project?

We're working on a product doc for some extra source map features. I'm curious to see what you think.
https://docs.google.com/document/d/1uwhgWIX9FDDGbeCqZf9LQHCl6ACK2fKQFLsy7SN4JAs/edit

@jvilk

This comment has been minimized.

jvilk commented Aug 30, 2017

I'm not directly involved, but I remember contributing ideas and maybe some code to that project. I do perform a lot of work with program transformations and source maps. I'll drop by the Slack channel to chat further.

@jasonLaster jasonLaster added this to the Backlog milestone Sep 26, 2017

@lukaszsobek

This comment has been minimized.

Contributor

lukaszsobek commented Oct 17, 2017

/claim

@claim claim bot added in progress and removed help wanted labels Oct 17, 2017

@claim

This comment has been minimized.

claim bot commented Oct 17, 2017

Thanks for claiming the issue! 👋

Here are some links for getting setup, contributing, and developing. We're always happy to answer questions in slack! If you become busy, feel free to /unclaim it.

🦊 Debugger team!

@nyrosmith

This comment has been minimized.

Contributor

nyrosmith commented Oct 30, 2017

Closing as the related PR got merged.

@nyrosmith nyrosmith closed this Oct 30, 2017

@claim claim bot removed the in progress label Oct 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment