Issue in setting accessor in PathTypeHandlerWithAttr

[akroshg]

When the property is deleted (say when we are in SimpleTypeHandler) and then later
if we create an accessor on the same property we have converted the type handler to PathTypeHandlerWithAttr.
But during setting the accessor on that slot we haven't removed the is_deleted attribute.
So when we try to get the property - we don't get that from the PathTypeHandlerWithAttr handler.
Fixed that by removing that attribute when we do SetAccessor.

[akroshg]

Please review commit 88f537b

fixes : #6271

[zenparsing]

Can this just be:

attr ^ ObjectSlotAttr_Deleted | ObjectSlotAttr_Accessor

?

Also, any idea why the already-merge commits are showing up in this PR? Maybe rebase?

[pleath]

The real problem here seems to be that we were able to create a property on a PathTypeHandler with the deleted attribute. That shouldn't be possible. Can you see how it happened?

[akroshg]

The object has SimpleTypeHandler<3> type handler (which has proto, length and name). Once we delete the length, we mark the attribute for that slot as deleted.
Around the time when we are about to add the accessor - we converted that handler to pathtypehandlerwithattr with the deleted property attribute for the 'length'.
At the time of adding the accessor - we didn't remove that attribute.

[akroshg]

    template<size_t size>
    BOOL SimpleTypeHandler<size>::SetAccessors(DynamicObject* instance, PropertyId propertyId, Var getter, Var setter, PropertyOperationFlags flags)
    {
        if (DoConvertToPathType(instance->GetDynamicType()))
        {
            return ConvertToPathType(instance)->SetAccessors(instance, propertyId, getter, setter, flags);
        }
        else
        {
            // Don't attempt to share cross-site function types
            return ConvertToDictionaryType(instance)->SetAccessors(instance, propertyId, getter, setter, flags);
        }
    }

[pleath]

I'm saying that setting a property on a path type handler with the deleted attribute shouldn't work, since path type handlers don't support deleted properties. It should have forced conversion to some sort of dictionary type handler. If there's a simple type with a deleted property, and we convert to path type without re-adding the deleted property, we'll have trouble even with the fix you're proposing.

[akroshg]

It seems the Pathtypehandler supports deleted property. In many of the function in pathtypehandler.cpp you can see there are checks for attr & ObjectSlotAttr_Deleted
you can find that in HasProperty/SetProperty/GetProperty/DeleteProperty functions.

[pleath]

It definitely doesn't. Those lines are there to prepare for support, but it's not there.

[akroshg]

huh, that is strange.
I can try looking into converting to DictionaryType at the time we are adding an accessor to simple type. Do you know if we marked something on the simple type that any property got deleted? DoConvertToPathType does not have that information.

[pleath]

If simple type does support deleted properties, then "deleted" must be recorded in the property's attributes.

[zenparsing]

There are other usages of DoConvertToPathPath (see SimpleTypeHandler::AddProperty). Should they also be checking for deleted properties? If so, would it make sense to move this "has deleted properties" check into that function?

[akroshg]

There are other places where DoConvertToPathType is used, but they are used inside GetIsLocked() check. And it seems, from the comment I read in SetAttribute, that GetIsLocked returns false if any property got deleted.
However I meant to put that in AddProperty - thanks for catching that.

[pleath]

This is a weaker solution than the one we discussed offline. Converting to a new type handler is equivalent to evolving the type from scratch. In this case it needs to be able to handle any situation that simple types can handle and path types can't. Deleted properties may not be the only one. We need an API that tries to convert to path type but may return some kind of dictionary type handler.

[akroshg]

But why create path type (and spent unnecessary cycles to create and throw away) when we are sure we are going to create dictionary type? Right now I only know for deleted property.

[akroshg]

also on other places we don't follow the same rule

        if (propertyCount >= sizeof(descriptors)/sizeof(SimplePropertyDescriptor))
        {
            Assert(propertyId != Constants::NoProperty);
            if (DoConvertToPathType(instance->GetDynamicType()))
            {
                return ConvertToPathType(instance)->SetPropertyWithAttributes(instance, propertyId, value, attributes, info, flags);
            }
            else
            {
                PropertyRecord const* propertyRecord = scriptContext->GetPropertyName(propertyId);
                return ConvertToSimpleDictionaryType(instance)->AddProperty(instance, propertyRecord, value, attributes, info, flags, possibleSideEffects);
            }
        }

we decide based on certain condition that we need to create path type or dictionary type.

[zenparsing]

Nit: HasDeletedProperties will read a bit better.