1064821 - Throw Component.exceptions instead of strings in contentprefs

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

The objective of this bug is to fix many of the throwing statements of directory toolkit/components/contentprefs (see http://dxr.mozilla.org/mozilla-central/search?q=throw+path%3Atoolkit%2Fcomponents%2Fcontentprefs&case=true ).

Generally, in JavaScript, you can call `throw foo` to throw an error, but if `foo` is not an instance of `Error`, this complicates debugging a lot. So, we want to look at all the calls to `throw foo` in that directory and make sure that we always throw instances of `Error`.

- wherever there is `throw "Some text"`, replace it with `throw new Error("Some text")`;
- wherever there is `throw Components.results.SOME_CONSTANT` or equivalently `throw Cr.SOME_CONSTANT`, replace it with `throw new Components.Exception(some message, Components.results.SOME_CONSTANT)`.

Comment 1

[Aman Singh]

David I want to work on this bug. Can you explain it.

Comment 2

[Mike Hoye [:mhoye]]

Aman: The first comment should be enough to go on - have you set up your build environment yet?

Comment 3

[Aman Singh]

yes I have already build Mozilla Central.

Comment 4

[Aman Singh]

Can you assign it to me?

Comment 5

[Mike Hoye [:mhoye]]

We assign patches to new contributors as soon as they get their first patch in; give it at try!

Comment 6

[alucardgabriel1]

David, how to throw an instance of an error if a do_throw function is used? ( do_throw() used in   http://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/tests/unit/test_bug503971.js#29 and other places too.)

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Let's not touch the calls to `do_throw`, they are not important (they appear only in tests).

Comment 8

[alucardgabriel1]

Attachment: Patch for bug-1064821

Comment 9

[Aman Singh]

Attachment: Proposed Patch

Comment 10

[Aman Singh]

Attachment: bug 1064821.patch

Comment 11

[vamsivarikuti]

David: You asked me to take this, Is it still open?

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8487492 [details] [diff] [review]
bug 1064821.patch

Review of attachment 8487492 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mostly good, but you should run the tests before asking for a review. Here, I'm pretty sure that test_contentPrefs.js is not going to work.
You can run these tests with
 ./mach xpcshell-test toolkit/components/contentprefs/tests/unit

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +218,5 @@
>    getPref: function ContentPrefService_getPref(aGroup, aName, aContext, aCallback) {
>      warnDeprecated();
>  
>      if (!aName)
> +      throw new Components.Exception("aName cannot be null or an empty string",

You can leave that line untouched. As it turns out `new Components.Exception(foo)` and `Components.Exception(foo)` are equivalent. Same below.

@@ +1181,1 @@
>              " to version " + aNewVersion);

Nit: Could you move the second line (" to version " ...) to align it with the quotes of the first line?

::: toolkit/components/contentprefs/tests/unit/test_contentPrefs.js
@@ +238,5 @@
>      interfaces: [Ci.nsIContentPrefObserver, Ci.nsISupports],
>    
>      QueryInterface: function ContentPrefTest_QueryInterface(iid) {
>        if (!this.interfaces.some( function(v) { return iid.equals(v) } ))
> +         throw new Components.Exception(no interface, Components.results.NS_ERROR_NO_INTERFACE);

You forgot quotes. Same below.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8487474 [details] [diff] [review]
Patch for bug-1064821

Review of attachment 8487474 [details] [diff] [review]:
-----------------------------------------------------------------

Good start.

::: toolkit/components/contentprefs/ContentPrefService2.jsm
@@ +823,5 @@
>  };
>  
>  function checkGroupArg(group) {
>    if (!group || typeof(group) != "string")
> +    throw new Error("domain must be nonempty string.");

You don't need to touch the `invalidArg` throws.

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +359,4 @@
>      }
>      catch(ex) {
>        this._dbConnection.rollbackTransaction();
> +      throw ex;//ex here itself is an instance of the error

That's good to know, but the comment is not useful.

@@ +1183,1 @@
>              " to version " + aNewVersion);

Could you indent the second line?

::: toolkit/components/contentprefs/tests/unit/head_contentPrefs.js
@@ +51,4 @@
>  
>    QueryInterface: function ContentPrefTest_QueryInterface(iid) {
>      if (!this.interfaces.some( function(v) { return iid.equals(v) } ))
> +      throw new Components.Exception(Cr.NS_ERROR_NO_INTERFACE);

You need a string as first argument for Components.Exception. Also below.

::: toolkit/components/contentprefs/tests/unit/test_contentPrefs.js
@@ +239,4 @@
>    
>      QueryInterface: function ContentPrefTest_QueryInterface(iid) {
>        if (!this.interfaces.some( function(v) { return iid.equals(v) } ))
> +        throw new Components.Exception(Cr.NS_ERROR_NO_INTERFACE);

As in head_contentPrefs.js.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

(In reply to vamsivarikuti from comment #11)
> David: You asked me to take this, Is it still open?

Apparently, two persons have already posted patches. Sorry about that.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Aman, Alucard, it seems that you are both posting patches on the same bug. It would probably be better if one of you could pick another bug. Aman, since Alucard posted the patch a before you (by just a few minutes), would you be interested in picking another bug?

Comment 16

[Aman Singh]

Ya sure. If you tell any other good bug for a newbie that would be of great help.

Comment 17

[alucardgabriel1]

Attachment: Patch for bug-1064821

Changes made as per asked.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

(In reply to Aman Singh from comment #16)
> Ya sure. If you tell any other good bug for a newbie that would be of great
> help.

Have you looked at http://www.joshmatthews.net/bugsahoy/?js=1&unowned=1&simple=1 ?

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8488158 [details] [diff] [review]
Patch for bug-1064821

Review of attachment 8488158 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there.
Can you apply the changes below?
Also, can you make sure that you follow the guidelines at https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F ? In particular, your commit message should be along the lines of "Bug 1064821 - Throw proper exceptions instead =in contentprefs;r=yoric"

::: toolkit/components/contentprefs/ContentPrefService2.jsm
@@ +818,4 @@
>        return this;
>      if (iid.equals(Ci.nsIContentPrefService))
>        return this._cps;
> +    throw new Components.Exception("No interface",Cr.NS_ERROR_NO_INTERFACE);

Nit: Could you add a whitespace after the ", "? Same in other places.

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +1179,4 @@
>        }
>      }
>      else
> +      throw new Error("no migrator function from version " + aOldVersion+ " to version " + aNewVersion);

That's not really what I said, but ok.

::: toolkit/components/contentprefs/tests/unit/head_contentPrefs.js
@@ +68,4 @@
>      // This causes extraneous errors to show up in the log when the directory
>      // service asks us first for CurProcD and MozBinD.  I wish there was a way
>      // to suppress those errors.
> +    throw new Components.Exception("Error in the log",Cr.NS_ERROR_FAILURE);

Instead of "Error in the log" (which doesn't mean anything), maybe `"Can't provide directory " + property`.

Comment 20

[alucardgabriel1]

Attachment: Throw proper exceptions

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8488699 [details] [diff] [review]
Throw proper exceptions

Review of attachment 8488699 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. Let's pass it through our tests:
https://tbpl.mozilla.org/?tree=Try&rev=a8b4891da8a0

::: toolkit/components/contentprefs/ContentPrefService2.jsm
@@ +818,4 @@
>        return this;
>      if (iid.equals(Ci.nsIContentPrefService))
>        return this._cps;
> +    throw new Components.Exception("No interface ",Cr.NS_ERROR_NO_INTERFACE);

Nit: Could you add a whitespace after the ","? (not just here)

Comment 22

[alucardgabriel1]

Attachment: Throw proper exceptions

Removed the white space.

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8490545 [details] [diff] [review]
Throw proper exceptions

Review of attachment 8490545 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see any change in your patch. Are you sure you didn't forget to commit/qref your changes?

Comment 24

[alucardgabriel1]

Attachment: Proper Exceptions

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8490671 [details] [diff] [review]
Proper Exceptions

Review of attachment 8490671 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see the new whitespaces I have requested.

::: toolkit/components/contentprefs/ContentPrefService2.jsm
@@ +818,4 @@
>        return this;
>      if (iid.equals(Ci.nsIContentPrefService))
>        return this._cps;
> +    throw new Components.Exception("No interface ",Cr.NS_ERROR_NO_INTERFACE);

I wanted a whitespace after the comma here.

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +69,4 @@
>        }
>        return this._contentPrefService2;
>      }
> +    throw new Components.Exception("No interface ",Cr.NS_ERROR_NO_INTERFACE);

And here.

@@ +219,4 @@
>      warnDeprecated();
>  
>      if (!aName)
> +      throw Components.Exception("aName cannot be null or an empty string ",

Please don't change this line here, it looks perfectly fine to me.

@@ +293,4 @@
>      warnDeprecated();
>  
>      if (!aName)
> +      throw Components.Exception("aName cannot be null or an empty string ",

Please don't change this line here, it looks perfectly fine to me.

@@ +1219,4 @@
>      if (aGroup instanceof Ci.nsIURI)
>        return this.grouper.group(aGroup);
>  
> +    throw Components.Exception("aGroup is not a string, nsIURI or null ",

Please don't change this line here, it looks perfectly fine to me.

::: toolkit/components/contentprefs/tests/unit/head_contentPrefs.js
@@ +51,4 @@
>  
>    QueryInterface: function ContentPrefTest_QueryInterface(iid) {
>      if (!this.interfaces.some( function(v) { return iid.equals(v) } ))
> +      throw new Components.Exception("No interface ",Cr.NS_ERROR_NO_INTERFACE);

I was expecting a whitespace here.

@@ +68,4 @@
>      // This causes extraneous errors to show up in the log when the directory
>      // service asks us first for CurProcD and MozBinD.  I wish there was a way
>      // to suppress those errors.
> +    throw new Components.Exception("Can't provide the directory ",Cr.NS_ERROR_FAILURE);

And here.

::: toolkit/components/contentprefs/tests/unit/test_contentPrefs.js
@@ +239,4 @@
>    
>      QueryInterface: function ContentPrefTest_QueryInterface(iid) {
>        if (!this.interfaces.some( function(v) { return iid.equals(v) } ))
> +        throw new Components.Exception("No interface ",Cr.NS_ERROR_NO_INTERFACE);

And here.

@@ +265,4 @@
>    
>      QueryInterface: function ContentPrefTest_QueryInterface(iid) {
>        if (!this.interfaces.some( function(v) { return iid.equals(v) } ))
> +        throw new Components.Exception("No interface ",Cr.NS_ERROR_NO_INTERFACE);

And here.

@@ +325,4 @@
>  
>        QueryInterface: function ContentPrefTest_QueryInterface(iid) {
>          if (!this.interfaces.some( function(v) { return iid.equals(v) } ))
> +          throw new Components.Exception("No interface ",Cr.NS_ERROR_NO_INTERFACE);

And here.

Comment 26

[alucardgabriel1]

Attachment: Spaces added

Sorry, I thought that I have to add white space before the closing double quotes. But now I have corrected that.

And David can you assign this bug to me?

Comment 27

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8490955 [details] [diff] [review]
Spaces added

Review of attachment 8490955 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, thanks for the patch.
Also, the tests appear good now, so all good with me.

Comment 28

[David Teller [:Yoric] - still alive but not very active]

Oh, one last thing: the patch itself doesn't quite follow our guidelines. Can you follow the steps at

https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

to make sure that we can actually land your patch?

Comment 29

[alucardgabriel1]

Attachment: Throw proper exceptions

Changes made.

Comment 30

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8491792 [details] [diff] [review]
Throw proper exceptions

Review of attachment 8491792 [details] [diff] [review]:
-----------------------------------------------------------------

You need to make sure that you have configured hg properly. In particular, it should show your name as the author of the patch.

Comment 31

[alucardgabriel1]

Attachment: Throw proper exceptions

I have added the author name.

Comment 32

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8493706 [details] [diff] [review]
Throw proper exceptions

Review of attachment 8493706 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. However, the code is unfortunately bitrotten, as someone has changed a little nsContentPrefService.js. Could you pull the latest version of mozilla-central and merge the changes?

Comment 33

[alucardgabriel1]

Attachment: Throe proper exception

Comment 34

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8496544 [details] [diff] [review]
Throe proper exception

Review of attachment 8496544 [details] [diff] [review]:
-----------------------------------------------------------------

I still have the same issue. Are you sure that you pulled the latest version from mozilla-central?
If you are using mercurial queues, here are the steps:

1. Return to a state before your patch, by using
 hg qpop -a
2. Pull and apply the latest version
 hg pull -u
3. Apply your patch
 hg qpush the_name_of_your_patch
4. This will tell you that there is a conflict in nsContentPrefService.js
5. In your editor, open the latest versions of nsContentPrefService.js and nsContentPrefService.js.rej. The former contains the code you are patching and the latter contains the part of your patch that cannot be applied anymore because of a conflict.
6. Fix nsContentPrefService.js.
7. Commit your changes
 hg qref
8. Upload them once again.

Comment 35

[David Teller [:Yoric] - still alive but not very active]

Any news, Alucard?

Comment 36

[sssarvjeet27]

Attachment: bug-1064821-fix.patch

Comment 37

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8558624 [details] [diff] [review]
bug-1064821-fix.patch

Review of attachment 8558624 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch. We'll want to leave a few of the calls to `throw` untouched, as they are special.

::: toolkit/components/contentprefs/ContentPrefService2.jsm
@@ +842,5 @@
>      if (supportedIIDs.some(function (i) iid.equals(i)))
>        return this;
>      if (iid.equals(Ci.nsIContentPrefService))
>        return this._cps;
> +    throw new Components.Exception("no interface", Components.results.NS_ERROR_NO_INTERFACE);

Let's not touch this `throw`, it is special. Same for the other calls to `throw Cr.NS_ERROR_NO_INTERFACE`.

::: toolkit/components/contentprefs/tests/unit/head_contentPrefs.js
@@ +67,5 @@
>  
>      // This causes extraneous errors to show up in the log when the directory
>      // service asks us first for CurProcD and MozBinD.  I wish there was a way
>      // to suppress those errors.
> +    throw new Components.Exception("error failure", Components.results.NS_ERROR_FAILURE);

Here too, it's a special case, leave it untouched.

Comment 38

[Anup Kumar [:reznord]]

I would like to take this bug and work on, because it looks similar to bug 1061521, to which I have submitted a patch following the comments.

Needinf'ing Yoric regarding the assignment of the bug.

Comment 39

[David Teller [:Yoric] - still alive but not very active]

This bug is not assigned and hasn't seen any activity in almost one year. Feel free to attach a patch.

Comment 40

[Jeffrey Tran]

Attachment: rev1 - Throw proper exceptions in contentprefs

Because we're ignoring "throw Cr.NS_ERROR_NO_INTERFACE" and "throw Cr.NS_ERROR_FAILURE" (comment 37), there are only 2 changes required.

After applying the patch, I ran the tests and 21/21 tests passed.

Comment 41

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8719627 [details] [diff] [review]
rev1 - Throw proper exceptions in contentprefs

Review of attachment 8719627 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay.
This patch looks good to me, thanks.
Do you need assistance for testing and landing?

Comment 42

[Jeffrey Tran]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #41)
> Comment on attachment 8719627 [details] [diff] [review]
> rev1 - Throw proper exceptions in contentprefs
> 
> Review of attachment 8719627 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay.
> This patch looks good to me, thanks.
> Do you need assistance for testing and landing?

Does this need to be pushed to the try server? I can do that myself. But I do need help to land it. Thanks

Comment 43

[David Teller [:Yoric] - still alive but not very active]

Yes, it needs to be pushed to the Try server. Let's wait for the results then land it.

Comment 44

[Jeffrey Tran]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c1f8316a0b1

Comment 45

[Mike Conley (:mconley) (:⚙️)]

What happened here? Is this okay to land?

Comment 46

[David Teller [:Yoric] - still alive but not very active]

Sorry, the results are not visible anymore, we'd need to repush.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8880c81f9345
Throw proper exceptions in contentprefs. r=yoric

Comment 48

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/8880c81f9345