Closed Bug 893186 Opened 6 years ago Closed 6 years ago

remove JS_GetPropertyAttributes and many of its friends

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

(Keywords: dev-doc-needed)

Attachments

(5 files, 1 obsolete file)

Fixes bug 628694! :) Patches coming.
Assignee: general → jorendorff
Attachment #774903 - Flags: review?(bobbyholley+bmo)
Because I'm going to delete getGenericAttributes.

This code makes me feel a bit ill. Some of this change isn't exactly necessary (specifically, changing it to skip the getAttributes call if we're not going to look at the result).

Tempted to do more but I really don't want to break anything.
Attachment #775006 - Flags: review?(n.nethercote)
Attachment #775006 - Attachment description: In Interpreter-inl.h, use JSObject::getGenericAttributes instead of JSObject::getPropertyAttributes → Part 2 - In Interpreter-inl.h, use JSObject::getGenericAttributes instead of JSObject::getPropertyAttributes
This is the primitive operation we want to expose.

A jsapi-test that uses JS_GetPropertyAttributes is also migrated to the new thing.
Attachment #775011 - Flags: review?(n.nethercote)
Attachment #775017 - Flags: review?(luke)
10 files changed, 1 insertion(+), 555 deletions(-)
Attachment #775018 - Flags: review?(jwalden+bmo)
Attachment #775018 - Attachment is patch: true
Attachment #775018 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 775018 [details] [diff] [review]
Part 5 - Delete a bunch of stuff

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

::: js/src/jsclass.h
@@ +279,5 @@
>  };
>  
>  #define JS_NULL_OBJECT_OPS                                                    \
>      {NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,   \
> +     NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL,NULL}

You mean you have to change a line rather than remove it, in this patch? C-c-c-c-combo breaker!
Attachment #775018 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 775006 [details] [diff] [review]
Part 2 - In Interpreter-inl.h, use JSObject::getGenericAttributes instead of JSObject::getPropertyAttributes

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

> Because I'm going to delete getGenericAttributes.

I think you meant getPropertyAttributes?

r=me on the assumption that getGenericAttributes() doesn't have side-effects, which I'm not certain about.
Attachment #775006 - Flags: review?(n.nethercote) → review+
Comment on attachment 775011 [details] [diff] [review]
Part 3 - Add JS_GetOwnPropertyDescriptor

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

::: js/src/jsapi.cpp
@@ +3965,5 @@
>      RootedObject obj2(cx);
>      RootedShape shape(cx);
>  
>      if (!LookupPropertyById(cx, obj, id, flags, &obj2, &shape))
>          return JS_FALSE;

Change |JS_FALSE| to |false|, and the |JS_TRUE| lower down to |true|.

@@ +4018,5 @@
> +    AssertHeapIsIdle(cx);
> +    CHECK_REQUEST(cx);
> +
> +    Rooted<JSPropertyDescriptor> desc(cx);
> +    if (!GetPropertyDescriptorById(cx, obj, id, flags, true, desc.address()))

Change |true| to |/* own = */ true|, please.

@@ +4040,5 @@
>  {
>      RootedObject obj(cx, objArg);
>      RootedId id(cx, idArg);
>      AutoPropertyDescriptorRooter desc(cx);
> +    if (!GetPropertyDescriptorById(cx, obj, id, flags, false, &desc))

Change |false| to |/* own = */ false|, please.

::: js/src/jsapi.h
@@ -3463,5 @@
>  JS_GetPropertyDescriptorById(JSContext *cx, JSObject *obj, jsid id, unsigned flags,
>                               JSPropertyDescriptor *desc);
>  
>  extern JS_PUBLIC_API(JSBool)
> -JS_GetOwnPropertyDescriptor(JSContext *cx, JSObject *obj, jsid id, jsval *vp);

Maybe it's the codeine talking, but AFAICT you've removed this declaration from jsapi.h (and introduced a new function with the same name but different arguments -- will that cause problems for embedders?), but you haven't removed the corresponding definition from jsapi.cpp.  That doesn't seem right.

::: js/src/vm/ScopeObject.cpp
@@ +1406,5 @@
>              desc->value = v;
>              return true;
>          }
>  
> +        return JS_GetOwnPropertyDescriptorById(cx, scope, id, flags, desc);

So the |flags| parameter was previously unused?  Huh.
Attachment #775011 - Flags: review?(n.nethercote)
(In reply to Nicholas Nethercote [:njn] from comment #7)
> Comment on attachment 775006 [details] [diff] [review]
> Part 2 - In Interpreter-inl.h, use JSObject::getGenericAttributes instead of
> > Because I'm going to delete getGenericAttributes.
> 
> I think you meant getPropertyAttributes?

Yes, sorry!

> r=me on the assumption that getGenericAttributes() doesn't have
> side-effects, which I'm not certain about.

The idea was not that it doesn't have side effects, but that it must have exactly the same behavior as the corresponding getPropertyAttributes call.
(In reply to Nicholas Nethercote [:njn] from comment #8)
> >  extern JS_PUBLIC_API(JSBool)
> > -JS_GetOwnPropertyDescriptor(JSContext *cx, JSObject *obj, jsid id, jsval *vp);
> 
> Maybe it's the codeine talking, but AFAICT you've removed this declaration
> from jsapi.h (and introduced a new function with the same name but different
> arguments -- will that cause problems for embedders?)

There aren't any embedders using this one, with confidence P=0.99.

If they were, they'd just get a compile-time error about the new signature. This is one of the easier kinds of API bustage to fix. Contrast changes to GC safety rules.

> but you haven't removed the corresponding definition from jsapi.cpp.
> That doesn't seem right.

Oops. I accidentally removed it in patch 5 instead. Fixed locally.

> ::: js/src/vm/ScopeObject.cpp
> > +        return JS_GetOwnPropertyDescriptorById(cx, scope, id, flags, desc);
> 
> So the |flags| parameter was previously unused?  Huh.

Right. Theoretically, I should add a test for this... but I'm not sure there's any way to trigger the error. Certainly no way using just standard facilities plus the debugger. Maybe a debugger chrome mochitest? I'll ask jimb.
Attachment #775017 - Flags: review?(luke) → review+
Attachment #774903 - Flags: review?(bobbyholley+bmo) → review+
> > r=me on the assumption that getGenericAttributes() doesn't have
> > side-effects, which I'm not certain about.
> 
> The idea was not that it doesn't have side effects, but that it must have
> exactly the same behavior as the corresponding getPropertyAttributes call.

There are circumstances (i.e. |attrs & JSPROP_READONLY| is false) where the old code called getPropertyAttributes() but the new code does not call getGenericAttributes().  So as long as those two functions are pure, that should be fine.  But if there's any doubt, perhaps you should revert to the prior structure for the |else| branch.
Coming back to this after a month. :-P

(In reply to Nicholas Nethercote [:njn] from comment #11)
> There are circumstances (i.e. |attrs & JSPROP_READONLY| is false) where the
> old code called getPropertyAttributes() but the new code does not call
> getGenericAttributes().  So as long as those two functions are pure, that
> should be fine.  But if there's any doubt, perhaps you should revert to the
> prior structure for the |else| branch.

Ah, I see. I don't think any getGenericAttributes implementations have bizarro side effects, no. I was just thinking of things like resolve hooks, which would be OK (does it really count as impure, if all that's happening is you're lazily doing work that you deferred earlier?).

But now I see we just did a lookupProperty() for the same property name on the same object, so any resolve hooks would already have been triggered. I'm sure we're fine here.
> I'm sure we're fine here.

It's ok by me if it's ok by you!
Attachment #775011 - Attachment is obsolete: true
Attachment #799837 - Flags: review?(n.nethercote)
Attachment #799837 - Flags: review?(n.nethercote) → review+
Depends on: 913885
This needs to be documented on <https://developer.mozilla.org/en-US/docs/SpiderMonkey/31> and on the individual pages for the removed methods.
Keywords: dev-doc-needed
Depends on: 919844
You need to log in before you can comment on or make changes to this bug.