diff --git a/spec/command-registry-spec.coffee b/spec/command-registry-spec.coffee index 2af185b16..b92d3e6b0 100644 --- a/spec/command-registry-spec.coffee +++ b/spec/command-registry-spec.coffee @@ -148,6 +148,16 @@ describe "CommandRegistry", -> grandchild.dispatchEvent(new CustomEvent('command-2', bubbles: true)) expect(calls).toEqual [] + describe "::add(selector, commandName, callback)", -> + it "throws an error when called with an invalid selector", -> + badSelector = '<>' + addError = null + try + registry.add badSelector, 'foo:bar', -> + catch error + addError = error + expect(addError.message).toContain(badSelector) + describe "::findCommands({target})", -> it "returns commands that can be invoked on the target or its ancestors", -> registry.add '.parent', 'namespace:command-1', -> diff --git a/spec/context-menu-manager-spec.coffee b/spec/context-menu-manager-spec.coffee index 1c3c1f21a..ab1f05080 100644 --- a/spec/context-menu-manager-spec.coffee +++ b/spec/context-menu-manager-spec.coffee @@ -151,6 +151,14 @@ describe "ContextMenuManager", -> shouldDisplay = false expect(contextMenu.templateForEvent(dispatchedEvent)).toEqual [] + it "throws an error when the selector is invalid", -> + addError = null + try + contextMenu.add '<>': [{label: 'A', command: 'a'}] + catch error + addError = error + expect(addError.message).toContain('<>') + describe "when the menus are specified in a legacy format", -> beforeEach -> jasmine.snapshotDeprecations() diff --git a/spec/fixtures/packages/package-with-invalid-activation-commands/package.json b/spec/fixtures/packages/package-with-invalid-activation-commands/package.json new file mode 100644 index 000000000..d98b7ef92 --- /dev/null +++ b/spec/fixtures/packages/package-with-invalid-activation-commands/package.json @@ -0,0 +1,9 @@ +{ + "name": "package-with-invalid-selectors", + "version": "1.0.0", + "activationCommands": { + "<>": [ + "foo:bar" + ] + } +} diff --git a/spec/fixtures/packages/package-with-invalid-context-menu/menus/menu.json b/spec/fixtures/packages/package-with-invalid-context-menu/menus/menu.json new file mode 100644 index 000000000..67197f8fe --- /dev/null +++ b/spec/fixtures/packages/package-with-invalid-context-menu/menus/menu.json @@ -0,0 +1,10 @@ +{ + "context-menu": { + "<>": [ + { + "label": "Hello", + "command:": "world" + } + ] + } +} diff --git a/spec/fixtures/packages/package-with-invalid-context-menu/package.json b/spec/fixtures/packages/package-with-invalid-context-menu/package.json new file mode 100644 index 000000000..c742d422a --- /dev/null +++ b/spec/fixtures/packages/package-with-invalid-context-menu/package.json @@ -0,0 +1,4 @@ +{ + "name": "package-with-invalid-context-menu", + "version": "1.0.0" +} diff --git a/spec/fixtures/packages/package-with-invalid-grammar/grammars/grammar.json b/spec/fixtures/packages/package-with-invalid-grammar/grammars/grammar.json new file mode 100644 index 000000000..41ec4c527 --- /dev/null +++ b/spec/fixtures/packages/package-with-invalid-grammar/grammars/grammar.json @@ -0,0 +1 @@ +>< diff --git a/spec/fixtures/packages/package-with-invalid-grammar/package.json b/spec/fixtures/packages/package-with-invalid-grammar/package.json new file mode 100644 index 000000000..4ed4081f5 --- /dev/null +++ b/spec/fixtures/packages/package-with-invalid-grammar/package.json @@ -0,0 +1,4 @@ +{ + "name": "package-with-invalid-grammar", + "version": "1.0.0" +} diff --git a/spec/fixtures/packages/package-with-invalid-settings/package.json b/spec/fixtures/packages/package-with-invalid-settings/package.json new file mode 100644 index 000000000..b09dccb8b --- /dev/null +++ b/spec/fixtures/packages/package-with-invalid-settings/package.json @@ -0,0 +1,4 @@ +{ + "name": "package-with-invalid-settings", + "version": "1.0.0" +} diff --git a/spec/fixtures/packages/package-with-invalid-settings/settings/settings.json b/spec/fixtures/packages/package-with-invalid-settings/settings/settings.json new file mode 100644 index 000000000..41ec4c527 --- /dev/null +++ b/spec/fixtures/packages/package-with-invalid-settings/settings/settings.json @@ -0,0 +1 @@ +>< diff --git a/spec/package-manager-spec.coffee b/spec/package-manager-spec.coffee index d9898505c..6efb17a55 100644 --- a/spec/package-manager-spec.coffee +++ b/spec/package-manager-spec.coffee @@ -18,7 +18,6 @@ describe "PackageManager", -> expect(pack.metadata.name).toBe "package-with-index" it "returns the package if it has an invalid keymap", -> - spyOn(console, 'warn') pack = atom.packages.loadPackage("package-with-broken-keymap") expect(pack instanceof Package).toBe true expect(pack.metadata.name).toBe "package-with-broken-keymap" @@ -30,10 +29,11 @@ describe "PackageManager", -> expect(pack.stylesheets.length).toBe 0 it "returns null if the package has an invalid package.json", -> - spyOn(console, 'warn') + addErrorHandler = jasmine.createSpy() + atom.notifications.onDidAddNotification(addErrorHandler) expect(atom.packages.loadPackage("package-with-broken-package-json")).toBeNull() - expect(console.warn.callCount).toBe(1) - expect(console.warn.argsForCall[0][0]).toContain("Failed to load package.json") + expect(addErrorHandler.callCount).toBe 1 + expect(addErrorHandler.argsForCall[0][0].message).toContain("Failed to load the package-with-broken-package-json package") it "returns null if the package is not found in any package directory", -> spyOn(console, 'warn') @@ -212,6 +212,46 @@ describe "PackageManager", -> runs -> expect(mainModule.activate.callCount).toBe 1 + it "adds a notification when the activation commands are invalid", -> + addErrorHandler = jasmine.createSpy() + atom.notifications.onDidAddNotification(addErrorHandler) + expect(-> atom.packages.activatePackage('package-with-invalid-activation-commands')).not.toThrow() + expect(addErrorHandler.callCount).toBe 1 + expect(addErrorHandler.argsForCall[0][0].message).toContain("Failed to activate the package-with-invalid-activation-commands package") + + it "adds a notification when the context menu is invalid", -> + addErrorHandler = jasmine.createSpy() + atom.notifications.onDidAddNotification(addErrorHandler) + expect(-> atom.packages.activatePackage('package-with-invalid-context-menu')).not.toThrow() + expect(addErrorHandler.callCount).toBe 1 + expect(addErrorHandler.argsForCall[0][0].message).toContain("Failed to activate the package-with-invalid-context-menu package") + + it "adds a notification when the grammar is invalid", -> + addErrorHandler = jasmine.createSpy() + atom.notifications.onDidAddNotification(addErrorHandler) + + expect(-> atom.packages.activatePackage('package-with-invalid-grammar')).not.toThrow() + + waitsFor -> + addErrorHandler.callCount > 0 + + runs -> + expect(addErrorHandler.callCount).toBe 1 + expect(addErrorHandler.argsForCall[0][0].message).toContain("Failed to load a package-with-invalid-grammar package grammar") + + it "adds a notification when the settings are invalid", -> + addErrorHandler = jasmine.createSpy() + atom.notifications.onDidAddNotification(addErrorHandler) + + expect(-> atom.packages.activatePackage('package-with-invalid-settings')).not.toThrow() + + waitsFor -> + addErrorHandler.callCount > 0 + + runs -> + expect(addErrorHandler.callCount).toBe 1 + expect(addErrorHandler.argsForCall[0][0].message).toContain("Failed to load the package-with-invalid-settings package settings") + describe "when the package has no main module", -> it "does not throw an exception", -> spyOn(console, "error") @@ -257,11 +297,13 @@ describe "PackageManager", -> runs -> expect(activatedPackage.name).toBe 'package-with-main' describe "when the package throws an error while loading", -> - it "logs a warning instead of throwing an exception", -> + it "adds a notification instead of throwing an exception", -> atom.config.set("core.disabledPackages", []) - spyOn(console, "warn") + addErrorHandler = jasmine.createSpy() + atom.notifications.onDidAddNotification(addErrorHandler) expect(-> atom.packages.activatePackage("package-that-throws-an-exception")).not.toThrow() - expect(console.warn).toHaveBeenCalled() + expect(addErrorHandler.callCount).toBe 1 + expect(addErrorHandler.argsForCall[0][0].message).toContain("Failed to load the package-that-throws-an-exception package") describe "when the package is not found", -> it "rejects the promise", -> diff --git a/src/command-registry.coffee b/src/command-registry.coffee index 71e35f7e5..ead4415d2 100644 --- a/src/command-registry.coffee +++ b/src/command-registry.coffee @@ -2,6 +2,7 @@ {specificity} = require 'clear-cut' _ = require 'underscore-plus' {$} = require './space-pen-extensions' +{validateSelector} = require './selector-validator' SequenceCount = 0 SpecificityCache = {} @@ -87,6 +88,7 @@ class CommandRegistry return disposable if typeof target is 'string' + validateSelector(target) @addSelectorBasedListener(target, commandName, callback) else @addInlineListener(target, commandName, callback) diff --git a/src/context-menu-manager.coffee b/src/context-menu-manager.coffee index b890fc29a..3f281afb1 100644 --- a/src/context-menu-manager.coffee +++ b/src/context-menu-manager.coffee @@ -8,6 +8,7 @@ fs = require 'fs-plus' {Disposable} = require 'event-kit' Grim = require 'grim' MenuHelpers = require './menu-helpers' +{validateSelector} = require './selector-validator' SpecificityCache = {} @@ -123,6 +124,7 @@ class ContextMenuManager addedItemSets = [] for selector, items of itemsBySelector + validateSelector(selector) itemSet = new ContextMenuItemSet(selector, items) addedItemSets.push(itemSet) @itemSets.push(itemSet) diff --git a/src/package-manager.coffee b/src/package-manager.coffee index bcd7dc28e..9f53d45e2 100644 --- a/src/package-manager.coffee +++ b/src/package-manager.coffee @@ -343,16 +343,18 @@ class PackageManager try metadata = Package.loadMetadata(packagePath) ? {} - if metadata.theme - pack = new ThemePackage(packagePath, metadata) - else - pack = new Package(packagePath, metadata) - pack.load() - @loadedPackages[pack.name] = pack - @emitter.emit 'did-load-package', pack - return pack catch error - console.warn "Failed to load package.json '#{path.basename(packagePath)}'", error.stack ? error + @handleMetadataError(error, packagePath) + return null + + if metadata.theme + pack = new ThemePackage(packagePath, metadata) + else + pack = new Package(packagePath, metadata) + pack.load() + @loadedPackages[pack.name] = pack + @emitter.emit 'did-load-package', pack + return pack else console.warn "Could not resolve '#{nameOrPath}' to a package path" null @@ -421,3 +423,10 @@ class PackageManager pack.deactivate() delete @activePackages[pack.name] @emitter.emit 'did-deactivate-package', pack + + handleMetadataError: (error, packagePath) -> + metadataPath = path.join(packagePath, 'package.json') + detail = "#{error.message} in #{metadataPath}" + stack = "#{error.stack}\n at #{metadataPath}:1:1" + message = "Failed to load the #{path.basename(packagePath)} package" + atom.notifications.addError(message, {stack, detail, dismissable: true}) diff --git a/src/package.coffee b/src/package.coffee index d8ba4a1d8..758894952 100644 --- a/src/package.coffee +++ b/src/package.coffee @@ -126,9 +126,8 @@ class Package @loadStylesheets() @settingsPromise = @loadSettings() @requireMainModule() unless @hasActivationCommands() - catch error - console.warn "Failed to load package named '#{@name}'", error.stack ? error + @handleError("Failed to load the #{@name} package", error) this reset: -> @@ -144,11 +143,14 @@ class Package unless @activationDeferred? @activationDeferred = Q.defer() @measure 'activateTime', => - @activateResources() - if @hasActivationCommands() - @subscribeToActivationCommands() - else - @activateNow() + try + @activateResources() + if @hasActivationCommands() + @subscribeToActivationCommands() + else + @activateNow() + catch error + @handleError("Failed to activate the #{@name} package", error) Q.all([@grammarsPromise, @settingsPromise, @activationDeferred.promise]) @@ -160,8 +162,8 @@ class Package @mainModule.activate?(atom.packages.getPackageState(@name) ? {}) @mainActivated = true @activateServices() - catch e - console.warn "Failed to activate package named '#{@name}'", e.stack + catch error + @handleError("Failed to activate the #{@name} package", error) @activationDeferred?.resolve() @@ -200,7 +202,16 @@ class Package activateResources: -> @activationDisposables = new CompositeDisposable @activationDisposables.add(atom.keymaps.add(keymapPath, map)) for [keymapPath, map] in @keymaps - @activationDisposables.add(atom.contextMenu.add(map['context-menu'])) for [menuPath, map] in @menus when map['context-menu']? + + for [menuPath, map] in @menus when map['context-menu']? + try + @activationDisposables.add(atom.contextMenu.add(map['context-menu'])) + catch error + if error.code is 'EBADSELECTOR' + error.message += " in #{menuPath}" + error.stack += "\n at #{menuPath}:1:1" + throw error + @activationDisposables.add(atom.menu.add(map['menu'])) for [menuPath, map] in @menus when map['menu']? unless @grammarsActivated @@ -218,6 +229,7 @@ class Package for name, {versions} of @metadata.consumedServices for version, methodName of versions @activationDisposables.add atom.packages.serviceHub.consume(name, version, @mainModule[methodName].bind(@mainModule)) + return loadKeymaps: -> if @bundledPackage and packagesCache[@name]? @@ -290,7 +302,9 @@ class Package loadGrammar = (grammarPath, callback) => atom.grammars.readGrammar grammarPath, (error, grammar) => if error? - console.warn("Failed to load grammar: #{grammarPath}", error.stack ? error) + detail = "#{error.message} in #{grammarPath}" + stack = "#{error.stack}\n at #{grammarPath}:1:1" + atom.notifications.addFatalError("Failed to load a #{@name} package grammar", {stack, detail, dismissable: true}) else grammar.packageName = @name @grammars.push(grammar) @@ -309,7 +323,9 @@ class Package loadSettingsFile = (settingsPath, callback) => ScopedProperties.load settingsPath, (error, settings) => if error? - console.warn("Failed to load package settings: #{settingsPath}", error.stack ? error) + detail = "#{error.message} in #{settingsPath}" + stack = "#{error.stack}\n at #{settingsPath}:1:1" + atom.notifications.addFatalError("Failed to load the #{@name} package settings", {stack, detail, dismissable: true}) else @settings.push(settings) settings.activate() if @settingsActivated @@ -370,7 +386,7 @@ class Package @activateStylesheets() requireMainModule: -> - return @mainModule if @mainModule? + return @mainModule if @mainModuleRequired unless @isCompatible() console.warn """ Failed to require the main module of '#{@name}' because it requires an incompatible native module. @@ -378,7 +394,9 @@ class Package """ return mainModulePath = @getMainModulePath() - @mainModule = require(mainModulePath) if fs.isFileSync(mainModulePath) + if fs.isFileSync(mainModulePath) + @mainModuleRequired = true + @mainModule = require(mainModulePath) getMainModulePath: -> return @mainModulePath if @resolvedMainModulePath @@ -409,7 +427,15 @@ class Package do (selector, command) => # Add dummy command so it appears in menu. # The real command will be registered on package activation - @activationCommandSubscriptions.add atom.commands.add selector, command, -> + try + @activationCommandSubscriptions.add atom.commands.add selector, command, -> + catch error + if error.code is 'EBADSELECTOR' + metadataPath = path.join(@path, 'package.json') + error.message += " in #{metadataPath}" + error.stack += "\n at #{metadataPath}:1:1" + throw error + @activationCommandSubscriptions.add atom.commands.onWillDispatch (event) => return unless event.type is command currentTarget = event.target @@ -528,3 +554,17 @@ class Package @compatible = @incompatibleModules.length is 0 else @compatible = true + + handleError: (message, error) -> + if error.filename and error.location and (error instanceof SyntaxError) + location = "#{error.filename}:#{error.location.first_line + 1}:#{error.location.first_column + 1}" + detail = "#{error.message} in #{location}" + stack = """ + SyntaxError: #{error.message} + at #{location} + """ + else + detail = error.message + stack = error.stack ? error + + atom.notifications.addFatalError(message, {stack, detail, dismissable: true}) diff --git a/src/selector-validator.coffee b/src/selector-validator.coffee new file mode 100644 index 000000000..f8dcb240a --- /dev/null +++ b/src/selector-validator.coffee @@ -0,0 +1,28 @@ +selectorCache = null +testElement = null + +# Parses CSS selectors and memoizes their validity so each selector will only +# be parsed once. +exports.isSelectorValid = (selector) -> + selectorCache ?= {} + cachedValue = selectorCache[selector] + return cachedValue if cachedValue? + + testElement ?= document.createElement('div') + try + # querySelector appears to be faster than webkitMatchesSelector + # http://jsperf.com/query-vs-matches + testElement.querySelector(selector) + selectorCache[selector] = true + true + catch selectorError + selectorCache[selector] = false + false + +# Parse the given CSS selector and throw an error if it is invalid. +exports.validateSelector = (selector) -> + return if exports.isSelectorValid(selector) + + error = new Error("'#{selector}' is not a valid selector") + error.code = 'EBADSELECTOR' + throw error