1620632 - On older gcc, FallibleTArray<E> cannot be move-constructed from nsTArray<E>

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Simon Giesecke [:sg] [he/him]]

On older gcc versions such as in the base-toolchains build, FallibleTArray<E> cannot be move-constructed from nsTArray<E>. Apparently, it always attempts to copy, which fails to compile if E is not copyable. See, e.g. https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=ed7eaba537577cff4eca4306efafee753910d71f&selectedJob=292003927

A workaround is to replace

    aFiles->AppendElement(std::move(mCloneInfo.mFiles));

by

    FallibleTArray<mozilla::dom::indexedDB::StructuredCloneFile> temp;
    temp.SwapElements(mCloneInfo.mFiles);
    aFiles->AppendElement(std::move(temp));

in that case, but clang and newer gcc accept the former.

Comment 1

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1620632 - Added tests for moving nsTArray/AutoTArray/FallibleTArray with MoveOnly element type.

Comment 2

[Simon Giesecke [:sg] [he/him]]

https://bugzilla.mozilla.org/attachment.cgi?id=9132013 contains tests that reproduce the issue with the following error:

In file included from /home/simon/work/gcc-cov/xpcom/tests/gtest/TestTArray.cpp:7:0,
                 from Unified_cpp_xpcom_tests_gtest3.cpp:2:
/home/simon/work/gcc-cov/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h: In instantiation of 'static void nsTArrayElementTraits<E>::Construct(E*, A&&) [with A = const TestTArray::MoveOnly&; E = TestTArray::MoveOnly]':
/home/simon/work/gcc-cov/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:567:49:   required from 'static void AssignRangeAlgorithm<IsPod, IsSameType>::implementation(ElemType*, IndexType, SizeType, const Item*) [with Item = TestTArray::MoveOnly; ElemType = TestTArray::MoveOnly; IndexType = long unsigned int; SizeType = long unsigned int; bool IsPod = false; bool IsSameType = true]'
/home/simon/work/gcc-cov/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:2258:65:   required from 'void nsTArray_Impl<E, Alloc>::AssignRange(nsTArray_Impl<E, Alloc>::index_type, nsTArray_Impl<E, Alloc>::size_type, const Item*) [with Item = TestTArray::MoveOnly; E = TestTArray::MoveOnly; Alloc = nsTArrayFallibleAllocator; nsTArray_Impl<E, Alloc>::index_type = long unsigned int; nsTArray_Impl<E, Alloc>::size_type = long unsigned int]'
/home/simon/work/gcc-cov/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:2423:14:   required from 'nsTArray_Impl<E, Alloc>::elem_type* nsTArray_Impl<E, Alloc>::AppendElements(const Item*, nsTArray_Impl<E, Alloc>::size_type) [with Item = TestTArray::MoveOnly; ActualAlloc = nsTArrayFallibleAllocator; E = TestTArray::MoveOnly; Alloc = nsTArrayFallibleAllocator; nsTArray_Impl<E, Alloc>::elem_type = TestTArray::MoveOnly; nsTArray_Impl<E, Alloc>::size_type = long unsigned int]'
/home/simon/work/gcc-cov/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:1633:45:   required from 'nsTArray_Impl<E, Alloc>::elem_type* nsTArray_Impl<E, Alloc>::AppendElements(const nsTArray_Impl<Item, Allocator>&) [with Item = TestTArray::MoveOnly; Allocator = nsTArrayFallibleAllocator; ActualAlloc = nsTArrayFallibleAllocator; E = TestTArray::MoveOnly; Alloc = nsTArrayFallibleAllocator; nsTArray_Impl<E, Alloc>::elem_type = TestTArray::MoveOnly]'
/home/simon/work/gcc-cov/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:944:67:   required from 'nsTArray_Impl<E, Alloc>::nsTArray_Impl(const self_type&) [with E = TestTArray::MoveOnly; Alloc = nsTArrayFallibleAllocator; nsTArray_Impl<E, Alloc>::self_type = nsTArray_Impl<TestTArray::MoveOnly, nsTArrayFallibleAllocator>]'
/home/simon/work/gcc-cov/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:2572:25:   required from 'FallibleTArray<E>::FallibleTArray(const FallibleTArray<E>&) [with E = TestTArray::MoveOnly]'
/home/simon/work/gcc-cov/xpcom/tests/gtest/TestTArray2.cpp:452:38:   required from here
/home/simon/work/gcc-cov/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:534:5: error: use of deleted function 'TestTArray::MoveOnly::MoveOnly(const TestTArray::MoveOnly&)'
     new (static_cast<void*>(aE)) E(std::forward<A>(aArg));
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from Unified_cpp_xpcom_tests_gtest3.cpp:11:0:
/home/simon/work/gcc-cov/xpcom/tests/gtest/TestTArray2.cpp:302:3: note: declared here
   MoveOnly(const MoveOnly&) = delete;
   ^~~~~~~~

Comment 3

[Simon Giesecke [:sg] [he/him]]

/home/simon/work/gcc-cov/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:2572:25:   required from 'FallibleTArray<E>::FallibleTArray(const FallibleTArray<E>&) [with E = TestTArray::MoveOnly]'

This is already wrong. The template <class Allocator> FallibleTArray(nsTArray_Impl<E, Allocator>&& aOther) constructor overload should be selected IIUC, but for some reason it isn't.

Comment 4

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1620632 - Rename confusingly named types/macros for relocation handling of nsTArray. r=froydnj

Comment 5

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1620632 - Ensure nsTArray_Impl only declares a copy-constructor/assignment operator if E is copy-constructible. r=froydnj

Depends on D66243

Comment 6

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1620632 - Revert workaround for nsTArray move construction in indexedDB ActorsParent.cpp. r=#dom-workers-and-storage

Depends on D66081

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60498f2cc643
Rename confusingly named types/macros for relocation handling of nsTArray. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/481db8b264cb
Ensure nsTArray_Impl only declares a copy-constructor/assignment operator if E is copy-constructible. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/22d53a1fab04
Added tests for moving nsTArray/AutoTArray/FallibleTArray with MoveOnly element type. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/96f76b3ff56c
Revert workaround for nsTArray move construction in indexedDB ActorsParent.cpp. r=dom-workers-and-storage-reviewers,janv

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/60498f2cc643
https://hg.mozilla.org/mozilla-central/rev/481db8b264cb
https://hg.mozilla.org/mozilla-central/rev/22d53a1fab04
https://hg.mozilla.org/mozilla-central/rev/96f76b3ff56c

Comment 9

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1620632 - Fix move assignment of AutoTArray from other nsTArray. r=froydnj

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5de51a6ce5f2
Fix move assignment of AutoTArray from other nsTArray. r=froydnj

Comment 11

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/5de51a6ce5f2