Throw Component.exceptions instead of strings in contentprefs

RESOLVED FIXED in Firefox 60

Status

()

Toolkit
Preferences
RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: Yoric, Assigned: Jeffrey Tran, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(3 attachments, 10 obsolete attachments)

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)`.
Summary: Bug 1061521 - Throw Component.exceptions instead of strings in contentprefs → Throw Component.exceptions instead of strings in contentprefs

Comment 1

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

Comment 2

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

Comment 3

4 years ago
yes I have already build Mozilla Central.

Comment 4

4 years ago
Can you assign it to me?

Comment 5

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

Comment 6

4 years ago
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.)
Let's not touch the calls to `do_throw`, they are not important (they appear only in tests).

Comment 8

4 years ago
Created attachment 8487474 [details] [diff] [review]
Patch for bug-1064821
Attachment #8487474 - Flags: review?(dteller)

Comment 9

4 years ago
Created attachment 8487483 [details] [diff] [review]
Proposed Patch

Comment 10

4 years ago
Created attachment 8487492 [details] [diff] [review]
bug 1064821.patch
Attachment #8487492 - Flags: review?(dteller)

Comment 11

4 years ago
David: You asked me to take this, Is it still open?
Flags: needinfo?(dteller)
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.
Attachment #8487492 - Flags: review?(dteller) → feedback+
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.
Attachment #8487474 - Flags: review?(dteller) → feedback+
(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.
Flags: needinfo?(dteller)
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?
Flags: needinfo?(alucardgabriel1)

Comment 16

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

Comment 17

4 years ago
Created attachment 8488158 [details] [diff] [review]
Patch for bug-1064821

Changes made as per asked.
Attachment #8488158 - Flags: review?(dteller)
Flags: needinfo?(alucardgabriel1)
(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 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`.
Attachment #8488158 - Flags: review?(dteller) → feedback+

Comment 20

4 years ago
Created attachment 8488699 [details] [diff] [review]
Throw proper exceptions
Attachment #8487474 - Attachment is obsolete: true
Attachment #8488158 - Attachment is obsolete: true
Attachment #8488699 - Flags: review?(dteller)
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)
Attachment #8488699 - Flags: review?(dteller) → review+

Comment 22

4 years ago
Created attachment 8490545 [details] [diff] [review]
Throw proper exceptions

Removed the white space.
Attachment #8488699 - Attachment is obsolete: true
Attachment #8490545 - Flags: review?(dteller)
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?
Attachment #8490545 - Flags: review?(dteller)

Comment 24

4 years ago
Created attachment 8490671 [details] [diff] [review]
Proper Exceptions
Attachment #8490545 - Attachment is obsolete: true
Attachment #8490671 - Flags: review?(dteller)
Attachment #8487492 - Attachment is obsolete: true
Attachment #8487483 - Attachment is obsolete: true
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.
Attachment #8490671 - Flags: review?(dteller)

Comment 26

4 years ago
Created attachment 8490955 [details] [diff] [review]
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?
Attachment #8490671 - Attachment is obsolete: true
Attachment #8490955 - Flags: review?(dteller)
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.
Attachment #8490955 - Flags: review?(dteller) → review+
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

4 years ago
Created attachment 8491792 [details] [diff] [review]
Throw proper exceptions

Changes made.
Attachment #8490955 - Attachment is obsolete: true
Attachment #8491792 - Flags: review?(dteller)
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.
Attachment #8491792 - Flags: review?(dteller)

Comment 31

4 years ago
Created attachment 8493706 [details] [diff] [review]
Throw proper exceptions

I have added the author name.
Attachment #8491792 - Attachment is obsolete: true
Attachment #8493706 - Flags: review?(dteller)
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?
Attachment #8493706 - Flags: review?(dteller) → review+

Comment 33

4 years ago
Created attachment 8496544 [details] [diff] [review]
Throe proper exception
Attachment #8493706 - Attachment is obsolete: true
Attachment #8496544 - Flags: review?(dteller)
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.
Attachment #8496544 - Flags: review?(dteller)
Any news, Alucard?
Flags: needinfo?(alucardgabriel1)

Comment 36

3 years ago
Created attachment 8558624 [details] [diff] [review]
bug-1064821-fix.patch
Attachment #8558624 - Flags: review?(dteller)
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.
Attachment #8558624 - Flags: review?(dteller) → feedback+
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.
Flags: needinfo?(dteller)
This bug is not assigned and hasn't seen any activity in almost one year. Feel free to attach a patch.
Flags: needinfo?(dteller)
Flags: needinfo?(alucardgabriel1)
(Assignee)

Comment 40

2 years ago
Created attachment 8719627 [details] [diff] [review]
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.
Attachment #8719627 - Flags: review?(dteller)
(Assignee)

Updated

2 years ago
Assignee: nobody → tranj23
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?
Attachment #8719627 - Flags: review?(dteller) → review+
Flags: needinfo?(tranj23)
(Assignee)

Comment 42

2 years ago
(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
Flags: needinfo?(tranj23)
Yes, it needs to be pushed to the Try server. Let's wait for the results then land it.
What happened here? Is this okay to land?
Flags: needinfo?(dteller)
Sorry, the results are not visible anymore, we'd need to repush.
Flags: needinfo?(dteller)

Comment 47

3 months ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8880c81f9345
Throw proper exceptions in contentprefs. r=yoric

Comment 48

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8880c81f9345
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.