547782 - GCWeakRef would be handier if it remembered Atom types

Status: VERIFIED WONTFIX

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P3)

Description

[Steven Johnson]

GCWeakRef is handy for making weak references to objects where the type is known, but if you want a weak reference to an Atom, we lose the Atom type, limiting what the caller can safely do. It looks like we could (optionally) safely preserve Atom type in the lower three bits of GCWeakRef::m_obj, thus allowing for a safe WeakAtom.

Comment 1

[Steven Johnson]

FYI, the specific place this would be useful is in WeakKeyHashtable and WeakValueHashtable; currently we store the Weak values by masquerading them as kDoubleType Atoms. This is fine as long as the caller always know how to properly reconstitute the value to its original type, but doesn't allow for safely mixing multiple Atom types.

Comment 2

[Steven Johnson]

Bumping this for reconsideration, as this would have a nice cascade effect: it would allow us to remove the AvmPlusScriptableObject::toAtom() virtual method, which is needed in exactly two places (Sampler and WeakValueHashtable), and would also simplify some hashtable refactoring I'm experimenting with; basically, the ability to have a WeakRef properly remember (and reconstitue) an Atom would make life safer.

Comment 3

[Steven Johnson]

Attachment: WeakAtom implementation

Here's a rough attempt. Comments on general approach welcome; I tried to avoid modifying existing WeakRef code except as minimally necessary.

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

any self tests already made?  (or a reason that I should not attempt to make some myself?)

Comment 5

[Lars T Hansen]

Comment on attachment 446815 [details] [diff] [review]
WeakAtom implementation

Minor point, in GC::ClearWeakRef, the masking is unnecessary on this line:

itemptr = GetRealPointer((void*)(uintptr_t(itemptr) & ~7))

because itemptr has already been stripped of its tag.

More seriously the new comments in the MMgc files are incorrect and misleading.  The input value to these functions may not be an atom, it may only be an atom of a pointer type.  (Otherwise an integer atom and a pointer could clash, since the tag bits are ignored during comparison.)  This needs to be clarified and there needs to be guards in the GC routines that the tags are pointer tags.

In debug builds it's probably the case that we should check that the WeakAtom API is not being misused.

R- for above reasons but I don't see any fundamental problems with the approach.

Comment 6

[Steven Johnson]

(In reply to comment #4)
> any self tests already made?  (or a reason that I should not attempt to make
> some myself?)

No and no.

(In reply to comment #5)
> (From update of attachment 446815 [details] [diff] [review])
> Minor point, in GC::ClearWeakRef, the masking is unnecessary on this line:

Actually I think I added that as a result of finding such a case... but I don't recall where. I'll recheck and change to an assert if appropriate.

> More seriously the new comments in the MMgc files are incorrect and misleading.
>  The input value to these functions may not be an atom, it may only be an atom
> of a pointer type.  (Otherwise an integer atom and a pointer could clash, since
> the tag bits are ignored during comparison.)  This needs to be clarified and
> there needs to be guards in the GC routines that the tags are pointer tags.

Good catch. I'll do so and repost.

> In debug builds it's probably the case that we should check that the WeakAtom
> API is not being misused.

Yep.

Comment 7

[Steven Johnson]

(In reply to comment #5)
> This needs to be clarified and
> there needs to be guards in the GC routines that the tags are pointer tags.

I agree that the comments need to be clarified, but I think the checking needs to be done in WeakAtom itself (not in GC), as IMHO we want GC to remain as ignorant of specific Atom type meanings as possible. (IOW, it should preserve the tag bits but not otherwise attempt to interpret them.)

Comment 8

[Steven Johnson]

Attachment: WeakAtom v2

Tweaked per Lars suggestions (mostly).

Comment 9

[Lars T Hansen]

(In reply to comment #7)
> (In reply to comment #5)
> > This needs to be clarified and
> > there needs to be guards in the GC routines that the tags are pointer tags.
> 
> I agree that the comments need to be clarified, but I think the checking needs
> to be done in WeakAtom itself (not in GC), as IMHO we want GC to remain as
> ignorant of specific Atom type meanings as possible. (IOW, it should preserve
> the tag bits but not otherwise attempt to interpret them.)

I will consider your new patch in the morning, but generally speaking I do not agree with this.  The GC's ignorance of tags is not a feature, and this is an excellent case in point: error checking that could be done and which would add significant defense in depth can't be done where it must be done because of information hiding.  I've been hoping to come across a case that would force that issue to be resolved and I may just have found it :-)

Comment 10

[Steven Johnson]

(In reply to comment #9)
>  The GC's ignorance of tags is not a feature, and this is an
> excellent case in point

If there is already motivation to add this knowledge, then don't let me stand in the way -- it didn't strike me that this one tweak was worthy of dragging in said knowledge, but if that was already on the table...

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #6)
> (In reply to comment #4)
> > any self tests already made?  (or a reason that I should not attempt to make
> > some myself?)
> 
> No and no.

Hmm.  I started working on making a self-test (since I want to acquaint myself with that part of the infrastructure anyway).  But while reviewing existing GCWeakRef clients, I encountered bug 472852.  Here be tygers...

Comment 12

[Steven Johnson]

(In reply to comment #10)
> (In reply to comment #9)
> >  The GC's ignorance of tags is not a feature, and this is an
> > excellent case in point
> 
> If there is already motivation to add this knowledge, then don't let me stand
> in the way -- it didn't strike me that this one tweak was worthy of dragging in
> said knowledge, but if that was already on the table...

Lars: so would you like me to rework this patch with this in mind? It's not urgent but I would like to land this as groundwork for other work...

Comment 13

[Lars T Hansen]

My inclination was 'yes' but then I started thinking about AS2.  But then AS2 has tags in the high bits, does it not?  Mumble.

Comment 14

[Lars T Hansen]

I need to think about it but I'll attempt to get to it tomorrow.

Comment 15

[Lars T Hansen]

Here's what we should do.  We should not worry about which tags are pointer tags and which tags are not, but we should assert that if we 'get' a weak reference object then the tag bits must match.  That's good enough.  And your patch already does that.

Comment 16

[Steven Johnson]

http://hg.mozilla.org/tamarin-redux/rev/8207de3b1ce9

(Note that no code actually uses WeakAtom yet; some Hashtable changes I have in the works will do so soon.)

Comment 17

[Steven Johnson]

Reopening as further use reveals a flaw: this patch continues to allow for only one WeakRef to exist per object, but this can cause misbehavior if the same object is requested both with and without tag bits, triggering this assert:

            GCAssert(ref->get() == item); // all bits should match, including lower 3

Comment 18

[Steven Johnson]

Upon reflection, I may have a better solution for the problem I originally intended this for -- I might end up backing this out entirely, as fixing the semantic issue above may be nontrivial.

Comment 19

[Lars T Hansen]

(In reply to comment #18)
> Upon reflection, I may have a better solution for the problem I originally
> intended this for -- I might end up backing this out entirely, as fixing the
> semantic issue above may be nontrivial.

You might be able to fix it if you knew which tags were pointer tags, since you could allow a 0 tag to be used on items already tagged with a pointer tag, but that would break down when we get around to fixing something we need to fix: the integer tag needs to be '0'.

Comment 20

[Steven Johnson]

(In reply to comment #19)
> You might be able to fix it if you knew which tags were pointer tags, since you
> could allow a 0 tag to be used on items already tagged with a pointer tag, but
> that would break down when we get around to fixing something we need to fix:
> the integer tag needs to be '0'.

Yep. I've found an alternate way to solve my problem, so I'll be backing this out unless someone really wants it fixed.

Comment 21

[Steven Johnson]

Backed out as http://hg.mozilla.org/tamarin-redux/rev/9f861af18528

Comment 22

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 447124 [details] [diff] [review]
WeakAtom v2

(clearing self from feedback; I was inspired by this bug to try to make weakref selftests.  Its tricky.)

Comment 23

[Trevor Baker]

bulk verifying resolved !fixed issues