Merge pull request #5828 from atom/catch-selector-errors

Handle invalid CSS selectors in menus and commands

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', ->

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()

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"
+    ]
+  }
+}

spec/fixtures/packages/package-with-invalid-context-menu/menus/menu.json

@@ -0,0 +1,10 @@
+{
+  "context-menu": {
+    "<>": [
+      {
+        "label": "Hello",
+        "command:": "world"
+      }
+    ]
+  }
+}

spec/fixtures/packages/package-with-invalid-context-menu/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "package-with-invalid-context-menu",
+  "version": "1.0.0"
+}

spec/fixtures/packages/package-with-invalid-grammar/grammars/grammar.json

@@ -0,0 +1 @@
+><

spec/fixtures/packages/package-with-invalid-grammar/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "package-with-invalid-grammar",
+  "version": "1.0.0"
+}

spec/fixtures/packages/package-with-invalid-settings/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "package-with-invalid-settings",
+  "version": "1.0.0"
+}

spec/fixtures/packages/package-with-invalid-settings/settings/settings.json

@@ -0,0 +1 @@
+><

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", ->

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)

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)

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})

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})

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