Add-ons Manager should use Components.Exception consistently when throwing exceptions

RESOLVED FIXED in mozilla18

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Unfocused, Assigned: blue)

Tracking

(Blocks: 1 bug)

Trunk
mozilla18
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=bmcbride@mozilla.com][lang=js])

Attachments

(7 attachments, 9 obsolete attachments)

1.14 KB, patch
Unfocused
: review+
Unfocused
: checkin+
Details | Diff | Splinter Review
661 bytes, patch
Unfocused
: review+
Unfocused
: checkin+
Details | Diff | Splinter Review
1.16 KB, patch
Unfocused
: review+
Unfocused
: checkin+
Details | Diff | Splinter Review
773 bytes, patch
Unfocused
: review+
Unfocused
: checkin+
Details | Diff | Splinter Review
1.63 KB, patch
Unfocused
: review+
Unfocused
: checkin+
Details | Diff | Splinter Review
13.05 KB, patch
Unfocused
: review+
Unfocused
: checkin+
Details | Diff | Splinter Review
9.15 KB, patch
Unfocused
: review-
Details | Diff | Splinter Review
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 :)
Assignee: nobody → blue
Status: NEW → ASSIGNED
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.
(Assignee)

Comment 5

6 years ago
Created attachment 635356 [details] [diff] [review]
toolkit/mozapps/extensions/AddonManager.jsm patch
(Assignee)

Comment 6

6 years ago
Created attachment 635357 [details] [diff] [review]
toolkit/mozapps/extensions/addonManager.js patch
(Assignee)

Comment 7

6 years ago
Created attachment 635358 [details] [diff] [review]
toolkit/mozapps/extensions/AddonRepository.jsm.patch
(Assignee)

Comment 8

6 years ago
Created attachment 635359 [details] [diff] [review]
toolkit/mozapps/extensions/AddonUpdateChecker.jsm patch
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.)
Attachment #635356 - Flags: review-
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"
Attachment #635357 - Flags: review-
(Assignee)

Comment 11

6 years ago
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.
Attachment #635358 - Flags: review-
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.
Attachment #635359 - Flags: review-
(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).
(Assignee)

Comment 15

6 years ago
(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? ;)
(Assignee)

Comment 16

6 years ago
Created attachment 635821 [details] [diff] [review]
updated AddonManager.jsm patch to reflect changes requested in comments
Attachment #635356 - Attachment is obsolete: true
(Assignee)

Comment 17

6 years ago
Created attachment 635823 [details] [diff] [review]
updated addonManager.js patch to reflect changes requested in comments
Attachment #635357 - Attachment is obsolete: true
(Assignee)

Comment 18

6 years ago
Created attachment 635825 [details] [diff] [review]
updated AddonRepository.jsm patch to reflect changes requested in comments
Attachment #635358 - Attachment is obsolete: true
(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.
Attachment #635825 - Flags: review+
Attachment #635823 - Flags: review+
Attachment #635821 - Flags: review+
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.)
Whiteboard: [good first bug][mentor=bmcbride@mozilla.com][lang=js] → [good first bug][mentor=bmcbride@mozilla.com][lang=js][leave open]
Attachment #635821 - Flags: checkin+
Attachment #635823 - Flags: checkin+
Attachment #635825 - Flags: checkin+
(Assignee)

Comment 22

6 years ago
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
(Assignee)

Comment 23

6 years ago
Created attachment 647255 [details] [diff] [review]
replacement patch for AddonUpdateChecker.jsm
Attachment #635359 - Attachment is obsolete: true
(Assignee)

Comment 24

6 years ago
Created attachment 647256 [details] [diff] [review]
patch for toolkit/mozapps/extensions/content/extensions.js
(Assignee)

Comment 25

6 years ago
Created attachment 647257 [details] [diff] [review]
patch for toolkit/mozapps/extensions/content/extensions-content.js
(Assignee)

Comment 26

6 years ago
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.
(Assignee)

Comment 28

6 years ago
Created attachment 647563 [details] [diff] [review]
patch for toolkit/mozapps/extensions/content/extensions.xml
(Assignee)

Comment 29

6 years ago
Created attachment 647564 [details] [diff] [review]
patch for toolkit/mozapps/extensions/content/setting.xml
(Assignee)

Comment 30

6 years ago
Created attachment 647565 [details] [diff] [review]
patch for toolkit/mozapps/extensions/test/browser/head.js
(Assignee)

Comment 31

6 years ago
Created attachment 647567 [details] [diff] [review]
patch for toolkit/mozapps/extensions/test/browser/browser_openDialog.js
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).
Attachment #647255 - Flags: review-
Attachment #647564 - Flags: review+
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.
Attachment #647565 - Flags: review-
Attachment #647563 - Flags: review+
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.
Attachment #647567 - Flags: review-
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.
Attachment #647257 - Flags: review-
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
Attachment #647256 - Flags: review-
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.
(Assignee)

Comment 39

6 years ago
(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. ;)
(Assignee)

Comment 40

6 years ago
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.)
(Assignee)

Comment 42

6 years ago
Created attachment 656081 [details] [diff] [review]
replacement bulk patch for earlier submissions in r-
Attachment #647255 - Attachment is obsolete: true
Attachment #647256 - Attachment is obsolete: true
Attachment #647257 - Attachment is obsolete: true
Attachment #647565 - Attachment is obsolete: true
Attachment #647567 - Attachment is obsolete: true
Attachment #656081 - Flags: review?(bmcbride)
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?
Attachment #656081 - Flags: review?(bmcbride) → review+
(Assignee)

Comment 44

6 years ago
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
Attachment #647563 - Flags: checkin+
Attachment #647564 - Flags: checkin+
Attachment #656081 - Flags: checkin+
(Assignee)

Comment 46

6 years ago
Created attachment 657962 [details] [diff] [review]
patch for /mozilla/toolkit/mozapps/extensions/test/unit/
Attachment #657962 - Flags: review?(bmcbride)
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.
Attachment #657962 - Flags: review?(bmcbride) → review-
Have you been able to make any more progress here, Fraser?
(Assignee)

Comment 50

6 years ago
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.

Comment 52

5 years ago
Can you confirm that you're still working on this bug?
Flags: needinfo?(blue)

Updated

5 years ago
Assignee: blue → nobody
Flags: needinfo?(blue)
Status: ASSIGNED → NEW

Comment 53

5 years ago
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?
Flags: needinfo?(amod.narvekar)
(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.
Flags: needinfo?(bmcbride)
Blocks: 965651
Lets close this bug, and continue any further work in bug 965651. Fresh info there.
Assignee: nobody → blue
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: needinfo?(bmcbride)
Flags: needinfo?(amod.narvekar)
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=bmcbride@mozilla.com][lang=js][leave open] → [good first bug][mentor=bmcbride@mozilla.com][lang=js]
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.