713965 - De-generify the deletion APIs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Remove ObjectOps::deleteGeneric, implementations of it as such, and so on.  This won't change the underlying representations, but it will mean the well-typed APIs will become mandatory, reducing work when the storage switchover does actually happen.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: 1 - Remove most callers of JSObject::deleteGeneric

Comment 2

[Jeff Walden [:Waldo]]

Attachment: 2 - Remove remaining deleteGeneric bits

Incidentally, it'd be helpful if you added PropertyName*/uint32_t/SpecialId-split versions of the jsid methods to TI code, so I don't have to create an id just for that.  If it all funnels to one implementation now, that's perfectly fine for now -- just to make it easier to do these conversions in the future.

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 584652 [details] [diff] [review]
1 - Remove most callers of JSObject::deleteGeneric

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

::: js/src/jsapi.cpp
@@ +4032,5 @@
> +                if (!obj->deleteProperty(cx, name->asPropertyName(), rval, false))
> +                    return false;
> +            }
> +        }
> +    }

This logic is cloned in four places, and needs to be factored so that it only appears once.  I'm not sure where the best place to do this is, though.  js_Delete{Property,Generic} seem to only work on native objects, maybe have an inline method for deleting a jsid or a JSAtom* and dispatch to the various specialized methods?

@@ +4061,5 @@
> +    uint32_t index;
> +    if (atom->isIndex(&index))
> +        return obj->deleteElement(cx, index, rval, false);
> +
> +    return obj->deleteProperty(cx, atom->asPropertyName(), rval, false);

Similar, this isIndex logic should only appear once, in a method to delete a JSAtom.

::: js/src/jsarray.cpp
@@ +546,5 @@
> +        if (!js_ValueToAtom(cx, DoubleValue(index), &atom))
> +            return -1;
> +        if (!obj->deleteProperty(cx, atom->asPropertyName(), &v, strict))
> +            return -1;
> +    }

It looks like most callers of DeleteArrayElement are passing in a uint32_t, for which ones is it possible to get a non-index element?

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Better patch

I don't particularly want to provide a JSAtom-based method, because if one's provided, people will end up not calling the PropertyName-based method when they know that they don't have an index.

It's possible for the callers in array_splice and a couple others.  Longer-run we won't be doing all property deletion using one workhorse that has to be double-generic.  In the shorter term, we should live with what we have.

I did some more thinking on this and came up with an alternative patch -- still kills the generic, jsid-flavored version, replaces it with a Value-flavored version.  This also gets rid of the generic delete methods when the class in question has property-type-specific code, and could more efficiently use the type-specific versions.

Comment 5

[Brian Hackett [Laid off!]]

Comment on attachment 586238 [details] [diff] [review]
Better patch

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

::: js/src/jsapi.cpp
@@ +4168,5 @@
>      assertSameCompartment(cx, obj, id);
>      JSAutoResolveFlags rf(cx, JSRESOLVE_QUALIFIED);
> +
> +    if (JSID_IS_SPECIAL(id))
> +        return obj->deleteSpecial(cx, JSID_TO_SPECIALID(id), rval, false);

Will the ValueIsSpecial test under deleteByValue take care of this case?  I can see how this would behave differently if someone tried to delete an object jsid off of a non-XML object (since ValueIsSpecial depends on the object being accessed --- creepy), but (a) are there semantics here that are worth preserving and (b) what keeps other callers of deleteByValue from falling into this pit?

Comment 6

[Jeff Walden [:Waldo]]

deleteByValue assumes you have a value.  Given a jsid, that's not necessarily the case.  It is if you have an atom-based or integer-based jsid.  If you have a SpecialId, it's the case if it's object-valued.  It's not the case if it's JSID_VOID, JSID_DEFAULT_XML_NAMESPACE, or JSID_EMPTY.  The first I understand to be ignorable, because it's just a fill-in value we document as not being something users need handle.  The second I think may be an internal hack, but I'm not entirely certain.  The last, I'm not 100% sure of.  This is enough uncertainty that for now, I'd like to address the possibility by not using IdToValue.

The other callers don't fall into this pit because they all derive from either Values (which couldn't represent the above oddities), from integers or doubles (likewise), or from atoms (likewise).

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c98dd9fd645

Comment 7

[Paul Wright]

This bug was merged to M-C with merge changeset 0d10b1ea459f.
Can it be marked FIXED?

Comment 8

[Jeff Walden [:Waldo]]

Yup.