Review of attachment 168743 [details] [review]:

Looks like a reasonable thing to do.

::: js/ui/search.js
@@ +264,3 @@
+                        results.push([provider, providerResults]);
+                } catch (error) {
+                    global.log("A " + error.name + " has occured: " + error.message);

Two minor issues:
 - "strings" are used to mark translatable strings,
   'strings' are used elsewhere; I don't think it is
   reasonable to translate error messages, but if you
   want to do it you need to use a format string
   (instead of the strings "A " and " has occured: ")
 - for debugging purposes provider.title would be
   useful here (more than error.name IMO)

Created attachment 168759 [details] [review]
Second go.

Fixes issues, also adds another try case for when things fail so badly provider is not defined, e.g. it does not implement one of the search functions.

Thanks for the feedback!

Review of attachment 168759 [details] [review]:

(In reply to comment #3)
> Fixes issues, also adds another try case for when things fail so badly provider
> is not defined, e.g. it does not implement one of the search functions.

That is wrong - a provider is only undefined if the extension explicitly passes an invalid object to Dash.addSearchProvider(). It makes more sense to handle the error there.

::: js/ui/search.js
@@ +268,3 @@
+                    } catch (error) {
+                        global.log ('A ' + error.name + ' has occured: ' + error.message + '. This likely means a search provider does not implement the function getSubsearchResultSet.');
+                    }

Eeek - nested exception handlers. This is pretty ugly in my opinion, and probably not too useful anyway. If you insist on handling the condition which triggers the exception, check against passing null to Dash.addSearchSystem().

Created attachment 170469 [details] [review]
Third go

Removed nested exceptions.

Review of attachment 170469 [details] [review]:

The patch is still slightly wrong, but easy to fix. I expect the next version to be good to go, so a short note on the commit message first: The body of the message could use a line break, so that the message fits a standard terminal window. There should be a blank line after the body, followed by the URL of this bug. Also, you probably want to check out git-bz[0], which adds support for bugzilla to git itself (trust me, it's completely awesome).

[0] http://blog.fishsoup.net/2008/11/16/git-bz-bugzilla-subcommand-for-git/

::: js/ui/search.js
@@ +259,3 @@
             for (let i = 0; i < this._previousResults.length; i++) {
+                try {
+                    let [provider, previousResults] = this._previousResults[i];

Hey, I finally understand where the nested exception handler was coming from! The way you initialize provider here, its scope is limited to the try{} block - accessing provider.title in the catch{} block throws an exception because provider is undefined there.

What you need to do is either declare the variable before the block - like:

let provider, previousResults;
try {
  [provider, previousResults] = ...

or move the entire initialization out of the block:

let [provider, previousResults] = this._previousResults[i];
try {
    ...

I don't see how an extension could make the second alternative throw an exception - so unless you can think of something, you are free to pick either of the alternatives here.

@@ +264,3 @@
+                        results.push([provider, providerResults]);
+                } catch (error) {
+                    global.log ('A ' + error.name + ' has occured in ' + provider.title + ': ' + error.message);

Sigh - logging is kind of messy. There are both log() and global.log() - the former writes to gnome-shell's stderr, while the latter appends to the "Errors" tab in looking glass. As far as I can tell, global.log() is only used with the extension system, everywhere else it's log(). The code here is intended for both build-in search providers and additional ones added by extensions, so you cannot avoid the risk of having messages end up in an unexpected place.

To be clear - I think you choice here is good, I just wanted to rant a little about our inconsistent logging :-)

@@ +270,3 @@
             for (let i = 0; i < this._providers.length; i++) {
+                try {
+                    let provider = this._providers[i];

Again, this has to be moved outside the try{} block.

Created attachment 170751 [details] [review]
Add basic error handling to search system.

If a search provider misbehaves, the search system fails to return any
results from any providers. Work around this.

Attachment 170751 [details] pushed as 4571a0e - Add basic error handling to search system.