On Fri, Dec 23, 2016 at 2:03 AM, smaug <sm...@welho.com> wrote:
> On 12/22/2016 08:14 PM, Bobby Holley wrote:
>
>> We've had this debate several times already, culminating in the attempt to
>> ban NS_ENSURE_* macros. It didn't work.
>>
>> This is a bit different. One of the most common NS_ENSURE_ macros is
> SUCCESS variant which explicitly has the
> return value.
> MOZ_TRY doesn't really hint at all, not even in its name, that it is doing
> early return.
>
To an unfamiliar reader, I don't think the name NS_ENSURE_SUCCESS provides
significantly more clarity than MOZ_TRY. Both suggest that there might be
something funny going on, but readers need to know the convention or look
it up. This is a cost that we have to weigh against the benefit that new
paradigms bring us.
The similarity to Rust helps us a bit here, just like the similarity of
MozPromise to ES Promises.
bholley
>
> But I've always like even NS_ENSURE_STATE, so I guess I could get used to
> code using Result, even though it at first sight looks rather verbose.
>
>
> 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
>
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
