m_slotDestroyInfo does not handle native slots, native slots need DWB's for proper DRC destruction

RESOLVED FIXED in flash10.2.x-Spicy

Status

Tamarin
Virtual Machine
P1
major
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Tommy Reilly, Assigned: Tommy Reilly)

Tracking

(Blocks: 2 bugs)

unspecified
flash10.2.x-Spicy
Dependency tree / graph
Bug Flags:
flashplayer-qrb +

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

8 years ago
we hit the "this is a native class" in computeSlotAreaAndSize and returns zero for both values, later on in buildSlotDestroyInfo we short circuit because these value are zero.   the TraitsBindings has 3 slots with 12 bytes for size correctly but these aren't the values used in buildSlotDestroyInfo.    This is for the Error class.  Not sure if this affects other native classes (like a subclasses of SpriteObject) that would be bad if it did.

Comment 1

8 years ago
That would indeed be bad. We should backtrack and see how far back this issue exists.
(Assignee)

Comment 2

8 years ago
So this affects any concrete instantiatons of native glue classes.   So Error is busted but a subclass of Error would be fine I think.    More study needed but from a quick investigtation of a flex app this means alot of the Event classes are getting no slot destroy info when they should be (since most Event classes are actual native classes instead of just AS3 subclasses of Event).
(Assignee)

Comment 3

8 years ago
Oh and its been this way since r 3100 when the slots stuff landed.
(Assignee)

Comment 4

8 years ago
I don't know what's going on in buildSlotDestroyInfo, based on my nascent understanding it should be building a bitmap for all slots including base classes but it appears to limit itself to its own slots.

buildSlotDestroyInfo pre rev 3100 looks more right to me.
(Assignee)

Comment 5

8 years ago
computeSlotAreaCountAndSize returns the size for all slots and slots area for everything except native slots, so its not appropriate to use those values in buildSlotDestroyInfo.     Its used to calculate the totalSize and we don't want to include native slots b/c they are included in the sizeofInstance.
(Assignee)

Comment 6

8 years ago
I'm trying to think of how to build a "verifier" that lives apart from buildSlotDestroyInfo, best I have so far is that the RC fields are declared with a special wrapper in debug builds that verifies in its ctor that its destroyslot info bit is set for the offset (this - findbeginning(this))

Comment 7

8 years ago
Hmm... thinking out loud here, maybe this is not-a-bug? Since the fields are declared with explicit DWB/DRCWB, we shouldn't need the slotdestroyinfo to reflect them, since the dtor's of the DWB fields should take care of things...
(Assignee)

Comment 8

8 years ago
They aren't declared that way, the setters use manual WB's

Comment 9

8 years ago
Doh! OK then, we have a real bug.

Comment 10

8 years ago
Looks like they used to be DWB's, but were changed here:

https://bugzilla.mozilla.org/show_bug.cgi?id=562761

but the claim there was that the slotDestroyInfo was correct...
After looking at the code in Traits.cpp I think the fix to https://bugzilla.mozilla.org/show_bug.cgi?id=562761 was not correct.  We either need to go back to putting DWB/DRCWB/ATOM_WB's into the generated structs for the slots, or we need to update computeSlotAreaCountAndSize to also compute slot area count and size for all classes, not just native ones.  I'd recommend updating computeSlotAreaCountAndSize to return slotAreaSizes, one for the size not included in getSizeOfInstance and another for the size of the slots included in getSizeOfInstance().
(In reply to comment #11)
> updating computeSlotAreaCountAndSize to return slotAreaSizes, one for the size
I meant "return two slotAreaSizes".
(Assignee)

Comment 13

8 years ago
Seems to me computeSlotAreaCountAndSize shouldn't have anything to do with building the slot destroy info, to build the slot destroy info we have to walk all the slot areas in each native class in the hierarchy setting bits for each NATIVE_CLASS_Slots section.     Or we have the bits represent slot index and not offset and lookup the offset in destroyInstance.

Traits.cpp@ rev 3099 looks right to me, just needs to be updated to properly calculate the offset
(Assignee)

Comment 14

8 years ago
reverting http://hg.mozilla.org/tamarin-redux/rev/fabb091254d1 is attractive but it doesn't help the new exact tracer
(Assignee)

Comment 15

8 years ago
hmm, it occurs to me that its more efficient to have the C++ compiler do this for us than to do it from a bitmap and safegc will want the naked pointers/atoms to be replaced with references anyways....
(In reply to comment #15)
> hmm, it occurs to me that its more efficient to have the C++ compiler do this

That is what the original slots patch did...
(In reply to comment #13)
> Seems to me computeSlotAreaCountAndSize shouldn't have anything to do with
> building the slot destroy info, to build the slot destroy info we have to walk
> all the slot areas in each native class in the hierarchy setting bits for each
> NATIVE_CLASS_Slots section.     Or we have the bits represent slot index and
> not offset and lookup the offset in destroyInstance.
> 
> Traits.cpp@ rev 3099 looks right to me, just needs to be updated to properly
> calculate the offset

You're right.  I forgot about the gaps between slot areas due to the native member vars.  Looking up the offset in destroyInstance seems potentially unpleasant since the GC may be doing the final collect.  Using offsets is also unfortunate since the bit array would have to be larger to have empty bits for all the native member vars.  The original solution of declaring the slots as DWB/DRCWB/ATOM_WB is definitely the most simple solution so far.

May be we should the destroy info of any Traits link to the destroy info of its base Traits.  Each destroy info would have a bit array for the local slots.  destroyInstance would then only need to compute the offset of the first slot before walking the bitmap.
(Assignee)

Comment 18

8 years ago
yeah I just got buy from Lars to revert to using ATOM_WB and DWBRC, buildDestroySlotInfo can stay simple, I'll post a patch to update it with comments about what it is and isn't doing

Comment 19

8 years ago
Presumably this is just a mod of nativegen.py; please push it back to me if (a) that's the case and (b) you don't have cycles to fix it soonish.
Assignee: nobody → treilly
Blocks: 576307
Priority: -- → P1
Target Milestone: --- → flash10.x - Serrano
(Assignee)

Comment 20

8 years ago
Created attachment 482880 [details] [diff] [review]
use DWBRC and ATOM_WB on slots
Attachment #482880 - Flags: superreview?(edwsmith)
Attachment #482880 - Flags: review?(stejohns)
(Assignee)

Comment 21

8 years ago
Created attachment 482882 [details] [diff] [review]
update builtins
Attachment #482882 - Flags: superreview?(edwsmith)
Attachment #482882 - Flags: review?(stejohns)
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 22

8 years ago
Created attachment 482884 [details] [diff] [review]
update comments and use more descriptive names
Attachment #482884 - Flags: superreview?(edwsmith)
Attachment #482884 - Flags: review?(stejohns)

Comment 23

8 years ago
Comment on attachment 482880 [details] [diff] [review]
use DWBRC and ATOM_WB on slots

What's the deal with the commented-out section at the end? If it's dead we should remove it, or at least provide a comment explaining why it's commented out...

Updated

8 years ago
Attachment #482884 - Flags: review?(stejohns) → review+

Updated

8 years ago
Attachment #482882 - Flags: review?(stejohns) → review+
(Assignee)

Comment 24

8 years ago
commented out section will be removed before landing

Updated

8 years ago
Attachment #482880 - Flags: review?(stejohns) → review+
(Assignee)

Updated

7 years ago
Summary: m_slotDestroyInfo not built correctly for native classes → m_slotDestroyInfo does not handle native slots, native slots need DWB's for proper DRC destruction

Updated

7 years ago
Flags: flashplayer-qrb+
Target Milestone: flash10.x - Serrano → flash10.2.x-Spicy
(Assignee)

Updated

7 years ago
Blocks: 597577
(Assignee)

Comment 25

7 years ago
Comment on attachment 482882 [details] [diff] [review]
update builtins

Remove builtins patch, it needs to be re-based, prolly don't need review on generated code anyways.   The re-gen'd builtins will be pushed when I push the new nativegen script.
Attachment #482882 - Attachment is obsolete: true
Attachment #482882 - Flags: superreview?(edwsmith)
Attachment #482882 - Flags: review+

Comment 26

7 years ago
Comment on attachment 482880 [details] [diff] [review]
use DWBRC and ATOM_WB on slots

the commented out code at the bottom (for slot in sortedSlots) should point to a bug # and explain why we're keeping it, or just be nuked.
Attachment #482880 - Flags: superreview?(edwsmith) → superreview+

Updated

7 years ago
Attachment #482884 - Flags: superreview?(edwsmith) → superreview+
(Assignee)

Comment 27

7 years ago
http://hg.mozilla.org/tamarin-redux/rev/2ec0ea9c37c0
http://hg.mozilla.org/tamarin-redux/rev/cc3a48f6dc93
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(rev 5363:45a1857ac6c9 has a log msg that points to this bug, but it is misannotated and should instead point to Bug 596927.)
You need to log in before you can comment on or make changes to this bug.