1583666 - Consider adding `[[clang::reinitializes]]` annotation to nsTArray::Clear and ClearAndRetainStorage APIs

Status: NEW

(Core :: XPCOM, task)

Description

[Daniel Holbert [:dholbert]]

Right now, if you std::move() the contents of a nsTArray elsewhere and then try to use that array, then you'll run afoul of the bugprone-use-after-move static analysis check. This came up in some reviewbot comments in https://phabricator.services.mozilla.com/D46895

In practice (at least in that ^^ phabricator patch), I think this is actually fine, thought strictly speaking it could be not-fine if we changed the internals of nsTArray.

The documentation for this static-analysis check indicates that it "considers a variable to be reinitialized" (which then makes subsequent usage fine) if certain APIs are called, including clear() on some STL types, or "a member function marked with the [[clang::reinitializes]] attribute" on non-STL types. I think we should annotate our nsTArray "Clear()" and "ClearAndRetainStorage()" APIs with that clang attribute, so that they make subsequent usages guaranteed-to-be-fine after an earlier std::move() call.

Comment 1

[Daniel Holbert [:dholbert]]

It looks like this was previously discussed a bit in code-review here (in a case where we we're relying on std::move()-into-a-temporary leaving the original array empty, which is a reasonable assumption since the temporary starts out empty):
https://phabricator.services.mozilla.com/D36598?id=125973#inline-241409

CC'ing the folks involved in that discussion.

As a refresher, here's the code snippet in question (and note that mExpandedLineNames is an array-of-arrays here, with type nsTArray<SmallPointerArray<const NameList>> ):

    // NOTE(emilio): We rely on std::move clearing out the array.
    SmallPointerArray<const NameList> names;
    for (size_t i = 0; i < lineNameLists.Length(); ++i) {
      if (nameListToMerge) {
        names.AppendElement(nameListToMerge);
        nameListToMerge = nullptr;
      }
      names.AppendElement(&lineNameLists[i]);
      if (i >= mTrackListValues.Length()) {
        mExpandedLineNames.AppendElement(std::move(names));
        continue;
      }

https://searchfox.org/mozilla-central/rev/23f836a71cfe961373c8bd0d0219ec60a64b3c8f/layout/generic/nsGridContainerFrame.cpp#1218-1219,1222,1227

As you can see there, we std::move the contents of names into a temporary, and then we potentially use names on the next loop, assuming it's empty & usable. (This is basically fine, assuming that the nsTArray move-constructor is sane, but it does trip this bugprone-use-after-move clang-tidy check.)

Question: Do we want to establish a best-practice around this sort of thing, to avoid clang-tidy complaining & to keep ourselves honest about post-move() state?

Strawman suggestion: even though it's not strictly necessary, perhaps we should explicitly call names.Clear() or names.ClearAndRetainStorage() after we've std::move'd out its contents, and we should annotate those functions with [[clang::reinitializes]] so that clang-tidy is aware that it's safe to use after that point. This is an extra line of code and an extra step, but it should be cheap or free, and it will help develop good habits for cases where we e.g. do:

  somePreExistingArray = std::move(names);
  // I might expect 'names' to be empty here, but it's not necessarily empty!
  // Clear() would make my expectation hold up though.