From ef3ab03d28114064b01abdb2ab6a0c5ffcf8b8d0 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 23 May 2016 18:37:26 +0200 Subject: [PATCH 01/30] Emit {will,did}SavePath on `ipcMain` before/after a buffer is saved --- spec/project-spec.coffee | 22 ++++++++++++++++++++++ src/application-delegate.coffee | 6 ++++++ src/atom-environment.coffee | 2 +- src/project.coffee | 4 +++- src/text-editor.coffee | 2 +- 5 files changed, 33 insertions(+), 3 deletions(-) diff --git a/spec/project-spec.coffee b/spec/project-spec.coffee index 17fea736065..89a629c1e92 100644 --- a/spec/project-spec.coffee +++ b/spec/project-spec.coffee @@ -112,6 +112,28 @@ describe "Project", -> editor.saveAs(tempFile) expect(atom.project.getPaths()[0]).toBe path.dirname(tempFile) + describe "before and after saving a buffer", -> + [buffer] = [] + beforeEach -> + waitsForPromise -> + atom.project.bufferForPath(path.join(__dirname, 'fixtures', 'sample.js')).then (o) -> + buffer = o + buffer.retain() + + afterEach -> + buffer.release() + + it "emits save events on the main process", -> + spyOn(atom.project.applicationDelegate, 'emitDidSavePath') + spyOn(atom.project.applicationDelegate, 'emitWillSavePath') + + buffer.save() + + expect(atom.project.applicationDelegate.emitDidSavePath.calls.length).toBe(1) + expect(atom.project.applicationDelegate.emitDidSavePath).toHaveBeenCalledWith(buffer.getPath()) + expect(atom.project.applicationDelegate.emitWillSavePath.calls.length).toBe(1) + expect(atom.project.applicationDelegate.emitWillSavePath).toHaveBeenCalledWith(buffer.getPath()) + describe "when a watch error is thrown from the TextBuffer", -> editor = null beforeEach -> diff --git a/src/application-delegate.coffee b/src/application-delegate.coffee index aee02ee8e63..efe2af28ee6 100644 --- a/src/application-delegate.coffee +++ b/src/application-delegate.coffee @@ -266,3 +266,9 @@ class ApplicationDelegate getAutoUpdateManagerErrorMessage: -> ipcRenderer.sendSync('get-auto-update-manager-error') + + emitWillSavePath: (path) -> + ipcRenderer.send('will-save-path', path) + + emitDidSavePath: (path) -> + ipcRenderer.send('did-save-path', path) diff --git a/src/atom-environment.coffee b/src/atom-environment.coffee index 7b3edee0af0..5247ceb97d0 100644 --- a/src/atom-environment.coffee +++ b/src/atom-environment.coffee @@ -185,7 +185,7 @@ class AtomEnvironment extends Model @clipboard = new Clipboard() - @project = new Project({notificationManager: @notifications, packageManager: @packages, @config}) + @project = new Project({notificationManager: @notifications, packageManager: @packages, @config, @applicationDelegate}) @commandInstaller = new CommandInstaller(@getVersion(), @applicationDelegate) diff --git a/src/project.coffee b/src/project.coffee index bf64753cf6b..f0ce265ed60 100644 --- a/src/project.coffee +++ b/src/project.coffee @@ -21,7 +21,7 @@ class Project extends Model Section: Construction and Destruction ### - constructor: ({@notificationManager, packageManager, config}) -> + constructor: ({@notificationManager, packageManager, config, @applicationDelegate}) -> @emitter = new Emitter @buffers = [] @paths = [] @@ -390,6 +390,8 @@ class Project extends Model @on 'buffer-created', (buffer) -> callback(buffer) subscribeToBuffer: (buffer) -> + buffer.onWillSave ({path}) => @applicationDelegate.emitWillSavePath(path) + buffer.onDidSave ({path}) => @applicationDelegate.emitDidSavePath(path) buffer.onDidDestroy => @removeBuffer(buffer) buffer.onDidChangePath => unless @getPaths().length > 0 diff --git a/src/text-editor.coffee b/src/text-editor.coffee index fa52eae6d20..e054a3e3372 100644 --- a/src/text-editor.coffee +++ b/src/text-editor.coffee @@ -124,7 +124,7 @@ class TextEditor extends Model @softTabs, @firstVisibleScreenRow, @firstVisibleScreenColumn, initialLine, initialColumn, @tabLength, @softWrapped, @decorationManager, @selectionsMarkerLayer, @buffer, suppressCursorCreation, @mini, @placeholderText, lineNumberGutterVisible, @largeFileMode, @config, @clipboard, @grammarRegistry, - @assert, @applicationDelegate, grammar, @showInvisibles, @autoHeight, @scrollPastEnd, @editorWidthInChars, + @assert, grammar, @showInvisibles, @autoHeight, @scrollPastEnd, @editorWidthInChars, @tokenizedBuffer, @ignoreInvisibles, @displayLayer } = params From 049321a49863c7e603332ce5954983e8c7491151 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 23 May 2016 18:37:47 +0200 Subject: [PATCH 02/30] :bug: :fire: Remove double subscription to the same buffer --- src/project.coffee | 1 - 1 file changed, 1 deletion(-) diff --git a/src/project.coffee b/src/project.coffee index f0ce265ed60..6e567edbdb6 100644 --- a/src/project.coffee +++ b/src/project.coffee @@ -360,7 +360,6 @@ class Project extends Model addBuffer: (buffer, options={}) -> @addBufferAtIndex(buffer, @buffers.length, options) - @subscribeToBuffer(buffer) addBufferAtIndex: (buffer, index, options={}) -> @buffers.splice(index, 0, buffer) From de599f9c66b1838540cfd263754225908a237048 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 23 May 2016 18:39:23 +0200 Subject: [PATCH 03/30] Make dialog asynchronous when a renderer process crashes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …so that other event handlers have the chance to execute even if the user doesn't choose an option in the message box. This will allow us to recover files when a window crashes. --- src/browser/atom-window.coffee | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/browser/atom-window.coffee b/src/browser/atom-window.coffee index c2909f8c94f..236af463284 100644 --- a/src/browser/atom-window.coffee +++ b/src/browser/atom-window.coffee @@ -141,14 +141,15 @@ class AtomWindow @browserWindow.webContents.on 'crashed', => global.atomApplication.exit(100) if @headless - chosen = dialog.showMessageBox @browserWindow, + dialog.showMessageBox @browserWindow, type: 'warning' buttons: ['Close Window', 'Reload', 'Keep It Open'] message: 'The editor has crashed' - detail: 'Please report this issue to https://github.com/atom/atom' - switch chosen - when 0 then @browserWindow.destroy() - when 1 then @browserWindow.reload() + detail: 'Please report this issue to https://github.com/atom/atom', + (chosen) => + switch chosen + when 0 then @browserWindow.destroy() + when 1 then @browserWindow.reload() @browserWindow.webContents.on 'will-navigate', (event, url) => unless url is @browserWindow.webContents.getURL() From b58ce49d0d38381265de9e0af3a9315ccb95f3ba Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 23 May 2016 18:48:03 +0200 Subject: [PATCH 04/30] Create `FileRecoveryService` to restore corrupted files after a crash --- src/browser/atom-application.coffee | 3 ++ src/browser/file-recovery-service.js | 60 ++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) create mode 100644 src/browser/file-recovery-service.js diff --git a/src/browser/atom-application.coffee b/src/browser/atom-application.coffee index a9fa92c4b82..72a91974031 100644 --- a/src/browser/atom-application.coffee +++ b/src/browser/atom-application.coffee @@ -4,6 +4,7 @@ AtomProtocolHandler = require './atom-protocol-handler' AutoUpdateManager = require './auto-update-manager' StorageFolder = require '../storage-folder' Config = require '../config' +FileRecoveryService = require './file-recovery-service' ipcHelpers = require '../ipc-helpers' {BrowserWindow, Menu, app, dialog, ipcMain, shell} = require 'electron' fs = require 'fs-plus' @@ -78,12 +79,14 @@ class AtomApplication @autoUpdateManager = new AutoUpdateManager(@version, options.test, @resourcePath, @config) @applicationMenu = new ApplicationMenu(@version, @autoUpdateManager) @atomProtocolHandler = new AtomProtocolHandler(@resourcePath, @safeMode) + @fileRecoveryService = new FileRecoveryService(path.join(process.env.ATOM_HOME, "recovery")) @listenForArgumentsFromNewProcess() @setupJavaScriptArguments() @handleEvents() @setupDockMenu() @storageFolder = new StorageFolder(process.env.ATOM_HOME) + @fileRecoveryService.start() if options.pathsToOpen?.length > 0 or options.urlsToOpen?.length > 0 or options.test @openWithOptions(options) diff --git a/src/browser/file-recovery-service.js b/src/browser/file-recovery-service.js new file mode 100644 index 00000000000..a3f052cd485 --- /dev/null +++ b/src/browser/file-recovery-service.js @@ -0,0 +1,60 @@ +'use babel' + +import {ipcMain} from 'electron' +import crypto from 'crypto' +import Path from 'path' +import fs from 'fs-plus' + +export default class FileRecoveryService { + constructor (recoveryDirectory) { + this.recoveryDirectory = recoveryDirectory + this.recoveryPathsByWindowAndFilePath = new WeakMap + this.crashListeners = new WeakSet + } + + start () { + this.willSavePathListener = ipcMain.on('will-save-path', this.willSavePath.bind(this)) + this.didSavePathListener = ipcMain.on('did-save-path', this.didSavePath.bind(this)) + } + + willSavePath (event, path) { + if (!fs.existsSync(path)) { + // Unexisting files won't be truncated/overwritten, and so there's no data to be lost. + return + } + + const window = event.sender + const recoveryFileName = crypto.createHash('sha1').update(path + Date.now().toString(), 'utf8').digest('hex').substring(0, 10) + const recoveryPath = Path.join(this.recoveryDirectory, recoveryFileName) + fs.writeFileSync(recoveryPath, fs.readFileSync(path)) + + if (!this.recoveryPathsByWindowAndFilePath.has(window)) { + this.recoveryPathsByWindowAndFilePath.set(window, new Map) + } + this.recoveryPathsByWindowAndFilePath.get(window).set(path, recoveryPath) + + if (!this.crashListeners.has(window)) { + window.on('crashed', () => this.recoverFilesForWindow(window)) + this.crashListeners.add(window) + } + } + + didSavePath (event, path) { + const recoveryPathsByFilePath = this.recoveryPathsByWindowAndFilePath.get(event.sender) + if (recoveryPathsByFilePath.has(path)) { + const recoveryPath = recoveryPathsByFilePath.get(path) + fs.unlinkSync(recoveryPath) + recoveryPathsByFilePath.delete(path) + } + } + + recoverFilesForWindow (window) { + const recoveryPathsByFilePath = this.recoveryPathsByWindowAndFilePath.get(window) + for (let [filePath, recoveryPath] of recoveryPathsByFilePath) { + fs.writeFileSync(filePath, fs.readFileSync(recoveryPath)) + fs.unlinkSync(recoveryPath) + } + + recoveryPathsByFilePath.clear() + } +} From 770199c76cabd5a43e82706cb639a960980b7196 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 23 May 2016 19:19:48 +0200 Subject: [PATCH 05/30] :art: event.sender -> window --- src/browser/file-recovery-service.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/browser/file-recovery-service.js b/src/browser/file-recovery-service.js index a3f052cd485..6f6c123a530 100644 --- a/src/browser/file-recovery-service.js +++ b/src/browser/file-recovery-service.js @@ -40,7 +40,8 @@ export default class FileRecoveryService { } didSavePath (event, path) { - const recoveryPathsByFilePath = this.recoveryPathsByWindowAndFilePath.get(event.sender) + const window = event.sender + const recoveryPathsByFilePath = this.recoveryPathsByWindowAndFilePath.get(window) if (recoveryPathsByFilePath.has(path)) { const recoveryPath = recoveryPathsByFilePath.get(path) fs.unlinkSync(recoveryPath) From ca32c1370f99cd0fe7f686891c8a330c0a48921f Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 23 May 2016 19:25:28 +0200 Subject: [PATCH 06/30] :fire: Unnecessary variable assignments --- src/browser/file-recovery-service.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/browser/file-recovery-service.js b/src/browser/file-recovery-service.js index 6f6c123a530..559c83cee8d 100644 --- a/src/browser/file-recovery-service.js +++ b/src/browser/file-recovery-service.js @@ -13,8 +13,8 @@ export default class FileRecoveryService { } start () { - this.willSavePathListener = ipcMain.on('will-save-path', this.willSavePath.bind(this)) - this.didSavePathListener = ipcMain.on('did-save-path', this.didSavePath.bind(this)) + ipcMain.on('will-save-path', this.willSavePath.bind(this)) + ipcMain.on('did-save-path', this.didSavePath.bind(this)) } willSavePath (event, path) { From 57195d7ba632df65cdbb2362006a2ffb1b01b455 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 23 May 2016 21:10:04 +0200 Subject: [PATCH 07/30] :white_check_mark: Write specs for FileRecoveryService --- spec/browser/file-recovery-service-spec.js | 94 ++++++++++++++++++++++ src/browser/file-recovery-service.js | 4 +- 2 files changed, 96 insertions(+), 2 deletions(-) create mode 100644 spec/browser/file-recovery-service-spec.js diff --git a/spec/browser/file-recovery-service-spec.js b/spec/browser/file-recovery-service-spec.js new file mode 100644 index 00000000000..d4c048631be --- /dev/null +++ b/spec/browser/file-recovery-service-spec.js @@ -0,0 +1,94 @@ +'use babel' + +import FileRecoveryService from '../../src/browser/file-recovery-service' +import temp from 'temp' +import fs from 'fs-plus' +import path from 'path' +import os from 'os' +import crypto from 'crypto' +import {Emitter} from 'event-kit' + +describe("FileRecoveryService", () => { + let mockWindow, recoveryService, recoveryDirectory + + beforeEach(() => { + mockWindow = new Emitter + recoveryDirectory = path.join(os.tmpdir(), crypto.randomBytes(5).toString('hex')) + recoveryService = new FileRecoveryService(recoveryDirectory) + }) + + describe("when no crash happens during a save", () => { + it("creates a recovery file and deletes it after saving", () => { + let filePath = temp.path() + + fs.writeFileSync(filePath, "some content") + recoveryService.willSavePath({sender: mockWindow}, filePath) + assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) + + fs.writeFileSync(filePath, "changed") + recoveryService.didSavePath({sender: mockWindow}, filePath) + assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) + assert.equal(fs.readFileSync(filePath, 'utf8'), "changed") + }) + + it("creates many recovery files and deletes them when many windows attempt to save the same file", () => { + const anotherMockWindow = new Emitter + let filePath = temp.path() + + fs.writeFileSync(filePath, "some content") + recoveryService.willSavePath({sender: mockWindow}, filePath) + recoveryService.willSavePath({sender: anotherMockWindow}, filePath) + assert.equal(fs.listTreeSync(recoveryDirectory).length, 2) + + fs.writeFileSync(filePath, "changed") + recoveryService.didSavePath({sender: mockWindow}, filePath) + recoveryService.didSavePath({sender: anotherMockWindow}, filePath) + assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) + assert.equal(fs.readFileSync(filePath, 'utf8'), "changed") + }) + }) + + describe("when a crash happens during a save", () => { + it("restores the created recovery file and deletes it", () => { + let filePath = temp.path() + + fs.writeFileSync(filePath, "some content") + recoveryService.willSavePath({sender: mockWindow}, filePath) + assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) + + fs.writeFileSync(filePath, "changed") + mockWindow.emit("crashed") + assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) + assert.equal(fs.readFileSync(filePath, 'utf8'), "some content") + }) + + it("restores the created recovery files and deletes them in the order in which windows crash", () => { + const anotherMockWindow = new Emitter + let filePath = temp.path() + + fs.writeFileSync(filePath, "window 1") + recoveryService.willSavePath({sender: mockWindow}, filePath) + fs.writeFileSync(filePath, "window 2") + recoveryService.willSavePath({sender: anotherMockWindow}, filePath) + assert.equal(fs.listTreeSync(recoveryDirectory).length, 2) + + fs.writeFileSync(filePath, "changed") + + mockWindow.emit("crashed") + assert.equal(fs.readFileSync(filePath, 'utf8'), "window 1") + assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) + + anotherMockWindow.emit("crashed") + assert.equal(fs.readFileSync(filePath, 'utf8'), "window 2") + assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) + }) + }) + + it("doesn't create a recovery file when the file that's being saved doesn't exist yet", () => { + recoveryService.willSavePath({sender: mockWindow}, "a-file-that-doesnt-exist") + assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) + + recoveryService.didSavePath({sender: mockWindow}, "a-file-that-doesnt-exist") + assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) + }) +}) diff --git a/src/browser/file-recovery-service.js b/src/browser/file-recovery-service.js index 559c83cee8d..89aca30fd2b 100644 --- a/src/browser/file-recovery-service.js +++ b/src/browser/file-recovery-service.js @@ -24,7 +24,7 @@ export default class FileRecoveryService { } const window = event.sender - const recoveryFileName = crypto.createHash('sha1').update(path + Date.now().toString(), 'utf8').digest('hex').substring(0, 10) + const recoveryFileName = crypto.randomBytes(5).toString('hex') const recoveryPath = Path.join(this.recoveryDirectory, recoveryFileName) fs.writeFileSync(recoveryPath, fs.readFileSync(path)) @@ -42,7 +42,7 @@ export default class FileRecoveryService { didSavePath (event, path) { const window = event.sender const recoveryPathsByFilePath = this.recoveryPathsByWindowAndFilePath.get(window) - if (recoveryPathsByFilePath.has(path)) { + if (recoveryPathsByFilePath != null && recoveryPathsByFilePath.has(path)) { const recoveryPath = recoveryPathsByFilePath.get(path) fs.unlinkSync(recoveryPath) recoveryPathsByFilePath.delete(path) From 97dd25d14f94d84fb524b5850bf61875b3eaddf1 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Tue, 24 May 2016 08:10:48 +0200 Subject: [PATCH 08/30] :green_heart: --- spec/git-repository-async-spec.js | 2 +- spec/git-spec.coffee | 2 +- spec/workspace-spec.coffee | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/git-repository-async-spec.js b/spec/git-repository-async-spec.js index fcb528819f9..8b824bab307 100644 --- a/spec/git-repository-async-spec.js +++ b/spec/git-repository-async-spec.js @@ -583,7 +583,7 @@ describe('GitRepositoryAsync', () => { it('subscribes to all the serialized buffers in the project', async () => { await atom.workspace.open('file.txt') - project2 = new Project({notificationManager: atom.notifications, packageManager: atom.packages, confirm: atom.confirm}) + project2 = new Project({notificationManager: atom.notifications, packageManager: atom.packages, confirm: atom.confirm, applicationDelegate: atom.applicationDelegate}) project2.deserialize(atom.project.serialize({isUnloading: true})) const repo = project2.getRepositories()[0].async diff --git a/spec/git-spec.coffee b/spec/git-spec.coffee index 82e37114624..a9de506a2a3 100644 --- a/spec/git-spec.coffee +++ b/spec/git-spec.coffee @@ -356,7 +356,7 @@ describe "GitRepository", -> atom.workspace.open('file.txt') runs -> - project2 = new Project({notificationManager: atom.notifications, packageManager: atom.packages, confirm: atom.confirm}) + project2 = new Project({notificationManager: atom.notifications, packageManager: atom.packages, confirm: atom.confirm, applicationDelegate: atom.applicationDelegate}) project2.deserialize(atom.project.serialize({isUnloading: false})) buffer = project2.getBuffers()[0] diff --git a/spec/workspace-spec.coffee b/spec/workspace-spec.coffee index f08e8555872..f1911e55737 100644 --- a/spec/workspace-spec.coffee +++ b/spec/workspace-spec.coffee @@ -25,7 +25,7 @@ describe "Workspace", -> projectState = atom.project.serialize({isUnloading: true}) atom.workspace.destroy() atom.project.destroy() - atom.project = new Project({notificationManager: atom.notifications, packageManager: atom.packages, confirm: atom.confirm.bind(atom)}) + atom.project = new Project({notificationManager: atom.notifications, packageManager: atom.packages, confirm: atom.confirm.bind(atom), applicationDelegate: atom.applicationDelegate}) atom.project.deserialize(projectState) atom.workspace = new Workspace({ config: atom.config, project: atom.project, packageManager: atom.packages, From 25fece4a1a01ebca3527b0004381501aab713992 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Tue, 24 May 2016 08:24:41 +0200 Subject: [PATCH 09/30] :art: file-recovery-service-spec.js -> file-recovery-service.spec.js --- ...ecovery-service-spec.js => file-recovery-service.spec.js} | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) rename spec/browser/{file-recovery-service-spec.js => file-recovery-service.spec.js} (95%) diff --git a/spec/browser/file-recovery-service-spec.js b/spec/browser/file-recovery-service.spec.js similarity index 95% rename from spec/browser/file-recovery-service-spec.js rename to spec/browser/file-recovery-service.spec.js index d4c048631be..6feabb8dccc 100644 --- a/spec/browser/file-recovery-service-spec.js +++ b/spec/browser/file-recovery-service.spec.js @@ -13,7 +13,7 @@ describe("FileRecoveryService", () => { beforeEach(() => { mockWindow = new Emitter - recoveryDirectory = path.join(os.tmpdir(), crypto.randomBytes(5).toString('hex')) + recoveryDirectory = temp.mkdirSync() recoveryService = new FileRecoveryService(recoveryDirectory) }) @@ -42,6 +42,9 @@ describe("FileRecoveryService", () => { fs.writeFileSync(filePath, "changed") recoveryService.didSavePath({sender: mockWindow}, filePath) + assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) + assert.equal(fs.readFileSync(filePath, 'utf8'), "changed") + recoveryService.didSavePath({sender: anotherMockWindow}, filePath) assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) assert.equal(fs.readFileSync(filePath, 'utf8'), "changed") From 858740cd5f80741d68ab17330aafd70db0b74c56 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Tue, 24 May 2016 08:32:29 +0200 Subject: [PATCH 10/30] :memo: Better description on spec --- spec/browser/file-recovery-service.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/browser/file-recovery-service.spec.js b/spec/browser/file-recovery-service.spec.js index 6feabb8dccc..87b0c46a047 100644 --- a/spec/browser/file-recovery-service.spec.js +++ b/spec/browser/file-recovery-service.spec.js @@ -65,7 +65,7 @@ describe("FileRecoveryService", () => { assert.equal(fs.readFileSync(filePath, 'utf8'), "some content") }) - it("restores the created recovery files and deletes them in the order in which windows crash", () => { + it("restores the created recovery files and deletes them in the order in which windows crash when they attempt to save the same file", () => { const anotherMockWindow = new Emitter let filePath = temp.path() From c7f4b33eb80fdeb122a227d93675d22a1b56b7c2 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Tue, 24 May 2016 09:04:06 +0200 Subject: [PATCH 11/30] Emit informative warning when a file can't be recovered --- spec/browser/file-recovery-service.spec.js | 18 ++++++++++++++++++ src/browser/file-recovery-service.js | 8 ++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/spec/browser/file-recovery-service.spec.js b/spec/browser/file-recovery-service.spec.js index 87b0c46a047..2d55358a944 100644 --- a/spec/browser/file-recovery-service.spec.js +++ b/spec/browser/file-recovery-service.spec.js @@ -85,6 +85,24 @@ describe("FileRecoveryService", () => { assert.equal(fs.readFileSync(filePath, 'utf8'), "window 2") assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) }) + + it("emits a warning when a file can't be recovered", () => { + let filePath = temp.path() + fs.writeFileSync(filePath, "content") + fs.chmodSync(filePath, 0444) + + const previousConsoleLog = console.log + let logs = [] + console.log = (message) => logs.push(message) + + recoveryService.willSavePath({sender: mockWindow}, filePath) + mockWindow.emit("crashed") + let recoveryFiles = fs.listTreeSync(recoveryDirectory) + assert.equal(recoveryFiles.length, 1) + assert.deepEqual(logs, [`Cannot recover ${filePath}. A recovery file has been saved here: ${recoveryFiles[0]}.`]) + + console.log = previousConsoleLog + }) }) it("doesn't create a recovery file when the file that's being saved doesn't exist yet", () => { diff --git a/src/browser/file-recovery-service.js b/src/browser/file-recovery-service.js index 89aca30fd2b..45e2652e885 100644 --- a/src/browser/file-recovery-service.js +++ b/src/browser/file-recovery-service.js @@ -52,8 +52,12 @@ export default class FileRecoveryService { recoverFilesForWindow (window) { const recoveryPathsByFilePath = this.recoveryPathsByWindowAndFilePath.get(window) for (let [filePath, recoveryPath] of recoveryPathsByFilePath) { - fs.writeFileSync(filePath, fs.readFileSync(recoveryPath)) - fs.unlinkSync(recoveryPath) + try { + fs.writeFileSync(filePath, fs.readFileSync(recoveryPath)) + fs.unlinkSync(recoveryPath) + } catch (error) { + console.log(`Cannot recover ${filePath}. A recovery file has been saved here: ${recoveryPath}.`) + } } recoveryPathsByFilePath.clear() From 4f1efe6ef69854942c03cddc00a3d1421dc674ca Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Tue, 24 May 2016 09:28:00 +0200 Subject: [PATCH 12/30] :shirt: Fix linter errors --- spec/browser/file-recovery-service.spec.js | 4 ++-- src/browser/file-recovery-service.js | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/browser/file-recovery-service.spec.js b/spec/browser/file-recovery-service.spec.js index 2d55358a944..0849a9b4ba6 100644 --- a/spec/browser/file-recovery-service.spec.js +++ b/spec/browser/file-recovery-service.spec.js @@ -12,7 +12,7 @@ describe("FileRecoveryService", () => { let mockWindow, recoveryService, recoveryDirectory beforeEach(() => { - mockWindow = new Emitter + mockWindow = new Emitter() recoveryDirectory = temp.mkdirSync() recoveryService = new FileRecoveryService(recoveryDirectory) }) @@ -32,7 +32,7 @@ describe("FileRecoveryService", () => { }) it("creates many recovery files and deletes them when many windows attempt to save the same file", () => { - const anotherMockWindow = new Emitter + const anotherMockWindow = new Emitter() let filePath = temp.path() fs.writeFileSync(filePath, "some content") diff --git a/src/browser/file-recovery-service.js b/src/browser/file-recovery-service.js index 45e2652e885..b7537937b6a 100644 --- a/src/browser/file-recovery-service.js +++ b/src/browser/file-recovery-service.js @@ -8,8 +8,8 @@ import fs from 'fs-plus' export default class FileRecoveryService { constructor (recoveryDirectory) { this.recoveryDirectory = recoveryDirectory - this.recoveryPathsByWindowAndFilePath = new WeakMap - this.crashListeners = new WeakSet + this.recoveryPathsByWindowAndFilePath = new WeakMap() + this.crashListeners = new WeakSet() } start () { @@ -29,7 +29,7 @@ export default class FileRecoveryService { fs.writeFileSync(recoveryPath, fs.readFileSync(path)) if (!this.recoveryPathsByWindowAndFilePath.has(window)) { - this.recoveryPathsByWindowAndFilePath.set(window, new Map) + this.recoveryPathsByWindowAndFilePath.set(window, new Map()) } this.recoveryPathsByWindowAndFilePath.get(window).set(path, recoveryPath) From 7a12984af3a62ad0e75d80f9232d68d7ef9495ca Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Tue, 24 May 2016 09:28:58 +0200 Subject: [PATCH 13/30] :fire: Unused requires on specs --- spec/browser/file-recovery-service.spec.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/spec/browser/file-recovery-service.spec.js b/spec/browser/file-recovery-service.spec.js index 0849a9b4ba6..75fbe316671 100644 --- a/spec/browser/file-recovery-service.spec.js +++ b/spec/browser/file-recovery-service.spec.js @@ -3,9 +3,6 @@ import FileRecoveryService from '../../src/browser/file-recovery-service' import temp from 'temp' import fs from 'fs-plus' -import path from 'path' -import os from 'os' -import crypto from 'crypto' import {Emitter} from 'event-kit' describe("FileRecoveryService", () => { From b84feeb853426b175ca8b4cdceddb6dc2cc72ebf Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 25 May 2016 00:11:30 +0200 Subject: [PATCH 14/30] Emit {will,did}SavePath events synchronously --- src/application-delegate.coffee | 4 ++-- src/browser/file-recovery-service.js | 15 +++++++++++---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/application-delegate.coffee b/src/application-delegate.coffee index efe2af28ee6..74c498e78e8 100644 --- a/src/application-delegate.coffee +++ b/src/application-delegate.coffee @@ -268,7 +268,7 @@ class ApplicationDelegate ipcRenderer.sendSync('get-auto-update-manager-error') emitWillSavePath: (path) -> - ipcRenderer.send('will-save-path', path) + ipcRenderer.sendSync('will-save-path', path) emitDidSavePath: (path) -> - ipcRenderer.send('did-save-path', path) + ipcRenderer.sendSync('did-save-path', path) diff --git a/src/browser/file-recovery-service.js b/src/browser/file-recovery-service.js index b7537937b6a..56ebd8067df 100644 --- a/src/browser/file-recovery-service.js +++ b/src/browser/file-recovery-service.js @@ -20,6 +20,7 @@ export default class FileRecoveryService { willSavePath (event, path) { if (!fs.existsSync(path)) { // Unexisting files won't be truncated/overwritten, and so there's no data to be lost. + event.returnValue = false return } @@ -37,16 +38,22 @@ export default class FileRecoveryService { window.on('crashed', () => this.recoverFilesForWindow(window)) this.crashListeners.add(window) } + + event.returnValue = true } didSavePath (event, path) { const window = event.sender const recoveryPathsByFilePath = this.recoveryPathsByWindowAndFilePath.get(window) - if (recoveryPathsByFilePath != null && recoveryPathsByFilePath.has(path)) { - const recoveryPath = recoveryPathsByFilePath.get(path) - fs.unlinkSync(recoveryPath) - recoveryPathsByFilePath.delete(path) + if (recoveryPathsByFilePath == null || !recoveryPathsByFilePath.has(path)) { + event.returnValue = false + return } + + const recoveryPath = recoveryPathsByFilePath.get(path) + fs.unlinkSync(recoveryPath) + recoveryPathsByFilePath.delete(path) + event.returnValue = true } recoverFilesForWindow (window) { From 3b4c1015cc2674676dbc340cfbafc3a437614fc6 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 25 May 2016 11:48:41 +0200 Subject: [PATCH 15/30] Forget window when it gets closed --- .../file-recovery-service.spec.js | 72 ++++++++++++------- .../file-recovery-service.js | 18 +++-- 2 files changed, 57 insertions(+), 33 deletions(-) rename spec/{browser => main-process}/file-recovery-service.spec.js (59%) rename src/{browser => main-process}/file-recovery-service.js (79%) diff --git a/spec/browser/file-recovery-service.spec.js b/spec/main-process/file-recovery-service.spec.js similarity index 59% rename from spec/browser/file-recovery-service.spec.js rename to spec/main-process/file-recovery-service.spec.js index 75fbe316671..7b60a9d7d74 100644 --- a/spec/browser/file-recovery-service.spec.js +++ b/spec/main-process/file-recovery-service.spec.js @@ -1,48 +1,63 @@ 'use babel' -import FileRecoveryService from '../../src/browser/file-recovery-service' +import {BrowserWindow} from 'electron' +import FileRecoveryService from '../../src/main-process/file-recovery-service' import temp from 'temp' import fs from 'fs-plus' import {Emitter} from 'event-kit' describe("FileRecoveryService", () => { - let mockWindow, recoveryService, recoveryDirectory + let recoveryService, recoveryDirectory, windows + + function createWindow () { + const window = new BrowserWindow({show: false}) + windows.push(window) + return window + } beforeEach(() => { - mockWindow = new Emitter() + windows = [] recoveryDirectory = temp.mkdirSync() recoveryService = new FileRecoveryService(recoveryDirectory) }) + afterEach(() => { + for (let window of windows) { + window.destroy() + } + }) + describe("when no crash happens during a save", () => { it("creates a recovery file and deletes it after saving", () => { - let filePath = temp.path() + const mockWindow = createWindow() + const filePath = temp.path() fs.writeFileSync(filePath, "some content") - recoveryService.willSavePath({sender: mockWindow}, filePath) + recoveryService.willSavePath({sender: mockWindow.webContents}, filePath) assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) fs.writeFileSync(filePath, "changed") - recoveryService.didSavePath({sender: mockWindow}, filePath) + recoveryService.didSavePath({sender: mockWindow.webContents}, filePath) assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) assert.equal(fs.readFileSync(filePath, 'utf8'), "changed") }) it("creates many recovery files and deletes them when many windows attempt to save the same file", () => { - const anotherMockWindow = new Emitter() - let filePath = temp.path() + const mockWindow = createWindow() + const anotherMockWindow = createWindow() + const filePath = temp.path() fs.writeFileSync(filePath, "some content") - recoveryService.willSavePath({sender: mockWindow}, filePath) - recoveryService.willSavePath({sender: anotherMockWindow}, filePath) + recoveryService.willSavePath({sender: mockWindow.webContents}, filePath) + recoveryService.willSavePath({sender: anotherMockWindow.webContents}, filePath) assert.equal(fs.listTreeSync(recoveryDirectory).length, 2) fs.writeFileSync(filePath, "changed") - recoveryService.didSavePath({sender: mockWindow}, filePath) + recoveryService.didSavePath({sender: mockWindow.webContents}, filePath) assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) assert.equal(fs.readFileSync(filePath, 'utf8'), "changed") - recoveryService.didSavePath({sender: anotherMockWindow}, filePath) + recoveryService.didSavePath({sender: anotherMockWindow.webContents}, filePath) assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) assert.equal(fs.readFileSync(filePath, 'utf8'), "changed") }) @@ -50,41 +65,44 @@ describe("FileRecoveryService", () => { describe("when a crash happens during a save", () => { it("restores the created recovery file and deletes it", () => { - let filePath = temp.path() + const mockWindow = createWindow() + const filePath = temp.path() fs.writeFileSync(filePath, "some content") - recoveryService.willSavePath({sender: mockWindow}, filePath) + recoveryService.willSavePath({sender: mockWindow.webContents}, filePath) assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) fs.writeFileSync(filePath, "changed") - mockWindow.emit("crashed") + mockWindow.webContents.emit("crashed") assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) assert.equal(fs.readFileSync(filePath, 'utf8'), "some content") }) it("restores the created recovery files and deletes them in the order in which windows crash when they attempt to save the same file", () => { - const anotherMockWindow = new Emitter - let filePath = temp.path() + const mockWindow = createWindow() + const anotherMockWindow = createWindow() + const filePath = temp.path() fs.writeFileSync(filePath, "window 1") - recoveryService.willSavePath({sender: mockWindow}, filePath) + recoveryService.willSavePath({sender: mockWindow.webContents}, filePath) fs.writeFileSync(filePath, "window 2") - recoveryService.willSavePath({sender: anotherMockWindow}, filePath) + recoveryService.willSavePath({sender: anotherMockWindow.webContents}, filePath) assert.equal(fs.listTreeSync(recoveryDirectory).length, 2) fs.writeFileSync(filePath, "changed") - mockWindow.emit("crashed") + mockWindow.webContents.emit("crashed") assert.equal(fs.readFileSync(filePath, 'utf8'), "window 1") assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) - anotherMockWindow.emit("crashed") + anotherMockWindow.webContents.emit("crashed") assert.equal(fs.readFileSync(filePath, 'utf8'), "window 2") assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) }) it("emits a warning when a file can't be recovered", () => { - let filePath = temp.path() + const mockWindow = createWindow() + const filePath = temp.path() fs.writeFileSync(filePath, "content") fs.chmodSync(filePath, 0444) @@ -92,8 +110,8 @@ describe("FileRecoveryService", () => { let logs = [] console.log = (message) => logs.push(message) - recoveryService.willSavePath({sender: mockWindow}, filePath) - mockWindow.emit("crashed") + recoveryService.willSavePath({sender: mockWindow.webContents}, filePath) + mockWindow.webContents.emit("crashed") let recoveryFiles = fs.listTreeSync(recoveryDirectory) assert.equal(recoveryFiles.length, 1) assert.deepEqual(logs, [`Cannot recover ${filePath}. A recovery file has been saved here: ${recoveryFiles[0]}.`]) @@ -103,10 +121,12 @@ describe("FileRecoveryService", () => { }) it("doesn't create a recovery file when the file that's being saved doesn't exist yet", () => { - recoveryService.willSavePath({sender: mockWindow}, "a-file-that-doesnt-exist") + const mockWindow = createWindow() + + recoveryService.willSavePath({sender: mockWindow.webContents}, "a-file-that-doesnt-exist") assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) - recoveryService.didSavePath({sender: mockWindow}, "a-file-that-doesnt-exist") + recoveryService.didSavePath({sender: mockWindow.webContents}, "a-file-that-doesnt-exist") assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) }) }) diff --git a/src/browser/file-recovery-service.js b/src/main-process/file-recovery-service.js similarity index 79% rename from src/browser/file-recovery-service.js rename to src/main-process/file-recovery-service.js index 56ebd8067df..56cc5e2fe18 100644 --- a/src/browser/file-recovery-service.js +++ b/src/main-process/file-recovery-service.js @@ -1,6 +1,6 @@ 'use babel' -import {ipcMain} from 'electron' +import {BrowserWindow, ipcMain} from 'electron' import crypto from 'crypto' import Path from 'path' import fs from 'fs-plus' @@ -9,7 +9,7 @@ export default class FileRecoveryService { constructor (recoveryDirectory) { this.recoveryDirectory = recoveryDirectory this.recoveryPathsByWindowAndFilePath = new WeakMap() - this.crashListeners = new WeakSet() + this.observedWindows = new WeakSet() } start () { @@ -24,7 +24,7 @@ export default class FileRecoveryService { return } - const window = event.sender + const window = BrowserWindow.fromWebContents(event.sender) const recoveryFileName = crypto.randomBytes(5).toString('hex') const recoveryPath = Path.join(this.recoveryDirectory, recoveryFileName) fs.writeFileSync(recoveryPath, fs.readFileSync(path)) @@ -34,16 +34,20 @@ export default class FileRecoveryService { } this.recoveryPathsByWindowAndFilePath.get(window).set(path, recoveryPath) - if (!this.crashListeners.has(window)) { - window.on('crashed', () => this.recoverFilesForWindow(window)) - this.crashListeners.add(window) + if (!this.observedWindows.has(window)) { + window.webContents.on("crashed", () => this.recoverFilesForWindow(window)) + window.on("closed", () => { + this.observedWindows.delete(window) + this.recoveryPathsByWindowAndFilePath.delete(window) + }) + this.observedWindows.add(window) } event.returnValue = true } didSavePath (event, path) { - const window = event.sender + const window = BrowserWindow.fromWebContents(event.sender) const recoveryPathsByFilePath = this.recoveryPathsByWindowAndFilePath.get(window) if (recoveryPathsByFilePath == null || !recoveryPathsByFilePath.has(path)) { event.returnValue = false From c8fae110e704cc08d4fe3146e1260878e261508d Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 25 May 2016 14:01:36 +0200 Subject: [PATCH 16/30] Handle recovery when many windows save the same file simultaneously --- .../file-recovery-service.spec.js | 60 ++++++++----- src/main-process/file-recovery-service.js | 85 ++++++++++++++----- 2 files changed, 100 insertions(+), 45 deletions(-) diff --git a/spec/main-process/file-recovery-service.spec.js b/spec/main-process/file-recovery-service.spec.js index 7b60a9d7d74..1cc0062f16e 100644 --- a/spec/main-process/file-recovery-service.spec.js +++ b/spec/main-process/file-recovery-service.spec.js @@ -42,7 +42,7 @@ describe("FileRecoveryService", () => { assert.equal(fs.readFileSync(filePath, 'utf8'), "changed") }) - it("creates many recovery files and deletes them when many windows attempt to save the same file", () => { + it("creates only one recovery file when many windows attempt to save the same file, deleting it when the last one finishes saving it", () => { const mockWindow = createWindow() const anotherMockWindow = createWindow() const filePath = temp.path() @@ -50,7 +50,7 @@ describe("FileRecoveryService", () => { fs.writeFileSync(filePath, "some content") recoveryService.willSavePath({sender: mockWindow.webContents}, filePath) recoveryService.willSavePath({sender: anotherMockWindow.webContents}, filePath) - assert.equal(fs.listTreeSync(recoveryDirectory).length, 2) + assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) fs.writeFileSync(filePath, "changed") recoveryService.didSavePath({sender: mockWindow.webContents}, filePath) @@ -78,26 +78,42 @@ describe("FileRecoveryService", () => { assert.equal(fs.readFileSync(filePath, 'utf8'), "some content") }) - it("restores the created recovery files and deletes them in the order in which windows crash when they attempt to save the same file", () => { - const mockWindow = createWindow() - const anotherMockWindow = createWindow() - const filePath = temp.path() - - fs.writeFileSync(filePath, "window 1") - recoveryService.willSavePath({sender: mockWindow.webContents}, filePath) - fs.writeFileSync(filePath, "window 2") - recoveryService.willSavePath({sender: anotherMockWindow.webContents}, filePath) - assert.equal(fs.listTreeSync(recoveryDirectory).length, 2) - - fs.writeFileSync(filePath, "changed") - - mockWindow.webContents.emit("crashed") - assert.equal(fs.readFileSync(filePath, 'utf8'), "window 1") - assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) - - anotherMockWindow.webContents.emit("crashed") - assert.equal(fs.readFileSync(filePath, 'utf8'), "window 2") - assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) + describe("when many windows attempt to save the same file", () => { + it("recovers the file when the window that initiated the save crashes", () => { + const mockWindow = createWindow() + const anotherMockWindow = createWindow() + const filePath = temp.path() + + fs.writeFileSync(filePath, "window 1") + recoveryService.willSavePath({sender: mockWindow.webContents}, filePath) + fs.writeFileSync(filePath, "window 2") + recoveryService.willSavePath({sender: anotherMockWindow.webContents}, filePath) + assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) + + fs.writeFileSync(filePath, "changed") + + mockWindow.webContents.emit("crashed") + assert.equal(fs.readFileSync(filePath, 'utf8'), "window 1") + assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) + }) + + it("recovers the file when a window that did not initiate the save crashes", () => { + const mockWindow = createWindow() + const anotherMockWindow = createWindow() + const filePath = temp.path() + + fs.writeFileSync(filePath, "window 1") + recoveryService.willSavePath({sender: mockWindow.webContents}, filePath) + fs.writeFileSync(filePath, "window 2") + recoveryService.willSavePath({sender: anotherMockWindow.webContents}, filePath) + assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) + + fs.writeFileSync(filePath, "changed") + + anotherMockWindow.webContents.emit("crashed") + assert.equal(fs.readFileSync(filePath, 'utf8'), "window 1") + assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) + }) }) it("emits a warning when a file can't be recovered", () => { diff --git a/src/main-process/file-recovery-service.js b/src/main-process/file-recovery-service.js index 56cc5e2fe18..71ae6200d4e 100644 --- a/src/main-process/file-recovery-service.js +++ b/src/main-process/file-recovery-service.js @@ -5,10 +5,47 @@ import crypto from 'crypto' import Path from 'path' import fs from 'fs-plus' +class RecoveryFile { + constructor (originalPath, recoveryPath) { + this.originalPath = originalPath + this.recoveryPath = recoveryPath + this.refCount = 0 + } + + storeSync () { + fs.writeFileSync(this.recoveryPath, fs.readFileSync(this.originalPath)) + } + + recoverSync () { + fs.writeFileSync(this.originalPath, fs.readFileSync(this.recoveryPath)) + this.removeSync() + this.refCount = 0 + } + + removeSync () { + fs.unlinkSync(this.recoveryPath) + } + + retain () { + if (this.refCount === 0) this.storeSync() + this.refCount++ + } + + release () { + this.refCount-- + if (this.refCount === 0) this.removeSync() + } + + isReleased () { + return this.refCount === 0 + } +} + export default class FileRecoveryService { constructor (recoveryDirectory) { this.recoveryDirectory = recoveryDirectory - this.recoveryPathsByWindowAndFilePath = new WeakMap() + this.recoveryFilesByFilePath = new Map() + this.recoveryFilesByWindow = new WeakMap() this.observedWindows = new WeakSet() } @@ -25,22 +62,24 @@ export default class FileRecoveryService { } const window = BrowserWindow.fromWebContents(event.sender) - const recoveryFileName = crypto.randomBytes(5).toString('hex') - const recoveryPath = Path.join(this.recoveryDirectory, recoveryFileName) - fs.writeFileSync(recoveryPath, fs.readFileSync(path)) - - if (!this.recoveryPathsByWindowAndFilePath.has(window)) { - this.recoveryPathsByWindowAndFilePath.set(window, new Map()) + let recoveryFile = this.recoveryFilesByFilePath.get(path) + if (recoveryFile == null) { + const recoveryPath = Path.join(this.recoveryDirectory, crypto.randomBytes(5).toString('hex')) + recoveryFile = new RecoveryFile(path, recoveryPath) + this.recoveryFilesByFilePath.set(path, recoveryFile) } - this.recoveryPathsByWindowAndFilePath.get(window).set(path, recoveryPath) + recoveryFile.retain() + + if (!this.recoveryFilesByWindow.has(window)) this.recoveryFilesByWindow.set(window, new Set()) + this.recoveryFilesByWindow.get(window).add(recoveryFile) if (!this.observedWindows.has(window)) { + this.observedWindows.add(window) window.webContents.on("crashed", () => this.recoverFilesForWindow(window)) window.on("closed", () => { this.observedWindows.delete(window) - this.recoveryPathsByWindowAndFilePath.delete(window) + this.recoveryFilesByWindow.delete(window) }) - this.observedWindows.add(window) } event.returnValue = true @@ -48,29 +87,29 @@ export default class FileRecoveryService { didSavePath (event, path) { const window = BrowserWindow.fromWebContents(event.sender) - const recoveryPathsByFilePath = this.recoveryPathsByWindowAndFilePath.get(window) - if (recoveryPathsByFilePath == null || !recoveryPathsByFilePath.has(path)) { - event.returnValue = false - return + const recoveryFile = this.recoveryFilesByFilePath.get(path) + if (recoveryFile != null) { + recoveryFile.release() + if (recoveryFile.isReleased()) this.recoveryFilesByFilePath.delete(path) + this.recoveryFilesByWindow.get(window).delete(recoveryFile) } - const recoveryPath = recoveryPathsByFilePath.get(path) - fs.unlinkSync(recoveryPath) - recoveryPathsByFilePath.delete(path) event.returnValue = true } recoverFilesForWindow (window) { - const recoveryPathsByFilePath = this.recoveryPathsByWindowAndFilePath.get(window) - for (let [filePath, recoveryPath] of recoveryPathsByFilePath) { + if (!this.recoveryFilesByWindow.has(window)) return + + for (const recoveryFile of this.recoveryFilesByWindow.get(window)) { try { - fs.writeFileSync(filePath, fs.readFileSync(recoveryPath)) - fs.unlinkSync(recoveryPath) + recoveryFile.recoverSync() } catch (error) { - console.log(`Cannot recover ${filePath}. A recovery file has been saved here: ${recoveryPath}.`) + console.log(`Cannot recover ${recoveryFile.originalPath}. A recovery file has been saved here: ${recoveryFile.recoveryPath}.`) + } finally { + this.recoveryFilesByFilePath.delete(recoveryFile.originalPath) } } - recoveryPathsByFilePath.clear() + this.recoveryFilesByWindow.delete(window) } } From 30307231e63a57d8bedac103be0ad88d4e33e95f Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 25 May 2016 14:05:09 +0200 Subject: [PATCH 17/30] :shirt: Fix linting issues --- src/main-process/file-recovery-service.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main-process/file-recovery-service.js b/src/main-process/file-recovery-service.js index 71ae6200d4e..16e3cde38fd 100644 --- a/src/main-process/file-recovery-service.js +++ b/src/main-process/file-recovery-service.js @@ -75,8 +75,8 @@ export default class FileRecoveryService { if (!this.observedWindows.has(window)) { this.observedWindows.add(window) - window.webContents.on("crashed", () => this.recoverFilesForWindow(window)) - window.on("closed", () => { + window.webContents.on('crashed', () => this.recoverFilesForWindow(window)) + window.on('closed', () => { this.observedWindows.delete(window) this.recoveryFilesByWindow.delete(window) }) From a2a734a37226728067947b84cc7a07163e733d65 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 25 May 2016 14:19:39 +0200 Subject: [PATCH 18/30] Generate readable recovery filenames --- src/main-process/file-recovery-service.js | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/main-process/file-recovery-service.js b/src/main-process/file-recovery-service.js index 16e3cde38fd..89106a6103c 100644 --- a/src/main-process/file-recovery-service.js +++ b/src/main-process/file-recovery-service.js @@ -6,6 +6,13 @@ import Path from 'path' import fs from 'fs-plus' class RecoveryFile { + static fileNameForPath (path) { + const extension = Path.extname(path) + const basename = Path.basename(path, extension).substring(0, 34) + const randomSuffix = crypto.randomBytes(3).toString('hex') + return `${basename}-${randomSuffix}${extension}` + } + constructor (originalPath, recoveryPath) { this.originalPath = originalPath this.recoveryPath = recoveryPath @@ -64,8 +71,10 @@ export default class FileRecoveryService { const window = BrowserWindow.fromWebContents(event.sender) let recoveryFile = this.recoveryFilesByFilePath.get(path) if (recoveryFile == null) { - const recoveryPath = Path.join(this.recoveryDirectory, crypto.randomBytes(5).toString('hex')) - recoveryFile = new RecoveryFile(path, recoveryPath) + recoveryFile = new RecoveryFile( + path, + Path.join(this.recoveryDirectory, RecoveryFile.fileNameForPath(path)) + ) this.recoveryFilesByFilePath.set(path, recoveryFile) } recoveryFile.retain() From c6a87b956ed19f275325b420d60e55acf160f4b7 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 25 May 2016 14:48:28 +0200 Subject: [PATCH 19/30] Add sinon --- package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/package.json b/package.json index 407b934049e..ee6e61e9f81 100644 --- a/package.json +++ b/package.json @@ -55,6 +55,7 @@ "season": "^5.3", "semver": "^4.3.3", "service-hub": "^0.7.0", + "sinon": "1.17.4", "source-map-support": "^0.3.2", "temp": "0.8.1", "text-buffer": "9.1.0", From 1a7858c9f11bdeb4096bfc76fc62d16b0f05202d Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 25 May 2016 14:49:21 +0200 Subject: [PATCH 20/30] Log a more informative message when cannot recover a file --- spec/main-process/file-recovery-service.spec.js | 16 ++++++++-------- src/main-process/file-recovery-service.js | 7 +++++-- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/spec/main-process/file-recovery-service.spec.js b/spec/main-process/file-recovery-service.spec.js index 1cc0062f16e..e18542ea895 100644 --- a/spec/main-process/file-recovery-service.spec.js +++ b/spec/main-process/file-recovery-service.spec.js @@ -5,9 +5,10 @@ import FileRecoveryService from '../../src/main-process/file-recovery-service' import temp from 'temp' import fs from 'fs-plus' import {Emitter} from 'event-kit' +import sinon from 'sinon' describe("FileRecoveryService", () => { - let recoveryService, recoveryDirectory, windows + let recoveryService, recoveryDirectory, windows, previousConsoleLog function createWindow () { const window = new BrowserWindow({show: false}) @@ -116,24 +117,23 @@ describe("FileRecoveryService", () => { }) }) - it("emits a warning when a file can't be recovered", () => { + it("emits a warning when a file can't be recovered", sinon.test(function () { const mockWindow = createWindow() const filePath = temp.path() fs.writeFileSync(filePath, "content") fs.chmodSync(filePath, 0444) - const previousConsoleLog = console.log let logs = [] - console.log = (message) => logs.push(message) + this.stub(console, 'log', (message) => logs.push(message)) recoveryService.willSavePath({sender: mockWindow.webContents}, filePath) mockWindow.webContents.emit("crashed") let recoveryFiles = fs.listTreeSync(recoveryDirectory) assert.equal(recoveryFiles.length, 1) - assert.deepEqual(logs, [`Cannot recover ${filePath}. A recovery file has been saved here: ${recoveryFiles[0]}.`]) - - console.log = previousConsoleLog - }) + assert.equal(logs.length, 1) + assert.match(logs[0], new RegExp(filePath)) + assert.match(logs[0], new RegExp(recoveryFiles[0])) + })) }) it("doesn't create a recovery file when the file that's being saved doesn't exist yet", () => { diff --git a/src/main-process/file-recovery-service.js b/src/main-process/file-recovery-service.js index 89106a6103c..142e86579f7 100644 --- a/src/main-process/file-recovery-service.js +++ b/src/main-process/file-recovery-service.js @@ -1,6 +1,6 @@ 'use babel' -import {BrowserWindow, ipcMain} from 'electron' +import {BrowserWindow, dialog, ipcMain} from 'electron' import crypto from 'crypto' import Path from 'path' import fs from 'fs-plus' @@ -113,7 +113,10 @@ export default class FileRecoveryService { try { recoveryFile.recoverSync() } catch (error) { - console.log(`Cannot recover ${recoveryFile.originalPath}. A recovery file has been saved here: ${recoveryFile.recoveryPath}.`) + console.log( + `There was a crash while saving "${recoveryFile.originalPath}", so this file might be blank or corrupted.\n` + + `Atom couldn't recover it automatically, but a recovery file has been saved at: "${recoveryFile.recoveryPath}".` + ) } finally { this.recoveryFilesByFilePath.delete(recoveryFile.originalPath) } From c2b01d54b8d1e526bb2d5a40275a8013469b927b Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 25 May 2016 15:46:37 +0200 Subject: [PATCH 21/30] Make coupling looser between the recovery service and the windows --- .../file-recovery-service.spec.js | 72 ++++++++----------- src/main-process/atom-application.coffee | 16 +++-- src/main-process/atom-window.coffee | 4 +- src/main-process/file-recovery-service.js | 39 ++++------ 4 files changed, 57 insertions(+), 74 deletions(-) diff --git a/spec/main-process/file-recovery-service.spec.js b/spec/main-process/file-recovery-service.spec.js index e18542ea895..4ff9ad372f7 100644 --- a/spec/main-process/file-recovery-service.spec.js +++ b/spec/main-process/file-recovery-service.spec.js @@ -4,61 +4,47 @@ import {BrowserWindow} from 'electron' import FileRecoveryService from '../../src/main-process/file-recovery-service' import temp from 'temp' import fs from 'fs-plus' -import {Emitter} from 'event-kit' import sinon from 'sinon' describe("FileRecoveryService", () => { - let recoveryService, recoveryDirectory, windows, previousConsoleLog - - function createWindow () { - const window = new BrowserWindow({show: false}) - windows.push(window) - return window - } + let recoveryService, recoveryDirectory beforeEach(() => { - windows = [] recoveryDirectory = temp.mkdirSync() recoveryService = new FileRecoveryService(recoveryDirectory) }) - afterEach(() => { - for (let window of windows) { - window.destroy() - } - }) - describe("when no crash happens during a save", () => { it("creates a recovery file and deletes it after saving", () => { - const mockWindow = createWindow() + const mockWindow = {} const filePath = temp.path() fs.writeFileSync(filePath, "some content") - recoveryService.willSavePath({sender: mockWindow.webContents}, filePath) + recoveryService.willSavePath(mockWindow, filePath) assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) fs.writeFileSync(filePath, "changed") - recoveryService.didSavePath({sender: mockWindow.webContents}, filePath) + recoveryService.didSavePath(mockWindow, filePath) assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) assert.equal(fs.readFileSync(filePath, 'utf8'), "changed") }) it("creates only one recovery file when many windows attempt to save the same file, deleting it when the last one finishes saving it", () => { - const mockWindow = createWindow() - const anotherMockWindow = createWindow() + const mockWindow = {} + const anotherMockWindow = {} const filePath = temp.path() fs.writeFileSync(filePath, "some content") - recoveryService.willSavePath({sender: mockWindow.webContents}, filePath) - recoveryService.willSavePath({sender: anotherMockWindow.webContents}, filePath) + recoveryService.willSavePath(mockWindow, filePath) + recoveryService.willSavePath(anotherMockWindow, filePath) assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) fs.writeFileSync(filePath, "changed") - recoveryService.didSavePath({sender: mockWindow.webContents}, filePath) + recoveryService.didSavePath(mockWindow, filePath) assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) assert.equal(fs.readFileSync(filePath, 'utf8'), "changed") - recoveryService.didSavePath({sender: anotherMockWindow.webContents}, filePath) + recoveryService.didSavePath(anotherMockWindow, filePath) assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) assert.equal(fs.readFileSync(filePath, 'utf8'), "changed") }) @@ -66,59 +52,59 @@ describe("FileRecoveryService", () => { describe("when a crash happens during a save", () => { it("restores the created recovery file and deletes it", () => { - const mockWindow = createWindow() + const mockWindow = {} const filePath = temp.path() fs.writeFileSync(filePath, "some content") - recoveryService.willSavePath({sender: mockWindow.webContents}, filePath) + recoveryService.willSavePath(mockWindow, filePath) assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) fs.writeFileSync(filePath, "changed") - mockWindow.webContents.emit("crashed") + recoveryService.didCrashWindow(mockWindow) assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) assert.equal(fs.readFileSync(filePath, 'utf8'), "some content") }) describe("when many windows attempt to save the same file", () => { it("recovers the file when the window that initiated the save crashes", () => { - const mockWindow = createWindow() - const anotherMockWindow = createWindow() + const mockWindow = {} + const anotherMockWindow = {} const filePath = temp.path() fs.writeFileSync(filePath, "window 1") - recoveryService.willSavePath({sender: mockWindow.webContents}, filePath) + recoveryService.willSavePath(mockWindow, filePath) fs.writeFileSync(filePath, "window 2") - recoveryService.willSavePath({sender: anotherMockWindow.webContents}, filePath) + recoveryService.willSavePath(anotherMockWindow, filePath) assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) fs.writeFileSync(filePath, "changed") - mockWindow.webContents.emit("crashed") + recoveryService.didCrashWindow(mockWindow) assert.equal(fs.readFileSync(filePath, 'utf8'), "window 1") assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) }) it("recovers the file when a window that did not initiate the save crashes", () => { - const mockWindow = createWindow() - const anotherMockWindow = createWindow() + const mockWindow = {} + const anotherMockWindow = {} const filePath = temp.path() fs.writeFileSync(filePath, "window 1") - recoveryService.willSavePath({sender: mockWindow.webContents}, filePath) + recoveryService.willSavePath(mockWindow, filePath) fs.writeFileSync(filePath, "window 2") - recoveryService.willSavePath({sender: anotherMockWindow.webContents}, filePath) + recoveryService.willSavePath(anotherMockWindow, filePath) assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) fs.writeFileSync(filePath, "changed") - anotherMockWindow.webContents.emit("crashed") + recoveryService.didCrashWindow(anotherMockWindow) assert.equal(fs.readFileSync(filePath, 'utf8'), "window 1") assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) }) }) it("emits a warning when a file can't be recovered", sinon.test(function () { - const mockWindow = createWindow() + const mockWindow = {} const filePath = temp.path() fs.writeFileSync(filePath, "content") fs.chmodSync(filePath, 0444) @@ -126,8 +112,8 @@ describe("FileRecoveryService", () => { let logs = [] this.stub(console, 'log', (message) => logs.push(message)) - recoveryService.willSavePath({sender: mockWindow.webContents}, filePath) - mockWindow.webContents.emit("crashed") + recoveryService.willSavePath(mockWindow, filePath) + recoveryService.didCrashWindow(mockWindow) let recoveryFiles = fs.listTreeSync(recoveryDirectory) assert.equal(recoveryFiles.length, 1) assert.equal(logs.length, 1) @@ -137,12 +123,12 @@ describe("FileRecoveryService", () => { }) it("doesn't create a recovery file when the file that's being saved doesn't exist yet", () => { - const mockWindow = createWindow() + const mockWindow = {} - recoveryService.willSavePath({sender: mockWindow.webContents}, "a-file-that-doesnt-exist") + recoveryService.willSavePath(mockWindow, "a-file-that-doesnt-exist") assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) - recoveryService.didSavePath({sender: mockWindow.webContents}, "a-file-that-doesnt-exist") + recoveryService.didSavePath(mockWindow, "a-file-that-doesnt-exist") assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) }) }) diff --git a/src/main-process/atom-application.coffee b/src/main-process/atom-application.coffee index 1b4537b2fed..61a8f7ed0ae 100644 --- a/src/main-process/atom-application.coffee +++ b/src/main-process/atom-application.coffee @@ -245,7 +245,7 @@ class AtomApplication options.window = window @openPaths(options) else - new AtomWindow(options) + new AtomWindow(@fileRecoveryService, options) else @promptForPathToOpen('all', {window}) @@ -328,6 +328,14 @@ class AtomApplication ipcMain.on 'get-auto-update-manager-error', (event) => event.returnValue = @autoUpdateManager.getErrorMessage() + ipcMain.on 'will-save-path', (event, path) => + @fileRecoveryService.willSavePath(@windowForEvent(event), path) + event.returnValue = true + + ipcMain.on 'did-save-path', (event, path) => + @fileRecoveryService.didSavePath(@windowForEvent(event), path) + event.returnValue = true + setupDockMenu: -> if process.platform is 'darwin' dockMenu = Menu.buildFromTemplate [ @@ -485,7 +493,7 @@ class AtomApplication windowInitializationScript ?= require.resolve('../initialize-application-window') resourcePath ?= @resourcePath windowDimensions ?= @getDimensionsForNewWindow() - openedWindow = new AtomWindow({initialPaths, locationsToOpen, windowInitializationScript, resourcePath, devMode, safeMode, windowDimensions, profileStartup, clearWindowState, env}) + openedWindow = new AtomWindow(@fileRecoveryService, {initialPaths, locationsToOpen, windowInitializationScript, resourcePath, devMode, safeMode, windowDimensions, profileStartup, clearWindowState, env}) if pidToKillWhenClosed? @pidsToOpenWindows[pidToKillWhenClosed] = openedWindow @@ -564,7 +572,7 @@ class AtomApplication packagePath = @packages.resolvePackagePath(packageName) windowInitializationScript = path.resolve(packagePath, pack.urlMain) windowDimensions = @getDimensionsForNewWindow() - new AtomWindow({windowInitializationScript, @resourcePath, devMode, safeMode, urlToOpen, windowDimensions, env}) + new AtomWindow(@fileRecoveryService, {windowInitializationScript, @resourcePath, devMode, safeMode, urlToOpen, windowDimensions, env}) else console.log "Package '#{pack.name}' does not have a url main: #{urlToOpen}" else @@ -609,7 +617,7 @@ class AtomApplication devMode = true isSpec = true safeMode ?= false - new AtomWindow({windowInitializationScript, resourcePath, headless, isSpec, devMode, testRunnerPath, legacyTestRunnerPath, testPaths, logFile, safeMode, env}) + new AtomWindow(@fileRecoveryService, {windowInitializationScript, resourcePath, headless, isSpec, devMode, testRunnerPath, legacyTestRunnerPath, testPaths, logFile, safeMode, env}) resolveTestRunnerPath: (testPath) -> FindParentDir ?= require 'find-parent-dir' diff --git a/src/main-process/atom-window.coffee b/src/main-process/atom-window.coffee index 4954cf409ce..0bb4ea182fa 100644 --- a/src/main-process/atom-window.coffee +++ b/src/main-process/atom-window.coffee @@ -15,7 +15,7 @@ class AtomWindow loaded: null isSpec: null - constructor: (settings={}) -> + constructor: (@fileRecoveryService, settings={}) -> {@resourcePath, initialPaths, pathToOpen, locationsToOpen, @isSpec, @headless, @safeMode, @devMode} = settings locationsToOpen ?= [{pathToOpen}] if pathToOpen locationsToOpen ?= [] @@ -125,6 +125,7 @@ class AtomWindow global.atomApplication.saveState(false) @browserWindow.on 'closed', => + @fileRecoveryService.didCloseWindow(this) global.atomApplication.removeWindow(this) @browserWindow.on 'unresponsive', => @@ -140,6 +141,7 @@ class AtomWindow @browserWindow.webContents.on 'crashed', => global.atomApplication.exit(100) if @headless + @fileRecoveryService.didCrashWindow(this) dialog.showMessageBox @browserWindow, type: 'warning' buttons: ['Close Window', 'Reload', 'Keep It Open'] diff --git a/src/main-process/file-recovery-service.js b/src/main-process/file-recovery-service.js index 142e86579f7..434219725de 100644 --- a/src/main-process/file-recovery-service.js +++ b/src/main-process/file-recovery-service.js @@ -56,19 +56,9 @@ export default class FileRecoveryService { this.observedWindows = new WeakSet() } - start () { - ipcMain.on('will-save-path', this.willSavePath.bind(this)) - ipcMain.on('did-save-path', this.didSavePath.bind(this)) - } - - willSavePath (event, path) { - if (!fs.existsSync(path)) { - // Unexisting files won't be truncated/overwritten, and so there's no data to be lost. - event.returnValue = false - return - } + willSavePath (window, path) { + if (!fs.existsSync(path)) return - const window = BrowserWindow.fromWebContents(event.sender) let recoveryFile = this.recoveryFilesByFilePath.get(path) if (recoveryFile == null) { recoveryFile = new RecoveryFile( @@ -79,34 +69,26 @@ export default class FileRecoveryService { } recoveryFile.retain() - if (!this.recoveryFilesByWindow.has(window)) this.recoveryFilesByWindow.set(window, new Set()) - this.recoveryFilesByWindow.get(window).add(recoveryFile) - if (!this.observedWindows.has(window)) { this.observedWindows.add(window) - window.webContents.on('crashed', () => this.recoverFilesForWindow(window)) - window.on('closed', () => { - this.observedWindows.delete(window) - this.recoveryFilesByWindow.delete(window) - }) } - event.returnValue = true + if (!this.recoveryFilesByWindow.has(window)) { + this.recoveryFilesByWindow.set(window, new Set()) + } + this.recoveryFilesByWindow.get(window).add(recoveryFile) } - didSavePath (event, path) { - const window = BrowserWindow.fromWebContents(event.sender) + didSavePath (window, path) { const recoveryFile = this.recoveryFilesByFilePath.get(path) if (recoveryFile != null) { recoveryFile.release() if (recoveryFile.isReleased()) this.recoveryFilesByFilePath.delete(path) this.recoveryFilesByWindow.get(window).delete(recoveryFile) } - - event.returnValue = true } - recoverFilesForWindow (window) { + didCrashWindow (window) { if (!this.recoveryFilesByWindow.has(window)) return for (const recoveryFile of this.recoveryFilesByWindow.get(window)) { @@ -124,4 +106,9 @@ export default class FileRecoveryService { this.recoveryFilesByWindow.delete(window) } + + didCloseWindow (window) { + this.observedWindows.delete(window) + this.recoveryFilesByWindow.delete(window) + } } From 8ba275a75d7f433bc6216c638e42ce0e6333ac83 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 25 May 2016 15:47:12 +0200 Subject: [PATCH 22/30] :art: Move RecoveryFile down --- src/main-process/file-recovery-service.js | 86 +++++++++++------------ 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/src/main-process/file-recovery-service.js b/src/main-process/file-recovery-service.js index 434219725de..7720923212c 100644 --- a/src/main-process/file-recovery-service.js +++ b/src/main-process/file-recovery-service.js @@ -5,49 +5,6 @@ import crypto from 'crypto' import Path from 'path' import fs from 'fs-plus' -class RecoveryFile { - static fileNameForPath (path) { - const extension = Path.extname(path) - const basename = Path.basename(path, extension).substring(0, 34) - const randomSuffix = crypto.randomBytes(3).toString('hex') - return `${basename}-${randomSuffix}${extension}` - } - - constructor (originalPath, recoveryPath) { - this.originalPath = originalPath - this.recoveryPath = recoveryPath - this.refCount = 0 - } - - storeSync () { - fs.writeFileSync(this.recoveryPath, fs.readFileSync(this.originalPath)) - } - - recoverSync () { - fs.writeFileSync(this.originalPath, fs.readFileSync(this.recoveryPath)) - this.removeSync() - this.refCount = 0 - } - - removeSync () { - fs.unlinkSync(this.recoveryPath) - } - - retain () { - if (this.refCount === 0) this.storeSync() - this.refCount++ - } - - release () { - this.refCount-- - if (this.refCount === 0) this.removeSync() - } - - isReleased () { - return this.refCount === 0 - } -} - export default class FileRecoveryService { constructor (recoveryDirectory) { this.recoveryDirectory = recoveryDirectory @@ -112,3 +69,46 @@ export default class FileRecoveryService { this.recoveryFilesByWindow.delete(window) } } + +class RecoveryFile { + static fileNameForPath (path) { + const extension = Path.extname(path) + const basename = Path.basename(path, extension).substring(0, 34) + const randomSuffix = crypto.randomBytes(3).toString('hex') + return `${basename}-${randomSuffix}${extension}` + } + + constructor (originalPath, recoveryPath) { + this.originalPath = originalPath + this.recoveryPath = recoveryPath + this.refCount = 0 + } + + storeSync () { + fs.writeFileSync(this.recoveryPath, fs.readFileSync(this.originalPath)) + } + + recoverSync () { + fs.writeFileSync(this.originalPath, fs.readFileSync(this.recoveryPath)) + this.removeSync() + this.refCount = 0 + } + + removeSync () { + fs.unlinkSync(this.recoveryPath) + } + + retain () { + if (this.refCount === 0) this.storeSync() + this.refCount++ + } + + release () { + this.refCount-- + if (this.refCount === 0) this.removeSync() + } + + isReleased () { + return this.refCount === 0 + } +} From 49a603a87300af781d031f9994956ecefd50609a Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 25 May 2016 15:54:39 +0200 Subject: [PATCH 23/30] Show also a message box when recovery is not successful --- spec/main-process/file-recovery-service.spec.js | 3 ++- src/main-process/atom-application.coffee | 1 - src/main-process/atom-window.coffee | 9 ++++----- src/main-process/file-recovery-service.js | 6 ++++-- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/spec/main-process/file-recovery-service.spec.js b/spec/main-process/file-recovery-service.spec.js index 4ff9ad372f7..58857fa1da7 100644 --- a/spec/main-process/file-recovery-service.spec.js +++ b/spec/main-process/file-recovery-service.spec.js @@ -1,6 +1,6 @@ 'use babel' -import {BrowserWindow} from 'electron' +import {dialog} from 'electron' import FileRecoveryService from '../../src/main-process/file-recovery-service' import temp from 'temp' import fs from 'fs-plus' @@ -111,6 +111,7 @@ describe("FileRecoveryService", () => { let logs = [] this.stub(console, 'log', (message) => logs.push(message)) + this.stub(dialog, 'showMessageBox') recoveryService.willSavePath(mockWindow, filePath) recoveryService.didCrashWindow(mockWindow) diff --git a/src/main-process/atom-application.coffee b/src/main-process/atom-application.coffee index 61a8f7ed0ae..2485c3ddc27 100644 --- a/src/main-process/atom-application.coffee +++ b/src/main-process/atom-application.coffee @@ -86,7 +86,6 @@ class AtomApplication @handleEvents() @setupDockMenu() @storageFolder = new StorageFolder(process.env.ATOM_HOME) - @fileRecoveryService.start() if options.pathsToOpen?.length > 0 or options.urlsToOpen?.length > 0 or options.test @openWithOptions(options) diff --git a/src/main-process/atom-window.coffee b/src/main-process/atom-window.coffee index 0bb4ea182fa..8001cf098e7 100644 --- a/src/main-process/atom-window.coffee +++ b/src/main-process/atom-window.coffee @@ -142,15 +142,14 @@ class AtomWindow global.atomApplication.exit(100) if @headless @fileRecoveryService.didCrashWindow(this) - dialog.showMessageBox @browserWindow, + chosen = dialog.showMessageBox @browserWindow, type: 'warning' buttons: ['Close Window', 'Reload', 'Keep It Open'] message: 'The editor has crashed' detail: 'Please report this issue to https://github.com/atom/atom', - (chosen) => - switch chosen - when 0 then @browserWindow.destroy() - when 1 then @browserWindow.reload() + switch chosen + when 0 then @browserWindow.destroy() + when 1 then @browserWindow.reload() @browserWindow.webContents.on 'will-navigate', (event, url) => unless url is @browserWindow.webContents.getURL() diff --git a/src/main-process/file-recovery-service.js b/src/main-process/file-recovery-service.js index 7720923212c..8f591234ee9 100644 --- a/src/main-process/file-recovery-service.js +++ b/src/main-process/file-recovery-service.js @@ -52,10 +52,12 @@ export default class FileRecoveryService { try { recoveryFile.recoverSync() } catch (error) { - console.log( + const message = 'A file that Atom was saving could be corrupted' + const detail = `There was a crash while saving "${recoveryFile.originalPath}", so this file might be blank or corrupted.\n` + `Atom couldn't recover it automatically, but a recovery file has been saved at: "${recoveryFile.recoveryPath}".` - ) + console.log(detail) + dialog.showMessageBox(window.browserWindow, {type: 'info', buttons: ['OK'], message, detail}) } finally { this.recoveryFilesByFilePath.delete(recoveryFile.originalPath) } From 3f8f3c95a8304390a8172440c40a196e2a9ab711 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 25 May 2016 15:55:36 +0200 Subject: [PATCH 24/30] :fire: Remove extra comma --- src/main-process/atom-window.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main-process/atom-window.coffee b/src/main-process/atom-window.coffee index 8001cf098e7..36a7a25c65e 100644 --- a/src/main-process/atom-window.coffee +++ b/src/main-process/atom-window.coffee @@ -146,7 +146,7 @@ class AtomWindow type: 'warning' buttons: ['Close Window', 'Reload', 'Keep It Open'] message: 'The editor has crashed' - detail: 'Please report this issue to https://github.com/atom/atom', + detail: 'Please report this issue to https://github.com/atom/atom' switch chosen when 0 then @browserWindow.destroy() when 1 then @browserWindow.reload() From 8733b52992244d0b3c882b7e8a5998169ace6f2e Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 25 May 2016 15:56:14 +0200 Subject: [PATCH 25/30] :fire: Extra imports --- src/main-process/file-recovery-service.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main-process/file-recovery-service.js b/src/main-process/file-recovery-service.js index 8f591234ee9..0dc12ef06b5 100644 --- a/src/main-process/file-recovery-service.js +++ b/src/main-process/file-recovery-service.js @@ -1,6 +1,6 @@ 'use babel' -import {BrowserWindow, dialog, ipcMain} from 'electron' +import {dialog} from 'electron' import crypto from 'crypto' import Path from 'path' import fs from 'fs-plus' From d8564add7a8b3ef252ae48c55e03a0834d767add Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 25 May 2016 17:02:02 +0200 Subject: [PATCH 26/30] Be a little more defensive when retaining/releasing recovery files --- src/main-process/file-recovery-service.js | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/main-process/file-recovery-service.js b/src/main-process/file-recovery-service.js index 0dc12ef06b5..d17d15c8b67 100644 --- a/src/main-process/file-recovery-service.js +++ b/src/main-process/file-recovery-service.js @@ -24,7 +24,12 @@ export default class FileRecoveryService { ) this.recoveryFilesByFilePath.set(path, recoveryFile) } - recoveryFile.retain() + + try { + recoveryFile.retain() + } catch (err) { + console.log(`Couldn't retain ${recoveryFile.recoveryPath}. Code: ${err.code}. Message: ${err.message}`) + } if (!this.observedWindows.has(window)) { this.observedWindows.add(window) @@ -39,7 +44,11 @@ export default class FileRecoveryService { didSavePath (window, path) { const recoveryFile = this.recoveryFilesByFilePath.get(path) if (recoveryFile != null) { - recoveryFile.release() + try { + recoveryFile.release() + } catch (err) { + console.log(`Couldn't release ${recoveryFile.recoveryPath}. Code: ${err.code}. Message: ${err.message}`) + } if (recoveryFile.isReleased()) this.recoveryFilesByFilePath.delete(path) this.recoveryFilesByWindow.get(window).delete(recoveryFile) } From aefcbcda47a2249a2ebd56a5895e07555e8be895 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 25 May 2016 17:19:59 +0200 Subject: [PATCH 27/30] :fire: Remove unneeded WeakSet --- src/main-process/file-recovery-service.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/main-process/file-recovery-service.js b/src/main-process/file-recovery-service.js index d17d15c8b67..c7e4d6c3b0d 100644 --- a/src/main-process/file-recovery-service.js +++ b/src/main-process/file-recovery-service.js @@ -10,7 +10,6 @@ export default class FileRecoveryService { this.recoveryDirectory = recoveryDirectory this.recoveryFilesByFilePath = new Map() this.recoveryFilesByWindow = new WeakMap() - this.observedWindows = new WeakSet() } willSavePath (window, path) { @@ -31,10 +30,6 @@ export default class FileRecoveryService { console.log(`Couldn't retain ${recoveryFile.recoveryPath}. Code: ${err.code}. Message: ${err.message}`) } - if (!this.observedWindows.has(window)) { - this.observedWindows.add(window) - } - if (!this.recoveryFilesByWindow.has(window)) { this.recoveryFilesByWindow.set(window, new Set()) } @@ -76,7 +71,6 @@ export default class FileRecoveryService { } didCloseWindow (window) { - this.observedWindows.delete(window) this.recoveryFilesByWindow.delete(window) } } From 6c348449562514e123be1a50b85ba1654c8d1237 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Thu, 26 May 2016 11:17:45 +0200 Subject: [PATCH 28/30] :bug: Don't try to recover the same file twice --- .../file-recovery-service.spec.js | 64 ++++++++----------- src/main-process/file-recovery-service.js | 20 ++++-- 2 files changed, 44 insertions(+), 40 deletions(-) diff --git a/spec/main-process/file-recovery-service.spec.js b/spec/main-process/file-recovery-service.spec.js index 58857fa1da7..01d625cfc69 100644 --- a/spec/main-process/file-recovery-service.spec.js +++ b/spec/main-process/file-recovery-service.spec.js @@ -65,42 +65,34 @@ describe("FileRecoveryService", () => { assert.equal(fs.readFileSync(filePath, 'utf8'), "some content") }) - describe("when many windows attempt to save the same file", () => { - it("recovers the file when the window that initiated the save crashes", () => { - const mockWindow = {} - const anotherMockWindow = {} - const filePath = temp.path() - - fs.writeFileSync(filePath, "window 1") - recoveryService.willSavePath(mockWindow, filePath) - fs.writeFileSync(filePath, "window 2") - recoveryService.willSavePath(anotherMockWindow, filePath) - assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) - - fs.writeFileSync(filePath, "changed") - - recoveryService.didCrashWindow(mockWindow) - assert.equal(fs.readFileSync(filePath, 'utf8'), "window 1") - assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) - }) - - it("recovers the file when a window that did not initiate the save crashes", () => { - const mockWindow = {} - const anotherMockWindow = {} - const filePath = temp.path() - - fs.writeFileSync(filePath, "window 1") - recoveryService.willSavePath(mockWindow, filePath) - fs.writeFileSync(filePath, "window 2") - recoveryService.willSavePath(anotherMockWindow, filePath) - assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) - - fs.writeFileSync(filePath, "changed") - - recoveryService.didCrashWindow(anotherMockWindow) - assert.equal(fs.readFileSync(filePath, 'utf8'), "window 1") - assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) - }) + it("restores the created recovery file when many windows attempt to save the same file and one of them crashes", () => { + const mockWindow = {} + const anotherMockWindow = {} + const filePath = temp.path() + + fs.writeFileSync(filePath, "A") + recoveryService.willSavePath(mockWindow, filePath) + fs.writeFileSync(filePath, "B") + recoveryService.willSavePath(anotherMockWindow, filePath) + assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) + + fs.writeFileSync(filePath, "C") + + recoveryService.didCrashWindow(mockWindow) + assert.equal(fs.readFileSync(filePath, 'utf8'), "A") + assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) + + fs.writeFileSync(filePath, "D") + recoveryService.willSavePath(mockWindow, filePath) + fs.writeFileSync(filePath, "E") + recoveryService.willSavePath(anotherMockWindow, filePath) + assert.equal(fs.listTreeSync(recoveryDirectory).length, 1) + + fs.writeFileSync(filePath, "F") + + recoveryService.didCrashWindow(anotherMockWindow) + assert.equal(fs.readFileSync(filePath, 'utf8'), "D") + assert.equal(fs.listTreeSync(recoveryDirectory).length, 0) }) it("emits a warning when a file can't be recovered", sinon.test(function () { diff --git a/src/main-process/file-recovery-service.js b/src/main-process/file-recovery-service.js index c7e4d6c3b0d..235fd05f443 100644 --- a/src/main-process/file-recovery-service.js +++ b/src/main-process/file-recovery-service.js @@ -10,6 +10,7 @@ export default class FileRecoveryService { this.recoveryDirectory = recoveryDirectory this.recoveryFilesByFilePath = new Map() this.recoveryFilesByWindow = new WeakMap() + this.windowsByRecoveryFile = new Map() } willSavePath (window, path) { @@ -33,7 +34,11 @@ export default class FileRecoveryService { if (!this.recoveryFilesByWindow.has(window)) { this.recoveryFilesByWindow.set(window, new Set()) } + if (!this.windowsByRecoveryFile.has(recoveryFile)) { + this.windowsByRecoveryFile.set(recoveryFile, new Set()) + } this.recoveryFilesByWindow.get(window).add(recoveryFile) + this.windowsByRecoveryFile.get(recoveryFile).add(window) } didSavePath (window, path) { @@ -46,6 +51,7 @@ export default class FileRecoveryService { } if (recoveryFile.isReleased()) this.recoveryFilesByFilePath.delete(path) this.recoveryFilesByWindow.get(window).delete(recoveryFile) + this.windowsByRecoveryFile.get(recoveryFile).delete(window) } } @@ -58,19 +64,26 @@ export default class FileRecoveryService { } catch (error) { const message = 'A file that Atom was saving could be corrupted' const detail = - `There was a crash while saving "${recoveryFile.originalPath}", so this file might be blank or corrupted.\n` + + `Error ${error.code}. There was a crash while saving "${recoveryFile.originalPath}", so this file might be blank or corrupted.\n` + `Atom couldn't recover it automatically, but a recovery file has been saved at: "${recoveryFile.recoveryPath}".` console.log(detail) dialog.showMessageBox(window.browserWindow, {type: 'info', buttons: ['OK'], message, detail}) } finally { + for (let window of this.windowsByRecoveryFile.get(recoveryFile)) { + this.recoveryFilesByWindow.get(window).delete(recoveryFile) + } + this.windowsByRecoveryFile.delete(recoveryFile) this.recoveryFilesByFilePath.delete(recoveryFile.originalPath) } } - - this.recoveryFilesByWindow.delete(window) } didCloseWindow (window) { + if (!this.recoveryFilesByWindow.has(window)) return + + for (let recoveryFile of this.recoveryFilesByWindow.get(window)) { + this.windowsByRecoveryFile.get(recoveryFile).delete(window) + } this.recoveryFilesByWindow.delete(window) } } @@ -96,7 +109,6 @@ class RecoveryFile { recoverSync () { fs.writeFileSync(this.originalPath, fs.readFileSync(this.recoveryPath)) this.removeSync() - this.refCount = 0 } removeSync () { From df263a2cb1750225c0c4e24df4c2d7b66d94fd89 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Thu, 26 May 2016 11:39:05 +0200 Subject: [PATCH 29/30] Return early when a recovery file can't be stored --- src/main-process/file-recovery-service.js | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/main-process/file-recovery-service.js b/src/main-process/file-recovery-service.js index 235fd05f443..5893b4c8d23 100644 --- a/src/main-process/file-recovery-service.js +++ b/src/main-process/file-recovery-service.js @@ -16,19 +16,15 @@ export default class FileRecoveryService { willSavePath (window, path) { if (!fs.existsSync(path)) return - let recoveryFile = this.recoveryFilesByFilePath.get(path) - if (recoveryFile == null) { - recoveryFile = new RecoveryFile( - path, - Path.join(this.recoveryDirectory, RecoveryFile.fileNameForPath(path)) - ) - this.recoveryFilesByFilePath.set(path, recoveryFile) - } + const recoveryPath = Path.join(this.recoveryDirectory, RecoveryFile.fileNameForPath(path)) + const recoveryFile = + this.recoveryFilesByFilePath.get(path) || new RecoveryFile(path, recoveryPath) try { recoveryFile.retain() } catch (err) { console.log(`Couldn't retain ${recoveryFile.recoveryPath}. Code: ${err.code}. Message: ${err.message}`) + return } if (!this.recoveryFilesByWindow.has(window)) { @@ -37,8 +33,10 @@ export default class FileRecoveryService { if (!this.windowsByRecoveryFile.has(recoveryFile)) { this.windowsByRecoveryFile.set(recoveryFile, new Set()) } + this.recoveryFilesByWindow.get(window).add(recoveryFile) this.windowsByRecoveryFile.get(recoveryFile).add(window) + this.recoveryFilesByFilePath.set(path, recoveryFile) } didSavePath (window, path) { @@ -116,13 +114,13 @@ class RecoveryFile { } retain () { - if (this.refCount === 0) this.storeSync() + if (this.isReleased()) this.storeSync() this.refCount++ } release () { this.refCount-- - if (this.refCount === 0) this.removeSync() + if (this.isReleased()) this.removeSync() } isReleased () { From 8355e7fe72c966750c1a8960be14c8d1456832d6 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Thu, 26 May 2016 17:16:16 +0200 Subject: [PATCH 30/30] Use fs.copyFileSync for buffered copy --- package.json | 2 +- src/main-process/file-recovery-service.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index ee6e61e9f81..2baba03e121 100644 --- a/package.json +++ b/package.json @@ -27,7 +27,7 @@ "event-kit": "^1.5.0", "find-parent-dir": "^0.3.0", "first-mate": "^5.1.1", - "fs-plus": "^2.8.0", + "fs-plus": "2.9.1", "fstream": "0.1.24", "fuzzaldrin": "^2.1", "git-utils": "^4.1.2", diff --git a/src/main-process/file-recovery-service.js b/src/main-process/file-recovery-service.js index 5893b4c8d23..f55e3f956df 100644 --- a/src/main-process/file-recovery-service.js +++ b/src/main-process/file-recovery-service.js @@ -101,11 +101,11 @@ class RecoveryFile { } storeSync () { - fs.writeFileSync(this.recoveryPath, fs.readFileSync(this.originalPath)) + fs.copyFileSync(this.originalPath, this.recoveryPath) } recoverSync () { - fs.writeFileSync(this.originalPath, fs.readFileSync(this.recoveryPath)) + fs.copyFileSync(this.recoveryPath, this.originalPath) this.removeSync() }