Closed Bug 1366511 Opened 7 years ago Closed 7 years ago

Add helpers for mixing nsresult and mozilla::Result code

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(3 files)

As discussed on dev.platform[1]:

>I've been trying to write most of my new code to use Result.h as much as
>possible. When I have to mix Result-based code with nsresult-based code,
>though, things tend to get ugly, and I wind up writing helpers. At this point
>I've wound up writing the same helpers in 3 or 4 different places, so it may
>be time to try to standardize on something.
>
>The helpers I've been using look something like:
>
> template <>
> class MOZ_MUST_USE_TYPE GenericErrorResult<nsresult>
> {
>   nsresult mErrorValue;
>
>   template<typename V, typename E2> friend class Result;
>
> public:
>   explicit GenericErrorResult(nsresult aErrorValue) : mErrorValue(aErrorValue
>) {}
>
>   operator nsresult() { return mErrorValue; }
> };
>
> static inline Result<Ok, nsresult>
> WrapNSResult(PRStatus aRv)
> {
>     if (aRv != PR_SUCCESS) {
>         return Err(NS_ERROR_FAILURE);
>     }
>     return Ok();
> }
>
> static inline Result<Ok, nsresult>
> WrapNSResult(nsresult aRv)
> {
>     if (NS_FAILED(aRv)) {
>         return Err(aRv);
>     }
>     return Ok();
> }
>
> #define NS_TRY(expr) MOZ_TRY(WrapNSResult(expr))
>
>And their use looks something like:
>
> Result<nsCOMPtr<nsIFile>, nsresult>
> GetFile(nsIFile* aBase, const char* aChild)
> {
>   nsCOMPtr<nsIFile> file;
>   NS_TRY(aBase->Clone(getter_AddRefs(file)));
>   NS_TRY(aBase->AppendNative(aChild));
>
>   return Move(file);
> }
>
> nsresult
> ReadFile(const char* aPath)
> {
>   nsCOMPtr<nsIFile> file;
>   MOZ_TRY_VAR(file, GetFile(mBase, aPath));
>
>   PRFileDesc* fd;
>   NS_TRY(file->OpenNSPRFileDesc(PR_RDONLY, 0, &fd));
>
>   ...
>   return NS_OK;
> }
>
>Where NS_TRY converts a nsresult or PRStatus to an appropriate Result or
>GenericErrorResult, and the GenericErrorResult<nsresult> specialization
>automatically converts an nsresult when used in nsresult-based code.

[1]: https://groups.google.com/forum/#!topic/mozilla.dev.platform/dmUVEqU2xKg
Comment on attachment 8869774 [details]
Bug 1366511: Part 1 - Allow packing Result<T, nsresult> values into a single word.

https://reviewboard.mozilla.org/r/141324/#review145070

The implementation of this patch sounds fine to me.

The only concern that I have, is about the interpretation of NS_SUCCESS_* values as error values, which would be propagated as error by the MOZ_TRY unless we check the NS_FAILED/NS_SUCCEEDED test on the returned error value.

I am divided between accepting this patch as-is, knowing that this hides most of the uses cases of nsresult error handling behind MOZ_TRY macro, and asking a for a new iteration on this patch, such that we can specialize the isOk() check for nsresult, in which case returning a NS_SUCCESS_* should assert in debug builds when we have a Result<Ok, nsresult>.

If we were to accept this patch, then changing the semantic after, to distinguish between NS_SUCCESS_* and NS_ERROR_* might be even more dramatic as we might have converted more code to Result<Ok, nsresult> without this expectation.

Thus, I suggest we specialize ResultImplementation(E aErrorValue) as well, such that we refuse any Ok-like value from nsresult when the Return<Ok, nsresult> is constructed.
Attachment #8869774 - Flags: review?(nicolas.b.pierron) → review-
Comment on attachment 8869775 [details]
Bug 1366511: Part 2 - Allow autoconverting Err(nsresult) to nsresult.

https://reviewboard.mozilla.org/r/141326/#review145108

Nice!

The trick is in the GenericErrorResult class which adds a way to coerce it-self to its wrapping type.  Thus, when MOZ_TRY macro attempts to return a GenericErrorResult out of the Result<…, nsresult>, it can be unwrapped to a nsresult.

I cannot think of any pitfalls where this might bite us back, and I wonder if we should not do the same for all GenericErrorResult.

::: xpcom/base/nscore.h:239
(Diff revision 1)
> +  nsresult mErrorValue;
> +
> +  template<typename V, typename E2> friend class Result;
> +
> +public:
> +  explicit GenericErrorResult(nsresult aErrorValue) : mErrorValue(aErrorValue) {}

nit: assert NS_FAILED(aErrorValue).
Attachment #8869775 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8869776 [details]
Bug 1366511: Part 3 - Add mozilla::ToResult() to convert other result types to equivalent Result.

https://reviewboard.mozilla.org/r/141328/#review145116

mfbt/ResultExtensions.h is empty.

I guess this file contains the overload of ::mozilla::ToResult for nsresult inputs, shouldn't that go in xpcom/base/nscore.h as done in Part 1 for the UnusedZero specialization?
Attachment #8869776 - Flags: review?(nicolas.b.pierron) → review-
Comment on attachment 8869774 [details]
Bug 1366511: Part 1 - Allow packing Result<T, nsresult> values into a single word.

https://reviewboard.mozilla.org/r/141324/#review145554

::: xpcom/base/nscore.h:221
(Diff revision 1)
> +// When used as an error value, nsresult should never be NS_OK.
> +// This specialization allows us to pack Result<Ok, nsresult> into a
> +// nsresult-sized value.
> +template<typename T> struct UnusedZero;
> +template<>
> +struct UnusedZero<nsresult>

It would be more accurate to say that `nsresult` uses the high sign bit to represent whether the value was a success or failure. There are many more success values than just `NS_OK`. See http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/xpcom/base/ErrorList.py#113,172,174,176,178,233,238,254,421,424-425,430,468,477,479,484,515-516,529,532,536,538,549-557,594,596,681,686,691,699-700,713-714,856,878-881,883-885,928,1150,1154-1156,1164 for some examples of non-`NS_OK` success nsresult values.

I don't like that we're making the nsresult only check against NS_OK here, In my opinion we should make it check against NS_SUCCEEDED instead. At the very least we should not create an Err(NS_SUCCESS_AUTH_FINISHED) if you create a Result<Ok, nsresult> out of NS_SUCCESS_AUTH_FINISHED. I'd be OK with either Ok, or a MOZ_ASSERT.
Comment on attachment 8869774 [details]
Bug 1366511: Part 1 - Allow packing Result<T, nsresult> values into a single word.

https://reviewboard.mozilla.org/r/141324/#review146270

As others have said, the correct thing to do test here is the sign bit, not the zeroness.
Attachment #8869774 - Flags: review?(ehsan) → review-
Comment on attachment 8869774 [details]
Bug 1366511: Part 1 - Allow packing Result<T, nsresult> values into a single word.

https://reviewboard.mozilla.org/r/141324/#review146272
Comment on attachment 8869775 [details]
Bug 1366511: Part 2 - Allow autoconverting Err(nsresult) to nsresult.

https://reviewboard.mozilla.org/r/141326/#review146274
Attachment #8869775 - Flags: review?(ehsan) → review+
Comment on attachment 8869776 [details]
Bug 1366511: Part 3 - Add mozilla::ToResult() to convert other result types to equivalent Result.

https://reviewboard.mozilla.org/r/141328/#review146278

It's hard to review this patch without the header. ;-)
Attachment #8869776 - Flags: review?(ehsan)
Comment on attachment 8869776 [details]
Bug 1366511: Part 3 - Add mozilla::ToResult() to convert other result types to equivalent Result.

https://reviewboard.mozilla.org/r/141328/#review146278

Ugh. I always do that...
Sorry for the slow replies. It's been a very busy week.

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> The only concern that I have, is about the interpretation of NS_SUCCESS_*
> values as error values, which would be propagated as error by the MOZ_TRY
> unless we check the NS_FAILED/NS_SUCCEEDED test on the returned error value.
> 
> I am divided between accepting this patch as-is, knowing that this hides
> most of the uses cases of nsresult error handling behind MOZ_TRY macro, and
> asking a for a new iteration on this patch, such that we can specialize the
> isOk() check for nsresult, in which case returning a NS_SUCCESS_* should
> assert in debug builds when we have a Result<Ok, nsresult>.

I chose this approach because we should never store a success value as a
Result when nsresult is the error type, which means that if we assert that
elsewhere (as GenericErrorResult<nsresult> does), all we need to care about
here is that the value is non-zero.

That means that later, we should be able to add a new packing variant that
left rotates the error value, so we can use the LSB as the error bit and pack
the result code in the same word as a free-LSB success value.

But I think this is worth having sooner, without that optimization, rather
than delaying until I have the time to implement it.

> Thus, I suggest we specialize ResultImplementation(E aErrorValue) as well,
> such that we refuse any Ok-like value from nsresult when the Return<Ok,
> nsresult> is constructed.

I'd like to do that, but at the moment, providing specialized Result
implementations outside of Result.h is a bit tricky, and we can't actually use
the nsresult type in Result.h.

I'd like to do some more work on that in a follow-up patch, though.
(In reply to Michael Layzell [:mystor] from comment #7)
> It would be more accurate to say that `nsresult` uses the high sign bit to
> represent whether the value was a success or failure. There are many more
> success values than just `NS_OK`. See
> http://searchfox.org/mozilla-central/rev/
> 6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/xpcom/base/ErrorList.py#113,172,174,
> 176,178,233,238,254,421,424-425,430,468,477,479,484,515-516,529,532,536,538,
> 549-557,594,596,681,686,691,699-700,713-714,856,878-881,883-885,928,1150,
> 1154-1156,1164 for some examples of non-`NS_OK` success nsresult values.
> 
> I don't like that we're making the nsresult only check against NS_OK here,

This is only the case for the packing variant. The ToResult helpers check NS_FAILED, and the GenericErrorResult specialization asserts that we never try to store a success result as a failure.
(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> I cannot think of any pitfalls where this might bite us back, and I wonder
> if we should not do the same for all GenericErrorResult.

I was considering that too. The only thing that stopped me was that we may wind up using more generic types as failure values in some places, and I'm not sure what kind of footguns await if we implicitly convert an error result into a non-result type. At least with nsresult, the semantics are more or less the same as they are for autoconversion to an appropriate Result type.
Comment on attachment 8869774 [details]
Bug 1366511: Part 1 - Allow packing Result<T, nsresult> values into a single word.

https://reviewboard.mozilla.org/r/141324/#review179540

This way, the error constructor should assert that any NS_SUCCESS_* values are rejected, and all others are accepted as errors.

::: mfbt/Result.h:99
(Diff revision 2)
>    V unwrap() const { return V(); }
>    E& unwrapErr() const { return *mErrorValue; }
>  };
>  
> +template <typename V, typename E>
> +class ResultImplementation<V, E, PackingStrategy::NullIsOk>

Add a new packing strategy named "NegativeAsError", and use it for nsresult specialization.
Attachment #8869774 - Flags: review?(nicolas.b.pierron) → review-
Comment on attachment 8869774 [details]
Bug 1366511: Part 1 - Allow packing Result<T, nsresult> values into a single word.

https://reviewboard.mozilla.org/r/141324/#review179540

> Add a new packing strategy named "NegativeAsError", and use it for nsresult specialization.

NegativeAsError is not needed, because Err(nsresult) (starting in part 3) uses GenericErrorResult<nsresult> constructor, which asserts that the value is indeed a failure, as ToResult<nsresult> constructor coerce all NS_SUCCESS_* into the Ok() value.
Comment on attachment 8869774 [details]
Bug 1366511: Part 1 - Allow packing Result<T, nsresult> values into a single word.

https://reviewboard.mozilla.org/r/141324/#review179552
Attachment #8869774 - Flags: review- → review+
Comment on attachment 8869776 [details]
Bug 1366511: Part 3 - Add mozilla::ToResult() to convert other result types to equivalent Result.

https://reviewboard.mozilla.org/r/141328/#review179538

r=me without the pkix::Result namespace removal, as this changes belong to another patch.
From what I recall this prefix was added to avoid the confusion between pkix::Result and mozilla::Result.  Knowing the unified builds are playing against us on this topic, I would recommend to avoid the removal of pkix namespace.

::: security/certverifier/tests/gtest/CTTestUtils.cpp:761
(Diff revision 2)
>  public:
> -  Result GetCertTrust(EndEntityOrCA, const CertPolicyId&,
> +  pkix::Result GetCertTrust(EndEntityOrCA, const CertPolicyId&,
> -                      Input, TrustLevel&) override
> +                            Input, TrustLevel&) override
>    {
>      ADD_FAILURE();
> -    return Result::FATAL_ERROR_LIBRARY_FAILURE;
> +    return pkix::Result::FATAL_ERROR_LIBRARY_FAILURE;

Do not remove pkix::result namespace, for unified build reasons.  Or move these changes to a different patch.
Attachment #8869776 - Flags: review?(nicolas.b.pierron) → review+
(In reply to Nicolas B. Pierron [:nbp] from comment #22)
> r=me without the pkix::Result namespace removal, as this changes belong to
> another patch.
> From what I recall this prefix was added to avoid the confusion between
> pkix::Result and mozilla::Result.  Knowing the unified builds are playing
> against us on this topic, I would recommend to avoid the removal of pkix
> namespace.

I'm not sure what you mean. I'm not removing the pkix namespace anywhere, just adding namespace qualifiers where they're missing. They were already added for pkix::Result everywhere else, but were still missing from a few gtest files, which broke when I added the Result forward declaration to nscore.h due to ambiguity.
Comment on attachment 8869774 [details]
Bug 1366511: Part 1 - Allow packing Result<T, nsresult> values into a single word.

https://reviewboard.mozilla.org/r/141324/#review179852

::: mfbt/Result.h:99
(Diff revision 2)
>    V unwrap() const { return V(); }
>    E& unwrapErr() const { return *mErrorValue; }
>  };
>  
> +template <typename V, typename E>
> +class ResultImplementation<V, E, PackingStrategy::NullIsOk>

Please add a comment explaining why NullIsOk is the right packing strategy to use here.
Attachment #8869774 - Flags: review?(ehsan) → review+
Comment on attachment 8869776 [details]
Bug 1366511: Part 3 - Add mozilla::ToResult() to convert other result types to equivalent Result.

https://reviewboard.mozilla.org/r/141328/#review179858
Attachment #8869776 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/88629182740457e16a97c55bc2d99a2c94960566
Bug 1366511: Part 1 - Allow packing Result<T, nsresult> values into a single word. r=ehsan,nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ae47966e150a75c69cc6b691529aa8c7126565c
Bug 1366511: Part 2 - Allow autoconverting Err(nsresult) to nsresult. r=ehsan,nbp

https://hg.mozilla.org/integration/mozilla-inbound/rev/d75df8f79607ba713e4e41a78eb57e265c927856
Bug 1366511: Part 3 - Add mozilla::ToResult() to convert other result types to equivalent Result. r=nbp,ehsan
You need to log in before you can comment on or make changes to this bug.