Closed Bug 687722 Opened 13 years ago Closed 13 years ago

Swapping two nsAutoTArrays shouldn't move them onto the heap

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

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

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

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

This is obviously problem if you want to swap into an auto array.
Blocks: 681755
Assignee: nobody → justin.lebar+bug
Attached patch WIP v1 (obsolete) — Splinter Review
This seems to work, but I'm skeptical it's right.  Pushed to try and going to figure out how to write tests.
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #561197 - Attachment is obsolete: true
Comment on attachment 561533 [details] [diff] [review]
Patch v1

...actually, this is pretty wrong.
Attachment #561533 - Flags: review-
Attached patch Patch v3 (obsolete) — Splinter Review
I think this one might be right.
Attachment #561533 - Attachment is obsolete: true
Attached patch Patch v4 (obsolete) — Splinter Review
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)
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.
Attached patch Patch v5Splinter Review
Attachment #561648 - Flags: review?(roc)
Attachment #561638 - Attachment is obsolete: true
Attachment #561638 - Flags: review?(roc)
Summary: Make swapping two nsAutoTArrays work → Swapping two nsAutoTArrays shouldn't move them onto the heap
https://hg.mozilla.org/mozilla-central/rev/51ca12bc0c44
Status: NEW → RESOLVED
Closed: 13 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.