893186 - remove JS_GetPropertyAttributes and many of its friends

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Fixes bug 628694! :) Patches coming.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Part 1 - Fix some code in xpconnect to use JS_HasProperty instead of JS_GetPropertyAttributes

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Part 2 - In Interpreter-inl.h, use JSObject::getGenericAttributes instead of JSObject::getPropertyAttributes

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.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Part 3 - Add JS_GetOwnPropertyDescriptor

This is the primitive operation we want to expose.

A jsapi-test that uses JS_GetPropertyAttributes is also migrated to the new thing.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Part 4 - Replace a use in JSD

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Part 5 - Delete a bunch of stuff

10 files changed, 1 insertion(+), 555 deletions(-)

Comment 6

[Jeff Walden [:Waldo]]

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!

Comment 7

[Nicholas Nethercote [inactive]]

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.

Comment 8

[Nicholas Nethercote [inactive]]

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.

Comment 9

[Jason Orendorff [:jorendorff]]

(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.

Comment 10

[Jason Orendorff [:jorendorff]]

(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.

Comment 11

[Nicholas Nethercote [inactive]]

> > 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.

Comment 12

[Jason Orendorff [:jorendorff]]

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.

Comment 13

[Nicholas Nethercote [inactive]]

> I'm sure we're fine here.

It's ok by me if it's ok by you!

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: Part 3 - Add JS_GetOwnPropertyDescriptor, v2

Comment 15

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c19e0a8d53e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a5583d59e3f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8f4f23d53e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9515bf89bc4a
https://hg.mozilla.org/integration/mozilla-inbound/rev/77e2eaaf2fbb

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2c19e0a8d53e
https://hg.mozilla.org/mozilla-central/rev/5a5583d59e3f
https://hg.mozilla.org/mozilla-central/rev/5b8f4f23d53e
https://hg.mozilla.org/mozilla-central/rev/9515bf89bc4a
https://hg.mozilla.org/mozilla-central/rev/77e2eaaf2fbb

Comment 17

[Jeff Walden [:Waldo]]

This needs to be documented on <https://developer.mozilla.org/en-US/docs/SpiderMonkey/31> and on the individual pages for the removed methods.