Swapping two nsAutoTArrays shouldn't move them onto the heap

RESOLVED FIXED in mozilla9

Status

()

RESOLVED FIXED
7 years ago
7 months ago

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Tracking

(Blocks: 1 bug)

Trunk
mozilla9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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

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

Updated

7 years ago
Blocks: 681755
(Assignee)

Updated

7 years ago
Assignee: nobody → justin.lebar+bug
Created attachment 561197 [details] [diff] [review]
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.
(Assignee)

Updated

7 years ago
Attachment #561197 - Attachment is obsolete: true
Comment on attachment 561533 [details] [diff] [review]
Patch v1

...actually, this is pretty wrong.
Attachment #561533 - Flags: review-
Created attachment 561601 [details] [diff] [review]
Patch v3

I think this one might be right.
(Assignee)

Updated

7 years ago
Attachment #561533 - Attachment is obsolete: true
Created attachment 561638 [details] [diff] [review]
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?
Attachment #561638 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #561601 - Attachment is obsolete: true
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.
> Maybe it's a bit perverse, but why not use nsAutoTArray<PRUint8,8192> temp; 

Awesome!  I *have* to do this.
Created attachment 561648 [details] [diff] [review]
Patch v5
Attachment #561648 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #561638 - Attachment is obsolete: true
Attachment #561638 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Blocks: 688532
(Assignee)

Updated

7 years ago
Summary: Make swapping two nsAutoTArrays work → Swapping two nsAutoTArrays shouldn't move them onto the heap
(Assignee)

Updated

7 years ago
Duplicate of this bug: 678019
https://hg.mozilla.org/mozilla-central/rev/51ca12bc0c44
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Version: unspecified → Trunk
Depends on: 803641
Depends on: 1473804
You need to log in before you can comment on or make changes to this bug.