Closed Bug 662851 Opened 9 years ago Closed 8 years ago

EnsureWritableCapacity should take a uint32_t

Categories

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

x86
All
defect

Tracking

(Not tracked)

VERIFIED FIXED
Q4 11 - Anza

People

(Reporter: treilly, Assigned: brbaker)

Details

Attachments

(3 files)

No description provided.
Attachment #538056 - Attachment is patch: true
Attachment #538056 - Flags: review?(stejohns)
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?)
Attachment #538056 - Flags: review?(stejohns) → review+
Grower is private inner class of ByteArray and there's no friending so I think its a contained issue
Attachment #538056 - Flags: superreview?(edwsmith)
changeset: 6382:3b80251e8145
user:      Tommy Reilly <treilly@adobe.com>
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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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.
Attachment #538056 - Flags: superreview?(edwsmith) → superreview+
Assignee: nobody → treilly
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: flashplayer-qrb+
Flags: flashplayer-injection-
Priority: -- → P3
Target Milestone: --- → Q4 11 - Anza
changeset: 6437:4d448a44169a
user:      Tommy Reilly <treilly@adobe.com>
summary:   Bug 662851 - Remove redundant cast found during post push review (r=edwsmith)

http://hg.mozilla.org/tamarin-redux/rev/4d448a44169a
changeset: 6438:42bc8f8b25bc
user:      Tommy Reilly <treilly@adobe.com>
summary:   Bug 662851 - Add some comments as requested in review

http://hg.mozilla.org/tamarin-redux/rev/42bc8f8b25bc
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);
  }
}
Assignee: treilly → nobody
Flags: in-testsuite-
Taking myself off this bug, hoping QE will come along and add an acceptance test for the ByteArray class length boundary conditions.
Assignee: nobody → brbaker
Flags: in-testsuite- → in-testsuite?
Attached patch testcaseSplinter Review
Attachment #547423 - Flags: review?
Attachment #547423 - Flags: review? → review?(treilly)
Attachment #547423 - Flags: review?(treilly) → review+
changeset: 6483:139ee1943775
user:      Brent Baker <brbaker@adobe.com>
summary:   Bug 662851: test boundary conditions at the biggest allowable byte array (r=treilly)

http://hg.mozilla.org/tamarin-redux/rev/139ee1943775
Attached patch buildbot tweaksSplinter Review
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.
Attachment #548181 - Flags: review?(jsudduth)
Attachment #548181 - Flags: review?(jsudduth) → review+
changeset: 6485:67830de8ef24
user:      Brent Baker <brbaker@adobe.com>
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
Status: REOPENED → RESOLVED
Closed: 9 years ago8 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.