I don't think that anyone deliberately set out to write the code this way.
Likely this is fallout from the mass-refactorings in bug 968520 and related
bugs. I'd recommend working with poiru and froydnj to see if there's any
automated follow-up we could do to remove/improve this pattern.
On Tue, Aug 1, 2017 at 12:31 PM, Alexis Beingessner <a.beingess...@gmail.com
> wrote:
> TL;DR: we're using fallible allocations without checking the result in
> cases where we're certain there is enough space already reserved. This is
> confusing and potentially dangerous for refactoring. Should we stop doing
> this?
>
> ---------
>
> I was recently searching through our codebase to look at all the ways we
> use fallible allocations, and was startled when I came across several lines
> that looked like this:
>
> dom::SocketElement &mSocket = *sockets.AppendElement(fallible);
>
> For those who aren't familiar with how our allocating APIs work:
>
> * by default we hard abort on allocation failure
> * but if you add `fallible` (or use the Fallible template), you will get
> back nullptr on allocation failure
>
> So in isolation this code is saying "I want to handle allocation failure"
> and then immediately not doing that and just dereferencing the result. This
> turns allocation failure into Undefined Behaviour, rather than a process
> abort.
>
> Thankfully, all the cases where I found this were preceded by something
> like the following:
>
> uint32_t length = socketData->mData.Length();if
> (!sockets.SetCapacity(length, fallible)) { JS_ReportOutOfMemory(cx);
> return NS_ERROR_OUT_OF_MEMORY;}for (uint32_t i = 0; i <
> socketData->mData.Length(); i++) { dom::SocketElement &mSocket =
> *sockets.AppendElement(fallible);
>
>
>
> So really, the fallible AppendElement *is* infallible, but we're just
> saying "fallible" anyway. I find this pattern concerning for two reasons:
>
> * It's confusing to anyone who encounters this for the first time.
> * It's a time bomb for any bad refactoring which makes the allocations
> *actually* fallible
>
> I can however think of two reasons why this might be desirable
>
> * It encourages the optimizer to understand that the allocations can't
> fail, removing branches (dubious, these branches would predict perfectly if
> they exist at all)
>
> * It's part of a guideline/lint that mandates only fallible allocations be
> used in this part of the code.
>
> If the latter is a concern, I would much rather we use infallible
> allocations with a required comment, or some other kind of decoration to
> state "this is fine". In the absence of a checker that can statically prove
> the the AppendElement calls will never fail (which in the given example
> would in fact warn that we don't use `length` in the loop condition), I
> would rather mistakes lead to us crashing more, rather than Undefined
> Behaviour.
>
> Thoughts?
> _______________________________________________
> 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
