Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Memory Leak in Preview Component #3822

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

Memory Leak in Preview Component #3822

jvilk opened this issue Aug 30, 2017 · 6 comments
Assignees
Labels

Comments

@jvilk
Copy link

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

/claim

@claim
Copy link

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
Copy link
Contributor

Closing as the related PR got merged.

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

No branches or pull requests

4 participants