1855796 - nsTArray::RemoveElementAtUnsafe() should have checks in debug builds

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Bug 1848714 changed nsTArray::RemoveElementAtUnsafe() from a private method to a public method, for use in places where we can be confident by inspection that the bounds check is redundant, and that profiling has indicated are performance sensitive.

However, this method lacks assertions, so not only are we not doing bounds checks in release builds, we are also not doing bounds checks in debug builds. This means that we can only be sure of catching buffer overflows in ASan builds, which is not great.

Maybe this can be done by turning RemoveElementAtUnsafe() back into a private method with a name like RemoveElementAtImpl(), and adding a new RemoveElementAtUnsafe() that is a wrapper around that with the appropriate assertions. Ideally this would share checking code with RemoveElement().

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1848714

:smaug, since you are the author of the regressor, bug 1848714, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Andrew McCreight [:mccr8]]

Oops, I thought I set it to "task" as this isn't exactly a user-facing defect.

Comment 3

[Andrew McCreight [:mccr8]]

I'll look at this I guess.

Comment 4

[Andrew McCreight [:mccr8]]

Oh right, it doesn't like regressions that are tasks.

Comment 5

[Andrew McCreight [:mccr8]]

It turns out that there are no external callers of RemoveElementsAtUnsafe(), so the simplest fix is to make it private again and just add a debug bounds check to RemoveElementAtUnsafe().

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Bug 1855796 - Add a debug check to nsTArray::RemoveElementAtUnsafe().

Also make RemoveElementsAtUnsafe() private again because there are no
external callers. If any external callers are added, then we probably
want them to be made to a different version that does checks in
debug builds.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a59a4fb2231
Add a debug check to nsTArray::RemoveElementAtUnsafe(). r=emilio

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/3a59a4fb2231