662851 - EnsureWritableCapacity should take a uint32_t

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Tommy Reilly]

Attachment: use a uint32_t instead

Comment 1

[Steven Johnson]

Comment on attachment 538056 [details] [diff] [review]
use a uint32_t instead

all callers pass a uint32_t, so this looks sound. (No callers in Flash-land I presume?)

Comment 2

[Tommy Reilly]

Grower is private inner class of ByteArray and there's no friending so I think its a contained issue

Comment 3

[Tamarin Bot]

changeset: 6382:3b80251e8145
user:      Tommy Reilly <[e-mail]>
summary:   Bug 662851 - Make EnsureWritableCapacity take uint32_t instead of uint64_t since all callers pass uint32_t (r=stejohns,sr-pending=ed)

http://hg.mozilla.org/tamarin-redux/rev/3b80251e8145

Comment 4

[Edwin Smith]

Comment on attachment 538056 [details] [diff] [review]
use a uint32_t instead

The first comparison in the function looks pretty sketchy either way -- it involves a size_t constant (GCHeap::kMaxObjectSize) and a uint32_t multiply).  Please add a comment explaining why its absolutely right.

>            if (newCapacity < minimumCapacity)
>               newCapacity = uint32_t(minimumCapacity);

The explicit uint32_t() cast here is now redundant, please fix.

are there any test cases that probe the boundary conditions at the biggest allowable byte array?  if not, please add, or create a bug to make sure someone is on the hook to add them.

Comment 5

[Tamarin Bot]

changeset: 6437:4d448a44169a
user:      Tommy Reilly <[e-mail]>
summary:   Bug 662851 - Remove redundant cast found during post push review (r=edwsmith)

http://hg.mozilla.org/tamarin-redux/rev/4d448a44169a

Comment 6

[Tamarin Bot]

changeset: 6438:42bc8f8b25bc
user:      Tommy Reilly <[e-mail]>
summary:   Bug 662851 - Add some comments as requested in review

http://hg.mozilla.org/tamarin-redux/rev/42bc8f8b25bc

Comment 7

[Tommy Reilly]

We need a test like this:

for(var i:uint32=0xffffc000;i<0x100000000;i+=4096)
{
    try {
     new ByteArray().length = i;
   } catch(e) {
    assert(e is MemoryErrorF);
  }
}

Comment 8

[Tommy Reilly]

Taking myself off this bug, hoping QE will come along and add an acceptance test for the ByteArray class length boundary conditions.

Comment 9

[Brent Baker]

Attachment: testcase

Comment 10

[Tamarin Bot]

changeset: 6483:139ee1943775
user:      Brent Baker <[e-mail]>
summary:   Bug 662851: test boundary conditions at the biggest allowable byte array (r=treilly)

http://hg.mozilla.org/tamarin-redux/rev/139ee1943775

Comment 11

[Brent Baker]

Attachment: buildbot tweaks

There are 2 things in this patch:

1) This testcase has internal logic to run specific tests based on if AVMFEATURE_64BIT vs AVMFEATURE_32BIT is defined, so this testcase needs to be skipped when doing 32vs64 differential testing.

2) Testcase will also cause the system malloc on mac32 to produce stderr (but not fail) so we need to skip it during an "AIR" configuration run, which is compiled with --enable-use-system-malloc.

Comment 12

[Tamarin Bot]

changeset: 6485:67830de8ef24
user:      Brent Baker <[e-mail]>
summary:   bug 662851: need to skip the testcase on a compile of test configurations, 32bit vs 64bit and mac32 when using system malloc (r=jsudduth)

http://hg.mozilla.org/tamarin-redux/rev/67830de8ef24