Adding error checking to JsCreateWeakReference

[MSLaguana]

If a tagged value or a non-RecyclableObject is passed in,
we will now report an error rather than crashing non-deterministically
in a future GC.

[obastemur]

Js::TaggedNumber::Is check here is redundant.

if (!Js::RecyclableObject::Is(value))
{
#ifndef FLOATVAR
       if (!Js::TaggedInt::Is(value))
       { // is this really necessary?
            return JsErrorInvalidArgument;
       }
#endif
       return JsNoWeakRefRequired;
}

[digitalinfinity]

I think this is to distinguish the cases between Chakra-originating JsValueRefs (i.e RecyclableObjects and TaggedNumbers) vs. an actual invalid argument. In practice though, the caller would probably have to handle them the same but I could imagine the caller asserting on JsErrorInvalidArgument since that points to a bug in their code, vs. just doing nothing on JsNoWeakRefRequired

[obastemur]

My suggestion above doesn't oppose to what you are saying while it costs 1 call less?

[obastemur]

Implementation of Js::TaggedNumber:Is -> https://github.com/Microsoft/ChakraCore/blob/eb3f30b3f58bb8af2fa990301cab43ed1bf19451/lib/Runtime/Language/TaggedInt.inl#L307

[MSLaguana]

The comment there suggests that the definition of TaggedNumber::Is may change in the future. If you are concerned about the fact that we potentially call both Js::TaggedNumber::Is and !Js::RecyclableObject::Is which are currently the same, surely the compiler would be able to see that as well, while still being future-proof?

[obastemur]

...surely the compiler would be able to see that as well, while still being future-proof?

Not sure how it could be future proof. It entirely depends what we want to do with TaggedNumber. Actually, it is IMHO more vulnerable. What if TaggedNumber version 2 is recyclable?

I didn't understand, what this method has to do with TaggedNumber at the first place.

recycler->FindOrCreateWeakReferenceHandle

If upcoming value is not nullptr but !RecycalableObject, we say, this PTR, whatever is that, doesn't need this op. No?

[MSLaguana]

Currently if you pass in a tagged value rather than a real recyclable object, then there's an assert which would fire on debug builds, but in release builds it just corrupts the weak reference hash table by adding an entry with a null heap block pointer, and that causes a segfault in a future GC. That's why we want to add some kind of validation at the JSRT layer, so that proper usage of the JSRT with values created by the JSRT does not result in a crash.

[obastemur]

Currently if you pass in a tagged value rather than a real recyclable object, then there's an assert which would fire on debug builds...

Well, the reason is that PTR is not RecycalableObject right? And that's the thing we should make sure before calling it in?

[MSLaguana]

Right, but to a user of the JSRT they don't know whether a given JsValueRef is a RecyclableObject or not, it's opaque to them. The idea of the new error code is to inform them that while this might be a valid JS object, it's not something that they can/need to take a weak reference to.

[obastemur]

How the suggested change makes it more complicated for user?

Well, the reason is that PTR is not RecycalableObject right? And that's the thing we should make sure before calling it in?

Right...

If it's just for us, then what we care is !RecycalableObject.

regardless;
I'm not sure if we can guarantee it for numbers even now. I'm not at that part of the project yet (crawling slowly) but how about NumberObject? Indeed that's a recyclable one and has nothing directly to do with TaggedNumber but.. from the user's perspective.. no difference?

So, isn't it safer to say, dear user.. this particular variable is not subject to GC.. expectedly, we didn't do anything with it.. hence -> !RecycalableObject