We've had this debate several times already, culminating in the attempt to
ban NS_ENSURE_* macros. It didn't work.
Subjectively, I think the reasons for the failure were:
(a) We want logging/warnings on exceptional return paths, especially the
ones we don't expect.
(b) We want to check every potential failure and propagate error codes
correctly.
(c) Trying to do (a) and (b) consistently without macros bloats every
function call to 5 lines of boilerplate.
If we do want to rehash the early-return macro discussion, we should do
that separately. As it stands, MOZ_TRY is necessary to give mozilla::Result
feature parity with nsresult. We don't want people to stick with nsresult
just because they like NS_ENSURE_SUCCESS.
bholley
On Wed, Dec 21, 2016 at 11:22 PM, Eric Rahm <er...@mozilla.com> wrote:
> The key point for me is that we're hiding the return. I'm fine with the
> more verbose explicitly-return-and-make-it-easier-for-the-reviewer-to-
> call-out-issues
> form. I understand this is going to have differing opinions -- I think
> there's merit to more concise code -- but for the things I look at I much
> prefer an explicit return. It makes both the writer and reviewer think
> about the consequences. If MOZ_TRY just release asserted on error I
> wouldn't have an issue.
>
> On Wed, Dec 21, 2016 at 10:59 PM, Kris Maglione <kmagli...@mozilla.com>
> wrote:
>
> > On Wed, Dec 21, 2016 at 10:53:45PM -0800, Eric Rahm wrote:
> >
> >> I like the idea of pulling in some Rusty concepts, but I'm concerned
> about
> >> adding yet another early return macro -- absolutely no arguments on the
> >> new
> >> type, just the |MOZ_TRY| macro. In practice these have lead to security
> >> issues (UAF, etc) and memory leaks in the C++ world (I'm looking at you
> >> NS_ENSURE). These aren't hypothetical issues, I've run into them both
> >> working on memshrink and sec issues in Firefox.
> >>
> >
> > We run into the same issues without those macros. The only real
> difference
> > is that when you have to follow every call with:
> >
> > if (NS_WARN_IF(NS_FAILED(rv))) {
> > return rv;
> > }
> >
> > It's much easier to lose track of the allocation you did four calls ago
> > that's now 20 lines away.
> >
> >
> > On Wed, Dec 21, 2016 at 9:53 AM, Ted Mielczarek <t...@mielczarek.org>
> >> wrote:
> >>
> >> On Wed, Dec 21, 2016, at 12:30 PM, Jason Orendorff wrote:
> >>> > The implicit conversion solves a real problem. Imagine these two
> >>> > operations
> >>> > have two different error types:
> >>> >
> >>> > MOZ_TRY(JS_DoFirstThing()); // JS::Error&
> >>> > MOZ_TRY(mozilla::pkix::DoSecondThing()); // pkix::Error
> >>> >
> >>> > We don't want our error-handling scheme getting in the way of using
> >>> them
> >>> > together. So we need a way of unifying the two error types: a shared
> >>> base
> >>> > class, perhaps, or a variant.
> >>> >
> >>> > Experience with Rust says that MOZ_TRY definitely needs to address
> this
> >>> > problem somehow. C++ implicit conversion is just one way to go; we
> can
> >>> > discuss alternatives in the bug.
> >>>
> >>> The `try` macro in Rust will auto-convert error types that implement
> >>> `Into<E>`, AIUI, but that's not automatic for all error types. I
> haven't
> >>> tried it, but I have seen multiple recommendations for the
> `error_chain`
> >>> crate to make this smoother:
> >>> https://docs.rs/error-chain/0.7.1/error_chain/
> >>>
> >>> It's basically just boilerplate to implement conversions from other
> >>> error types. I wouldn't be surprised if something like that percolates
> >>> into the Rust standard library at some point.
> >>>
> >>> -Ted
> >>> _______________________________________________
> >>> dev-platform mailing list
> >>> dev-platform@lists.mozilla.org
> >>> https://lists.mozilla.org/listinfo/dev-platform
> >>>
> >>> _______________________________________________
> >> dev-platform mailing list
> >> dev-platform@lists.mozilla.org
> >> https://lists.mozilla.org/listinfo/dev-platform
> >>
> >
> > --
> > Kris Maglione
> > Firefox Add-ons Engineer
> > Mozilla Corporation
> >
> > It's always good to take an orthogonal view of something. It develops
> > ideas.
> > --Ken Thompson
> >
> >
> _______________________________________________
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
