The Add-ons Manager is a bit inconsistent in the way it throws exceptions: * throw Components.Exception(...) * throw new Error(...) * throw Components.results.whatever Ideally, it should always use Components.Exception. See bug 746908 for examples of it's usage.

I should have mentioned, the relevant files are under: * Backend: /toolkit/mozapps/extensions/ * Frontend: /toolkit/mozapps/extensions/content/ * Tests: /toolkit/mozapps/extensions/tests/ And some documentation on Components.Exception: https://developer.mozilla.org/en/Components.Exception

Congrats, Fraser - you're the owner of a bug :)

Fraser: There's a lot of small changes spread all around the Add-ons Manager for this, so when you're working on it, it'd be good if you could do it in multiple patches (split up as you see fit). That will make it a lot easier to track down issues, deal with bitrot, etc.

I noticed the following test: /toolkit/mozapps/extensions/test/unit/test_bug421396.js has a typo in several of the cases where it throws an exception. Instead of Components.results, it uses Components.errors (which doesn't exist). Would be good to fix that here too.

Comment on attachment 635356 [details] [diff] [review] toolkit/mozapps/extensions/AddonManager.jsm patch Review of attachment 635356 [details] [diff] [review]: ----------------------------------------------------------------- These exceptions are for missing arguments, so they should use Cr.NS_ERROR_INVALID_ARG as the error code (2nd argument). (Components.Exception defaults to the Cr.NS_ERROR_FAILURE error code, if you don't specify one.)

Comment on attachment 635357 [details] [diff] [review] toolkit/mozapps/extensions/addonManager.js patch Review of attachment 635357 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla-central/toolkit/mozapps/extensions/addonManager.js @@ +205,5 @@ > classID: Components.ID("{4399533d-08d1-458c-a87a-235f74451cfa}"), > _xpcom_factory: { > createInstance: function(aOuter, aIid) { > if (aOuter != null) > + throw Components.Exception("aOuter is not null", Cr.NS_ERROR_NO_AGGREGATION); Ideally, the error messages should describe why, not just what. The usual test used for that error (it's common) is: "Component does not support aggregation"

Hey Blair, As I'm still getting used to the source tree, where is the Components class kept? It looks like this is where the Components.Exception error code constants would be defined. This will make it a lot easier to specify the correct error code. Thanks!

Comment on attachment 635358 [details] [diff] [review] toolkit/mozapps/extensions/AddonRepository.jsm.patch Review of attachment 635358 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla-central/toolkit/mozapps/extensions/AddonRepository.jsm @@ +1563,5 @@ > this.initialized = false; > ERROR("Failed to open database", e); > if (aSecondAttempt || dbMissing) { > this.databaseOk = false; > + throw Components.Exception("Failed to open database: " + e); e is already an exception object, though it is nice to have this text prepended to it. e contains its error code (e.result), which should be passed to Components.Exception as the 2nd argument. @@ +1695,5 @@ > try { > return this.asyncStatementsCache[aKey] = this.connection.createAsyncStatement(sql); > } catch (e) { > ERROR("Error creating statement " + aKey + " (" + sql + ")"); > + throw Components.Exception("Error creating statement " + aKey + " (" + sql + "): " + e); Same as above.

Comment on attachment 635359 [details] [diff] [review] toolkit/mozapps/extensions/AddonUpdateChecker.jsm patch Review of attachment 635359 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla-central/toolkit/mozapps/extensions/AddonUpdateChecker.jsm @@ +291,5 @@ > try { > updateString = serializer.serializeResource(ds, extensionRes); > } > catch (e) { > + throw Components.Exception("Failed to generate signed string for " + aId + ". Serializer threw " + e); Can skip the "Serializer threw " part of the string (but still append e). Also, include e.result.

(In reply to blue from comment #11) > As I'm still getting used to the source tree, where is the Components class > kept? It looks like this is where the Components.Exception error code > constants would be defined. This will make it a lot easier to specify the > correct error code. Sadly, I don't think there's one single place in the source for all error codes :\ But they're all part of Components.results (we usually set the constant Cr to that object - you'll want to do that at the top of extensions.js when you get to it). This is many of them: https://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsError.h#139 There's also: http://james-ross.co.uk/mozilla/misc/nserror_list (But I can't vouch for it's accuracy.) alternatively, if you open the Add-ons Manager, the open the devtools console for that tab, and enter: inspect(Components.results) You'll get a UI that lists the properties of that object (ie, all the error codes).

(In reply to Blair McBride (:Unfocused) from comment #13) > Comment on attachment 635359 [details] [diff] [review] > toolkit/mozapps/extensions/AddonUpdateChecker.jsm patch > > Review of attachment 635359 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mozilla-central/toolkit/mozapps/extensions/AddonUpdateChecker.jsm > @@ +291,5 @@ > > try { > > updateString = serializer.serializeResource(ds, extensionRes); > > } > > catch (e) { > > + throw Components.Exception("Failed to generate signed string for " + aId + ". Serializer threw " + e); > > Can skip the "Serializer threw " part of the string (but still append e). > Also, include e.result. There's also a second one in there where the function throws if the resource has a property that can't be serialized. Would the INVALID_ARG error code work for that one or just cause unnecessary confusion? ;)

(In reply to blue from comment #15) > There's also a second one in there where the function throws if the resource > has a property that can't be serialized. Would the INVALID_ARG error code > work for that one or just cause unnecessary confusion? ;) Hmm, grey area :) Lets just use the default error code for that.

Fraser: Haven't heard from you in awhile, any update here? Anything I can help with?

Landed the 3 finished parts to avoid bitrot. Landed them as one changeset on the fx-team branch, which will get merged into mozilla-central within a day or so: https://hg.mozilla.org/integration/fx-team/rev/1fbf3da47806 (To whoever merges this: Please leave this bug open, we're not done here yet.)

Hey Blair, Yep, still alive, just been very bogged down with things at work. New virtualization cluster design and battling with storage vendors about pricing for the past month. :) I've been poking at this when I have a chance and should be submitting several more patches very shortly! Fraser

Blair, just submitted three patches, one replacing the missing components you requested in AddonUpdateChecker.jsm (I've obsoleted the previous patch), and two new patches. More should be coming this evening and/or tomorrow when I have more time to upload and comment.

Great, thanks Fraser :) If there are any other files, could you put further changes into just one patch? Splitting up patches like this is mostly useful for when there would otherwise be a huge number of changes, but there generally seems to only be a few changes per file.

Comment on attachment 647255 [details] [diff] [review] replacement patch for AddonUpdateChecker.jsm Review of attachment 647255 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla-central/toolkit/mozapps/extensions/AddonUpdateChecker.jsm @@ +157,5 @@ > items.push(aIndent + "<em:" + prop + " NC:parseType=\"Integer\">" + > target.Value + "</em:" + prop + ">\n"); > } > else { > + throw Components.Exception("Cannot serialize unknown literal type", Cr.NS_ERROR_FAILURE); If the error code is Cr.NS_ERROR_FAILURE, you don't need to specify it - Components.Exception automatically defaults to that error code if you don't specify one (not specifying it when you don't need to makes these a bit easier to read, IMO).

Comment on attachment 647565 [details] [diff] [review] patch for toolkit/mozapps/extensions/test/browser/head.js Review of attachment 647565 [details] [diff] [review]: ----------------------------------------------------------------- r- to remove unnecessary cases of Cr.NS_ERROR_FAILURE. ::: mozilla-central/toolkit/mozapps/extensions/test/browser/head.js @@ +140,5 @@ > return aCallback.apply(null, args); > } > catch (e) { > info("Exception thrown: " + e); > + throw Components.Exception("Function log_exceptions threw an exception: " + e, e.result); This exception should be left alone - the function is used to wrap other function calls and log any exception, which should then pass through untouched.

Comment on attachment 647567 [details] [diff] [review] patch for toolkit/mozapps/extensions/test/browser/browser_openDialog.js Review of attachment 647567 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla-central/toolkit/mozapps/extensions/test/browser/browser_openDialog.js @@ +62,5 @@ > return CustomChromeProtocol.QueryInterface(aIID); > }, > > lockFactory: function BNPH_lockFactory(aLock) { > + throw Components.Exception("Function BNPH_lockFactory is not implemented", lockFactory is the function name that's exposed via the API, so the message should mention that, not BNPH_lockFactory.

Comment on attachment 647257 [details] [diff] [review] patch for toolkit/mozapps/extensions/content/extensions-content.js Review of attachment 647257 [details] [diff] [review]: ----------------------------------------------------------------- r- to remove unnecessary cases of Cr.NS_ERROR_FAILURE. ::: mozilla-central-new/toolkit/mozapps/extensions/content/extensions-content.js @@ +88,5 @@ > > // Resolve and validate urls > var url = this.resolveURL(item.URL); > if (!this.checkLoadURIFromScript(url)) > + throw Components.Exception("insufficient permissions to install: " + url, May as well fix the capitalization of the 'i' while you're here.

Comment on attachment 647256 [details] [diff] [review] patch for toolkit/mozapps/extensions/content/extensions.js Review of attachment 647256 [details] [diff] [review]: ----------------------------------------------------------------- r- to remove unnecessary cases of Cr.NS_ERROR_FAILURE. ::: mozilla-central-new/toolkit/mozapps/extensions/content/extensions.js @@ +2336,5 @@ > > show: function(aType, aRequest) { > if (!(aType in AddonManager.addonTypes)) > + throw Components.Exception("Attempting to show unknown type " + aType, > + Cr.NS_ERROR_INVALID_ARGS); Typo, should be Cr.NS_ERROR_INVALID_ARG

Oh, I keep forgetting to mention: When you attach a patch, there's a series of flags you can set on the attachment. You request review by setting the review flag to "?", and entering the reviewer's email address. That makes it *much* easier to keep track of reviews, and makes sure they don't get lost.

(In reply to Blair McBride (:Unfocused) from comment #34) > Comment on attachment 647565 [details] [diff] [review] > patch for toolkit/mozapps/extensions/test/browser/head.js > > Review of attachment 647565 [details] [diff] [review]: > ----------------------------------------------------------------- > > r- to remove unnecessary cases of Cr.NS_ERROR_FAILURE. > > ::: mozilla-central/toolkit/mozapps/extensions/test/browser/head.js > @@ +140,5 @@ > > return aCallback.apply(null, args); > > } > > catch (e) { > > info("Exception thrown: " + e); > > + throw Components.Exception("Function log_exceptions threw an exception: " + e, e.result); > > This exception should be left alone - the function is used to wrap other > function calls and log any exception, which should then pass through > untouched. I was going to ask about this one...it seemed ironic to alter the exception line of a function called log_exception. ;)

Hey Blair, I'm going through and making a bulk patch for the changes you requested from review and I noticed I missed declaring a variable to store Components.results in toolkit/mozapps/extensions/content/extensions-content.js. Because of this something else caught my eye: variables declared at the top in extensions-content.js are declared with 'let' rather than 'const'. Would you like me to change these? I know both let and const will declare variables that are block-scoped, but I thought let allowed changes to be made, which seems bad in this situation. Fraser

I had to ask around for why it was using 'let' in the first place :) Seems bug 668307 changed it to use 'let', and was just being over-cautious (and, I think, with a slight misunderstanding of how 'const' works, with regard to scopes). So, long story short: yes, it would be good to have that use 'const'. (As a fun fact: The spec says that 'const' is block-scoped, however our implementation far out-dates the spec and its currently function-scoped. See bug 611388. However that won't affect its usage here.)

Comment on attachment 656081 [details] [diff] [review] replacement bulk patch for earlier submissions in r- Review of attachment 656081 [details] [diff] [review]: ----------------------------------------------------------------- Looks good - thanks :) Is there anything else left?

There's a large amount of changes to be made to one of the other tests, but that shouldn't take long. There are a couple of minor changes to be made in other tests, but besides those I think we're done. I'll be finishing those up today.

Landed the most recent 3 finished patches: https://hg.mozilla.org/integration/fx-team/rev/393d50f1cdc5

Comment on attachment 657962 [details] [diff] [review] patch for /mozilla/toolkit/mozapps/extensions/test/unit/ Review of attachment 657962 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, you should ignore this entire directory - turns out we don't even use these tests. I filed bug 788416 earlier today (with a patch) to remove it from the tree.

Have you been able to make any more progress here, Fraser?

Unfortunately no. My full time job has been keeping me unbelievably busy over the past three weeks with a VMware upgrade and a new OSPF configuration for offsite VPNs. I'm hoping to be able to get back to this next week when things calm down a bit.

Ok - thanks for the update :) Let me know if/when there's anything I can help with.

Can you confirm that you're still working on this bug?

I would like to work on this bug. I will scan through the patches and the comments.

Amod, are you still interested in working on this bug?

(as discussed in #introduction) We probably need a summary of what work is left to be done on this bug, given that it is a good first bug which has been partially completed.

Lets close this bug, and continue any further work in bug 965651. Fresh info there.