Last Comment Bug 687722 - Swapping two nsAutoTArrays shouldn't move them onto the heap
: Swapping two nsAutoTArrays shouldn't move them onto the heap
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla9
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
: 678019 (view as bug list)
Depends on: 803641
Blocks: needs-more-autoarray 681755
  Show dependency treegraph
 
Reported: 2011-09-19 16:36 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-10-19 12:06 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP v1 (15.22 KB, patch)
2011-09-20 08:24 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v1 (18.73 KB, patch)
2011-09-21 11:42 PDT, Justin Lebar (not reading bugmail)
justin.lebar+bug: review-
Details | Diff | Splinter Review
Patch v3 (21.90 KB, patch)
2011-09-21 15:54 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v4 (22.40 KB, patch)
2011-09-21 18:34 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v5 (23.10 KB, patch)
2011-09-21 20:38 PDT, Justin Lebar (not reading bugmail)
roc: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2011-09-19 16:36:01 PDT
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) 2011-09-20 08:24:13 PDT
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.
Comment 2 Justin Lebar (not reading bugmail) 2011-09-21 11:42:04 PDT
Created attachment 561533 [details] [diff] [review]
Patch v1
Comment 3 Justin Lebar (not reading bugmail) 2011-09-21 11:42:33 PDT
Try builds: https://tbpl.mozilla.org/?tree=Try&rev=b6b915510084
Comment 4 Justin Lebar (not reading bugmail) 2011-09-21 12:55:22 PDT
Comment on attachment 561533 [details] [diff] [review]
Patch v1

...actually, this is pretty wrong.
Comment 5 Justin Lebar (not reading bugmail) 2011-09-21 15:54:36 PDT
Created attachment 561601 [details] [diff] [review]
Patch v3

I think this one might be right.
Comment 6 Justin Lebar (not reading bugmail) 2011-09-21 18:34:05 PDT
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?
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-09-21 19:26:41 PDT
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) 2011-09-21 19:32:32 PDT
> 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) 2011-09-21 20:38:19 PDT
Created attachment 561648 [details] [diff] [review]
Patch v5
Comment 10 Justin Lebar (not reading bugmail) 2011-09-22 14:35:01 PDT
*** Bug 678019 has been marked as a duplicate of this bug. ***
Comment 11 Ed Morley [:emorley] 2011-09-22 17:37:29 PDT
https://hg.mozilla.org/mozilla-central/rev/51ca12bc0c44

Note You need to log in before you can comment on or make changes to this bug.