On Tue, Aug 01, 2017 at 12:31:24PM -0400, Alexis Beingessner wrote:
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); ... 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:
The MFBT Vector class handles this with a set of infallibleAppend[1] methods that assert that space has been reserved for the new elements before they're called. If we're using this pattern elsewhere without the same safeties, either those places should probably switch to using Vectors, or we should probably add similar checked infallible methods to nsTArray.
[1]: http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/mfbt/Vector.h#707-733 _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
