907327 - Add mapping from XPCOM error codes (nsresult) to symbolic error names like PR_ErrorToString does for NSPR errors

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Add mozilla::GetErrorName(nsresult rv, /*out*/ nsACString & name);

This will be useful for improving error messages in tests. This kind of error mapping is already being done by the networking dashboard. I'm just proposing it to be moved to XPCOM.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Change the networking dashboard to use mozilla::GetErrorName

Comment 2

[Nathan Froyd [:froydnj]]

Brian, did you just not see this as useful and dropped it, or did you just decide to focus your efforts elsewhere, still seeing this as useful?

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Nathan Froyd (:froydnj) from comment #2)
> Brian, did you just not see this as useful and dropped it, or did you just
> decide to focus your efforts elsewhere, still seeing this as useful?

I think it would be helpful. This is one of the many bugs I dropped when I stopped working for MoCo. It would be great if somebody picked it up.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I may take a look at finishing this out in a bit.

Related to this, I noticed that js/xpconnect/src/xpc.msg maintains a mapping of (some) error codes to their symbolic names (plus some string explanations too).

Perhaps both use cases can be unified in some way.

Comment 5

[Nathan Froyd [:froydnj]]

Ryan, are you still looking at this?

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

No, I haven't done anything here yet.  Feel free to take it if you'd like to!

Comment 7

[Nathan Froyd [:froydnj]]

Anuj, is this something you would be interested in doing?

Comment 8

[Anuj Agarwal [:anujagarwal464]]

ok :)

Comment 9

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug792989-part1.diff

Comment 10

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug792989-part2.diff

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8507905 [details] [diff] [review]
bug792989-part1.diff

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

Looks good, just some small nits.

::: xpcom/base/ErrorNames.cpp
@@ +1,1 @@
> +#include "mozilla/ErrorNames.h"

This file needs a license header at the top, as well as appropriate editor modelines.  Please see:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line

@@ +1,2 @@
> +#include "mozilla/ErrorNames.h"
> +#include "mozilla/ArrayUtils.h"

Please alphabetically sort the header #includes.

@@ +2,5 @@
> +#include "mozilla/ArrayUtils.h"
> +#include "nsString.h"
> +#include "prerror.h"
> +
> +using namespace mozilla;

This bit shouldn't be necessary, since all the interesting things in this file are defined inside |namespace mozilla { }|.

@@ +26,5 @@
> +
> +namespace mozilla {
> +
> +void
> +GetErrorName(nsresult rv, /*out*/ nsACString & name)

No space between nsACString and &, please.  The /*out*/ comment is also unnecessary.

::: xpcom/base/ErrorNames.h
@@ +16,5 @@
> +// GetErrorName(NS_OK, name) will result in name == "NS_OK".
> +// When the symbolic name is unknown, name will be of the form
> +// "NS_ERROR_GENERATE_SUCCESS(<module>, <code>)" or
> +// "NS_ERROR_GENERATE_FAILURE(<module>, <code>)".
> +void GetErrorName(nsresult rv, /*out*/ nsACString & name);

Likewise with the |name| parameter here.

::: xpcom/base/moz.build
@@ +88,5 @@
>  
>  # nsDebugImpl isn't unified because we disable PGO so that NS_ABORT_OOM isn't
>  # optimized away oddly.
>  SOURCES += [
> +    'ErrorNames.cpp',

Please include this in UNIFIED_SOURCES instead of SOURCES.

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8507906 [details] [diff] [review]
bug792989-part2.diff

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

Looks good, just requires some small changes.

::: netwerk/base/src/Dashboard.cpp
@@ +914,3 @@
>          }
> +    }
> +    nsCString errorCString;

I think I'd make this an nsAutoCString.

::: xpcom/base/ErrorNames.cpp
@@ -26,5 @@
>  
>  namespace mozilla {
>  
>  void
> -GetErrorName(nsresult rv, /*out*/ nsACString & name)

This change can be removed once you have nsACString& in the previous patch.

::: xpcom/base/ErrorNames.h
@@ +16,5 @@
>  // GetErrorName(NS_OK, name) will result in name == "NS_OK".
>  // When the symbolic name is unknown, name will be of the form
>  // "NS_ERROR_GENERATE_SUCCESS(<module>, <code>)" or
>  // "NS_ERROR_GENERATE_FAILURE(<module>, <code>)".
> +void GetErrorName(nsresult rv, /*out*/ nsACString &name);

Likewise.

Comment 13

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug792989-part1.diff

Comment 14

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug792989-part2.diff

Comment 15

[Anuj Agarwal [:anujagarwal464]]

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=aeed80b316c4

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 8508015 [details] [diff] [review]
bug792989-part1.diff

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

::: xpcom/base/moz.build
@@ +40,5 @@
>  
>  EXPORTS += [
>      'CodeAddressService.h',
>      'ErrorList.h',
> +    'ErrorNames.h',

It looks like this should be exported in EXPORTS.mozilla according to your try build.

Comment 17

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug792989-part1.diff

Comment 18

[Anuj Agarwal [:anujagarwal464]]

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8858d81915d5

Comment 19

[Nathan Froyd [:froydnj]]

Comment on attachment 8508016 [details] [diff] [review]
bug792989-part2.diff

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

::: netwerk/base/src/Dashboard.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Please remove this line.

Comment 20

[Anuj Agarwal [:anujagarwal464]]

Attachment: bug792989-part2.diff

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3333c226a1d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/83f801de85fb

Comment 22

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #21)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3333c226a1d8
> https://hg.mozilla.org/integration/mozilla-inbound/rev/83f801de85fb

also relanded in
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9965ddd32fd6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2a1cd73f279c

sicne the inital commit had a wrong bug number in it

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9965ddd32fd6
https://hg.mozilla.org/mozilla-central/rev/2a1cd73f279c