Last Comment Bug 713965 - De-generify the deletion APIs
: De-generify the deletion APIs
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla12
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 586842
  Show dependency treegraph
Reported: 2011-12-28 15:10 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-02-01 14:30 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

1 - Remove most callers of JSObject::deleteGeneric (9.74 KB, patch)
2011-12-28 15:12 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
2 - Remove remaining deleteGeneric bits (13.93 KB, patch)
2011-12-28 15:14 PST, Jeff Walden [:Waldo] (remove +bmo to email)
bhackett1024: review+
Details | Diff | Splinter Review
Better patch (28.19 KB, patch)
2012-01-05 15:00 PST, Jeff Walden [:Waldo] (remove +bmo to email)
bhackett1024: review+
Details | Diff | Splinter Review

Description User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-28 15:10:20 PST
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 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-28 15:12:15 PST
Created attachment 584652 [details] [diff] [review]
1 - Remove most callers of JSObject::deleteGeneric
Comment 2 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-28 15:14:19 PST
Created attachment 584653 [details] [diff] [review]
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 User image Brian Hackett (:bhackett) 2012-01-02 07:31:54 PST
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 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-05 15:00:57 PST
Created attachment 586238 [details] [diff] [review]
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 User image Brian Hackett (:bhackett) 2012-01-05 19:57:36 PST
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 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-06 13:31:14 PST
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).
Comment 7 User image Paul Wright 2012-01-09 13:14:18 PST
This bug was merged to M-C with merge changeset 0d10b1ea459f.
Can it be marked FIXED?
Comment 8 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-02-01 14:30:28 PST

Note You need to log in before you can comment on or make changes to this bug.