From 5f68af27f510d2e00fadf132ab927ae17d0045c6 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 20 Nov 2014 10:56:51 -0700 Subject: [PATCH 1/4] Fix memory leak in GitRepository and convert to CompositeDisposables We were calling @unsubscribe with the TextBuffer, which previously unsubscribed from that object. The problem is that we were no longer subscribing to that object directly, but only adding subscriptions to that object. This caused us to never unsubscribe from buffers. --- src/git-repository.coffee | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/git-repository.coffee b/src/git-repository.coffee index ec7ee017b..43a31e8aa 100644 --- a/src/git-repository.coffee +++ b/src/git-repository.coffee @@ -1,9 +1,8 @@ {basename, join} = require 'path' _ = require 'underscore-plus' -{Subscriber} = require 'emissary' EmitterMixin = require('emissary').Emitter -{Emitter} = require 'event-kit' +{Emitter, Disposable, CompositeDisposable} = require 'event-kit' fs = require 'fs-plus' GitUtils = require 'git-utils' {deprecate} = require 'grim' @@ -45,7 +44,6 @@ Task = require './task' module.exports = class GitRepository EmitterMixin.includeInto(this) - Subscriber.includeInto(this) @exists: (path) -> if git = @open(path) @@ -75,6 +73,8 @@ class GitRepository constructor: (path, options={}) -> @emitter = new Emitter + @subscriptions = new CompositeDisposable + @repo = GitUtils.open(path) unless @repo? throw new Error("No Git repository found searching path: #{path}") @@ -88,13 +88,15 @@ class GitRepository refreshOnWindowFocus ?= true if refreshOnWindowFocus - {$} = require './space-pen-extensions' - @subscribe $(window), 'focus', => + onWindowFocus = => @refreshIndex() @refreshStatus() + window.addEventListener 'focus', onWindowFocus + @subscriptions.add new Disposable(-> window.removeEventListener 'focus', onWindowFocus) + if @project? - @subscribe @project.eachBuffer (buffer) => @subscribeToBuffer(buffer) + @subscriptions.add @project.eachBuffer (buffer) => @subscribeToBuffer(buffer) # Public: Destroy this {GitRepository} object. # @@ -109,7 +111,7 @@ class GitRepository @repo.release() @repo = null - @unsubscribe() + @subscriptions.dispose() ### Section: Event Subscription @@ -403,10 +405,13 @@ class GitRepository if path = buffer.getPath() @getPathStatus(path) - @subscribe buffer.onDidSave(getBufferPathStatus) - @subscribe buffer.onDidReload(getBufferPathStatus) - @subscribe buffer.onDidChangePath(getBufferPathStatus) - @subscribe buffer.onDidDestroy => @unsubscribe(buffer) + bufferSubscriptions = new CompositeDisposable + bufferSubscriptions.add buffer.onDidSave(getBufferPathStatus) + bufferSubscriptions.add buffer.onDidReload(getBufferPathStatus) + bufferSubscriptions.add buffer.onDidChangePath(getBufferPathStatus) + bufferSubscriptions.add buffer.onDidDestroy => + bufferSubscriptions.dispose() + @subscriptions.remove(bufferSubscriptions) # Subscribes to editor view event. checkoutHeadForEditor: (editor) -> From 7034fe3b364120b7a6fd74ee7150305629e26aaf Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 20 Nov 2014 11:02:58 -0700 Subject: [PATCH 2/4] :arrow_up: space-pen to call cleanData in callRemoveHooks to stop leaks --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index fa83647de..e1ee9aa93 100644 --- a/package.json +++ b/package.json @@ -57,7 +57,7 @@ "season": "^1.0.2", "semver": "2.2.1", "serializable": "^1", - "space-pen": "3.8.1", + "space-pen": "3.8.2", "temp": "0.7.0", "text-buffer": "^3.6.1", "theorist": "^1.0.2", From 616a94a10ed29d9c909dc3d3a97e7c1844492284 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 20 Nov 2014 11:12:50 -0700 Subject: [PATCH 3/4] Only SpacePen callRemoveHooks on removed pane item view if destroyed --- src/pane-element.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pane-element.coffee b/src/pane-element.coffee index 417f1089e..f5d018443 100644 --- a/src/pane-element.coffee +++ b/src/pane-element.coffee @@ -95,7 +95,7 @@ class PaneElement extends HTMLElement itemRemoved: ({item, index, destroyed}) -> if viewToRemove = @model.getView(item) - callRemoveHooks(viewToRemove) + callRemoveHooks(viewToRemove) if destroyed viewToRemove.remove() paneDestroyed: -> From b5edefcae888ca0418388b68dc092a426f1b1fa5 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 20 Nov 2014 11:13:40 -0700 Subject: [PATCH 4/4] Trigger editor:will-be-removed from SpacePen shim, not component By the time the component is getting unmounted, we have already called remove hooks on the SpacePen shim so subscriptions to the event have been removed. --- src/text-editor-component.coffee | 1 - src/text-editor-view.coffee | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/text-editor-component.coffee b/src/text-editor-component.coffee index 3b50b3997..15e8e0b08 100644 --- a/src/text-editor-component.coffee +++ b/src/text-editor-component.coffee @@ -199,7 +199,6 @@ TextEditorComponent = React.createClass componentWillUnmount: -> {editor, hostElement} = @props - hostElement.__spacePenView.trigger 'editor:will-be-removed', [hostElement.__spacePenView] @unsubscribe() @scopedConfigSubscriptions.dispose() window.removeEventListener 'resize', @requestHeightAndWidthMeasurement diff --git a/src/text-editor-view.coffee b/src/text-editor-view.coffee index f27b88e76..61869ddff 100644 --- a/src/text-editor-view.coffee +++ b/src/text-editor-view.coffee @@ -138,6 +138,7 @@ class TextEditorView extends View beforeRemove: -> @trigger 'editor:detached', [this] + @trigger 'editor:will-be-removed', [this] @attached = false remove: (selector, keepData) ->