Field names array in ArrayClass::generic_sortOn defeats reference counting

RESOLVED FIXED in Q3 11 - Serrano

Status

Tamarin
Virtual Machine
P2
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Lars T Hansen, Assigned: Lars T Hansen)

Tracking

(Blocks: 2 bugs)

unspecified
Q3 11 - Serrano
Dependency tree / graph

Details

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
The code is constructing a heap-allocated array that's an implicit subclass of GCObject, with each element of the type FieldName:

        struct FieldName
        {
            RCObject::GCMember<String> name;
            int options;
        };

(That could have been a GCInlineObject but that would not have changed the point of this bug.)

The names will correctly have their reference counts incremented upon being stored in the array, but because the array's destructor is never called (it's a GCObject) the names' reference counts will not be decremented when the array is GC'd.  As a consequence, any names participating can only ever be reclaimed by garbage collection.

This is one instance where a disciplined "GCArray" type would probably have helped us write correct code more easily.
(Assignee)

Comment 1

7 years ago
Indeed it would appear that the field name array should be allocated by means of VMPI_alloca.  There's some cleanup to do; ~ArraySort explicitly deletes the field names array, but there's really no need for that kind of song and dance so far as I can see.
(Assignee)

Updated

7 years ago
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 2

7 years ago
Created attachment 530254 [details] [diff] [review]
Convert to VMPI_alloca
Attachment #530254 - Flags: review?(edwsmith)
(Assignee)

Updated

7 years ago
Priority: -- → P2
Whiteboard: has-patch
Target Milestone: --- → Q3 11 - Serrano
(Assignee)

Updated

7 years ago
Blocks: 654946
(Assignee)

Comment 3

7 years ago
(In reply to comment #2)
> Created attachment 530254 [details] [diff] [review] [review]
> Convert to VMPI_alloca

When landing this it will also be good to note, in the definition of ArraySort, that ArraySort is only supposed to be stack-allocated (it's the only way using VMPI_alloca for the field names array makes sense, lifetime-wise).
(Assignee)

Comment 4

7 years ago
Comment on attachment 530254 [details] [diff] [review]
Convert to VMPI_alloca

Redirecting "R?".
Attachment #530254 - Flags: review?(edwsmith) → review?(stejohns)

Comment 5

7 years ago
Comment on attachment 530254 [details] [diff] [review]
Convert to VMPI_alloca

Review of attachment 530254 [details] [diff] [review]:
-----------------------------------------------------------------

Scope creep: we should totally add a "VMPI_calloca" call to avoid the need for an explicit overflow check. (And then maybe deprecate VMPI_alloca.)
Attachment #530254 - Flags: review?(stejohns) → review+

Comment 6

7 years ago
changeset: 6295:d48515716429
user:      Lars T Hansen <lhansen@adobe.com>
summary:   Fix 654932 - Field names array in ArrayClass::generic_sortOn defeats reference counting (r=stejohns)

http://hg.mozilla.org/tamarin-redux/rev/d48515716429
(Assignee)

Updated

7 years ago
Whiteboard: has-patch → fixed-in-tr
(Assignee)

Comment 7

7 years ago
lhansen-MacPro:tamarin-redux-serrano lhansen$ hg tip
changeset:   6241:f224b9f7e225
tag:         tip
user:        Lars T Hansen <lhansen@adobe.com>
date:        Tue May 10 09:45:02 2011 +0200
summary:     Fix 654932 - Field names array in ArrayClass::generic_sortOn defeats reference counting (r=stejohns)
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-tr
You need to log in before you can comment on or make changes to this bug.