687722 - Swapping two nsAutoTArrays shouldn't move them onto the heap

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Justin Lebar (not reading bugmail)]

Currently the first thing nsTArray::SwapElements does is EnsureNotUsingAutoArray().

This is obviously problem if you want to swap into an auto array.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: WIP v1

This seems to work, but I'm skeptical it's right.  Pushed to try and going to figure out how to write tests.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 3

[Justin Lebar (not reading bugmail)]

Try builds: https://tbpl.mozilla.org/?tree=Try&rev=b6b915510084

Comment 4

[Justin Lebar (not reading bugmail)]

Comment on attachment 561533 [details] [diff] [review]
Patch v1

...actually, this is pretty wrong.

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3

I think this one might be right.

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: Patch v4

Linux results are in, and this is looking good on Try.

https://tbpl.mozilla.org/?tree=Try&rev=7c113a934f81

Roc, what do you think?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 561638 [details] [diff] [review]
Patch v4

Review of attachment 561638 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/glue/nsTArray-inl.h
@@ +253,5 @@
> +  // The IsAutoArrayRestorer RAII class makes sure that:
> +  //
> +  //   * mIsAutoArray has the same value as it did when we started, and
> +  //   * mHdr points to the auto buffer, if we have an auto buffer and mHdr
> +  //     would otherwise point to sEmptyHdr.

Most of this comment belongs in the IsAutoArrayRestorer class.

@@ +256,5 @@
> +  //   * mHdr points to the auto buffer, if we have an auto buffer and mHdr
> +  //     would otherwise point to sEmptyHdr.
> +
> +  mozilla::TArrayInternal::IsAutoArrayRestorer<Alloc> ourAutoRestorer(*this);
> +  mozilla::TArrayInternal::IsAutoArrayRestorer<Allocator> otherAutoRestorer(other);

would it not be simpler to just make IsAutoArrayRestorer a member of nsTArray_base?

@@ +317,5 @@
> +  // Don't alloca a huge buffer and smash the stack.  One of these two arrays
> +  // should be an auto array, so it's unlikely that the smaller one will be
> +  // huge.  But one could, in theory, declare a huge auto array on the heap.
> +  if (tempBytes <= 8192) {
> +    temp = alloca(tempBytes);

Maybe it's a bit perverse, but why not use nsAutoTArray<PRUint8,8192> temp; temp.SetLength(tempBytes) here? alloca seems unnecessary.

Comment 8

[Justin Lebar (not reading bugmail)]

> Maybe it's a bit perverse, but why not use nsAutoTArray<PRUint8,8192> temp; 

Awesome!  I *have* to do this.

Comment 9

[Justin Lebar (not reading bugmail)]

Attachment: Patch v5

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/51ca12bc0c44