Swapping two nsAutoTArrays shouldn't move them onto the heap

RESOLVED FIXED in mozilla9

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 1 bug)

Trunk
mozilla9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

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

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

Updated

6 years ago
Blocks: 681755
(Assignee)

Updated

6 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 1

6 years ago
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)

Comment 2

6 years ago
Created attachment 561533 [details] [diff] [review]
Patch v1
(Assignee)

Updated

6 years ago
Attachment #561197 - Attachment is obsolete: true
(Assignee)

Comment 3

6 years ago
Try builds: https://tbpl.mozilla.org/?tree=Try&rev=b6b915510084
(Assignee)

Comment 4

6 years ago
Comment on attachment 561533 [details] [diff] [review]
Patch v1

...actually, this is pretty wrong.
Attachment #561533 - Flags: review-
(Assignee)

Comment 5

6 years ago
Created attachment 561601 [details] [diff] [review]
Patch v3

I think this one might be right.
(Assignee)

Updated

6 years ago
Attachment #561533 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
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

6 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.
(Assignee)

Comment 8

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

Awesome!  I *have* to do this.
(Assignee)

Comment 9

6 years ago
Created attachment 561648 [details] [diff] [review]
Patch v5
Attachment #561648 - Flags: review?(roc)
(Assignee)

Updated

6 years ago
Attachment #561638 - Attachment is obsolete: true
Attachment #561638 - Flags: review?(roc)
Attachment #561648 - Flags: review?(roc) → review+
(Assignee)

Updated

6 years ago
Blocks: 688532
(Assignee)

Updated

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

Updated

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