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
[email protected]
https://lists.mozilla.org/listinfo/dev-platform
