1088002 - Change JSAPI for defining accessors to assume JSNative accessors, not JSPropertyOP ones

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Basically the goal is to flip the sense of the JSPROP_NATIVE_ACCESSORS flag.  This only affects JS_DefineProperty, JS_DefineElement, JS_DefineUCProperty, JS_DefinePropertyById, and JS_DefineProperties.  I did not change the internal defineProperty hooks, because by the time we get to those JSNatives have already been boxed into JSFunctions, so a non-null accessor without the relevant JSPROP_GETTER/SETTER flag must be a JSPropertyOp.

After this change, we can easily find all the places that are using JSPropertyOp accessors: they have the JSPROP_PROPOP_ACCESSORS flag.

Now there _is_ a false positive here: someone trying to define a prop with stub accessors (as opposed to class-default accessors; I really wish we would kill that) has to use JSPROP_PROPOP_ACCESSORS, because the stubs are JSPropertyOps.  I'll also post a patch to auto-guess in this case, in case people care.

The big first question is whether we're OK making this API change.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Change GetLocationProperty to a JSNative

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change JS_DefineElement, JS_DefineProperty, JS_DefineUCProperty, JS_DefinePropertyById, and JS_DefineProperties to default to using JSNative accessors, not JSPropertyOp accessors

Also, right now we assert that if we have native accessors we're JSPROP_SHARED.  Sure would be nice to just assume that instead of forcing tehe caller to add boilerplate....

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I'll also post a patch to auto-guess in this case, in case people care.

What that patch shows, by the way, is that we have the following JSPropertyOp uses in the Firefox tree:

1)  writeToProto_getProperty/writeToProto_setProperty in Sandbox.cpp
2)  NativeGet in testSetProperty.cpp
3)  StructType::FieldGetter/FieldSetter in CTypes.cpp

As far as I can tell, that's it.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Stop requiring JSPROP_PROPOP_ACCESSORS just to use stub accessors

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, I guess that doesn't count JSClasses with non-stub class ops.

Those would be:

4)  ArrayType::Getter/Setter in CTypes.cpp
5)  env_setProperty in XPCShellImpl.cpp
6)  XPC_WN_OnlyIWrite_SetPropertyStub in XPCWrappedNativeJSOps.cpp
7)  XPC_WN_MaybeResolvingStrictPropertyStub (set op) in XPCWrappedNativeJSOps.cpp
8)  XPC_WN_CannotModifyStrictPropertyStub (set op) in XPCWrappedNativeJSOps.cpp
9)  XPC_WN_Helper_SetProperty in XPCWrappedNativeJSOps.cpp
10) XPC_WN_Helper_GetProperty in XPCWrappedNativeJSOps.cpp
11) XPC_WN_OnlyIWrite_Proto_SetPropertyStub in XPCWrappedNativeJSOps.cpp
12) NPObjWrapper_GetProperty/NPObjWrapper_SetProperty in nsJSNPRuntime.cpp
13) test_prop_get in testClassGetter.cpp

It also doesn't count manual weirdness, which comprises:

14) array_length_getter/setter in jsarray.cpp
15) ArgGetter/Setter and StrictArgGetter/Setter in ArgumentsObject.cpp
16) UnknownPropertyStub/UnknownStrictPropertyStub in JavaScriptShared.cpp
17) NativeGet in testSetProperty.cpp

Comment 6

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> 1)  writeToProto_getProperty/writeToProto_setProperty in Sandbox.cpp

Note that this was landed recently, with the understanding that billm is explicitly on the hook for fixing this up whenever we're otherwise ready to remove the propertyop stuff (see the comment above those functions).

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yeah, I'd be more worried about the array length bits, which can't just become a JSNative accessor per spec.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8510290 [details] [diff] [review]
part 2.  Change JS_DefineElement, JS_DefineProperty, JS_DefineUCProperty, JS_DefinePropertyById, and JS_DefineProperties to default to using JSNative accessors, not JSPropertyOp accessors

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

::: dom/base/nsDOMClassInfo.cpp
@@ +1259,5 @@
>        !JS_DefineUCProperty(cx, global, mData->mNameUTF16,
>                             NS_strlen(mData->mNameUTF16),
> +                           desc.value(),
> +                           // Descriptors never store JSNatives for accessors:
> +                           // they have either JSFunctions or JSPropertyOps

Dunno about DOM comment standards, but JS would end this with a period.

::: dom/bindings/BindingUtils.cpp
@@ +961,3 @@
>            // They all have getters, so we can just make it.
>            JS::Rooted<JSFunction*> fun(cx,
> +                                      JS_NewFunctionById(cx, (JSNative)attrSpec.getter.native.op,

Remove the cast, right?

@@ +972,2 @@
>              // We have a setter! Make it.
> +            fun = JS_NewFunctionById(cx, (JSNative)attrSpec.setter.native.op, 1, 0,

Remove the cast?

::: js/ipc/WrapperAnswer.cpp
@@ +179,5 @@
>  
> +    if (!JS_DefinePropertyById(cx, obj, id, desc.value(),
> +                               // Descrriptors never store JSNatives for
> +                               // accessors: they have either JSFunctions or
> +                               // JSPropertyOps

Period.

::: js/src/jsapi.cpp
@@ +3051,5 @@
>                    unsigned attrs,
> +                  Native getter /* = nullptr */, Native setter /* = nullptr */)
> +{
> +    return DefineProperty(cx, obj, name, value, NativeOpWrapper(getter),
> +                          NativeOpWrapper(setter),

Keep the two NativeOpWrappers on one line.

@@ +3284,5 @@
>      for (; ps->name; ps++) {
>          if (!PropertySpecNameToId(cx, ps->name, &id))
>              return false;
>  
> +        if (!(ps->flags & JSPROP_GETTER)) {

So I was going to say have an explanatory comment here.  But really, we do still have a dichotomy here, that we can and should lean on hard for understanding.  So let's add an isSelfHosted() member function to JSPropertySpec that simply returns |flags & JSPROP_GETTER|, and use that here.  (Probably the blocks should invert as well, to avoid a negative, but that's at least followup-revision-land.)  That'll make clear that it's this (or implicitly that), better than a comment to that effect.

@@ +3292,5 @@
>  
>              // If you do not have a self-hosted getter, you should not have a
>              // self-hosted setter. This is the closest approximation to that
>              // assertion we can have with our setup.
> +            MOZ_ASSERT_IF(ps->setter.native.info, ps->setter.native.op);

Given the above, this is the unusual case where *fewer* assertions makes for better code.  If we fork conditionally on self-hosted or not, it's clear there are only two cases to consider.  Adding assertions muddies the water so much that it's much less clear what's happening -- I think I probably spent half an hour, at least, trying to understand this, because of the comments suggesting the state space was harder to understand than it actually was.

So remove this assertion, the above assertion, and the comments by each.

@@ +3302,5 @@
>              }
>          } else {
>              // If you have self-hosted getter/setter, you can't have a
>              // native one.
> +            MOZ_ASSERT(!ps->getter.native.op && !ps->setter.native.op);

As before, remove this comment and assertion.

::: js/src/jsapi.h
@@ +2349,5 @@
>   * Macro static initializers which make it easy to pass no JSJitInfo as part of a
>   * JSPropertySpec or JSFunctionSpec.
>   */
> +#define JSNATIVE_WRAPPER(native) { {native, nullptr} }
> +#define JSNATIVE_NULLWRAPPER JSNATIVE_WRAPPER(nullptr)

I'd prefer use of JSNATIVE_WRAPPER(nullptr) everywhere instead of a further-obfuscating macro, if you're willing to make the change through this.

::: js/src/perf/jsperf.cpp
@@ +131,5 @@
>  
>  // If this were C++ these would be "static const" members.
>  
> +static const uint8_t PM_CATTRS =
> +    JSPROP_ENUMERATE|JSPROP_READONLY|JSPROP_PERMANENT|JSPROP_PROPOP_ACCESSORS;

Please move this into RegisterPerfMeasurement so it's more obvious it's used/applied only there.

::: js/src/proxy/DirectProxyHandler.cpp
@@ +43,5 @@
> +                               desc.getter(), desc.setter()) &&
> +           JS_DefinePropertyById(cx, target, id, v,
> +                                 // Descriptors never store JSNatives for
> +                                 // accessors: they have either JSFunctions or
> +                                 // JSPropertyOps

Period.

::: js/src/vm/ScopeObject.cpp
@@ +1679,5 @@
>  
> +        return JS_DefinePropertyById(cx, scope, id, desc.value(),
> +                                     // Descriptors never store JSNatives for
> +                                     // accessors: they have either JSFunctions
> +                                     // or JSPropertyOps

Bleargh, this needs a bottle of mouthwash by it.  (And, more seriously, a period at the end of a complete sentence.)

::: js/src/vm/Shape-inl.h
@@ +228,5 @@
> +                                               MOZ_GUARD_OBJECT_NOTIFIER_PARAM_IN_IMPL)
> +{
> +    if (attrs & (JSPROP_GETTER | JSPROP_SETTER))
> +        inner.emplace(cx, attrs, reinterpret_cast<PropertyOp *>(pgetter),
> +                      reinterpret_cast<StrictPropertyOp *>(psetter));

Braces around the if-body, because it extends over two lines.  And use JS_CAST_NATIVE_TO(pgetter, PropertyOp), or whatever it is.

::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp
@@ +365,5 @@
>                  *resolved = true;
> +            return JS_DefinePropertyById(ccx, obj, id, desc.value(),
> +                                         // Descriptors never store JSNatives
> +                                         // for accessors: they have either
> +                                         // JSFunctions or JSPropertyOps.

...and somehow you have a period on *one* of these.  How?  :-P  :-)

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +504,4 @@
>          RootedFunction getterObj(cx);
>          RootedFunction setterObj(cx);
>          unsigned flags = psMatch->flags;
> +        if (!(flags & JSPROP_GETTER)) {

!psMatch->isSelfHosted()

Same comment as before about nice to invert this, in a separate patch or so.

@@ +536,5 @@
> +                                     UndefinedHandleValue,
> +                                     // This particular descriptor, unlike most,
> +                                     // actually stores JSNatives directly,
> +                                     // since we just set it up.  Do NOT pass
> +                                     // JSPROP_PROPOP_ACCESSORS here!

Ugh, this hurt to grok.

@@ +731,5 @@
>          // We have code to Xray self-hosted accessors. But at present, there don't appear
>          // to be any self-hosted accessors anywhere in SpiderMonkey, let alone in on an
>          // Xrayable class, so we can't test it. Assert against it to make sure that we get
>          // test coverage in test_XrayToJS.xul when the time comes.
> +        MOZ_ASSERT(!(ps->flags & JSPROP_GETTER),

!ps->isSelfHosted()

@@ +1189,5 @@
>  
> +        return JS_DefinePropertyById(cx, holder, id, desc.value(),
> +                                     // Descriptors never store JSNatives for
> +                                     // accessors: they have either JSFunctions
> +                                     // or JSPropertyOps

Period.

@@ +1251,5 @@
>      // Define the property.
> +    return JS_DefinePropertyById(cx, holder, id, desc.value(),
> +                                 // Descriptors never store JSNatives for
> +                                 // accessors: they have either JSFunctions or
> +                                 // JSPropertyOps

Period.

@@ +1549,5 @@
>  
> +    return JS_DefinePropertyById(cx, holder, id, desc.value(),
> +                                 // Descriptors never store JSNatives for
> +                                 // accessors: they have either JSFunctions or
> +                                 // JSPropertyOps

Period.

@@ +1873,5 @@
>  
> +    if (!JS_DefinePropertyById(cx, holder, id, desc.value(),
> +                               // Descriptors never store JSNatives for
> +                               // accessors: they have either JSFunctions or
> +                               // JSPropertyOps

Period.

@@ +2024,5 @@
>  
>      return JS_DefinePropertyById(cx, expandoObject, id, wrappedDesc.value(),
> +                                 // Descriptors never store JSNatives for
> +                                 // accessors: they have either JSFunctions
> +                                 // or JSPropertyOps

Period.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> So remove this assertion, the above assertion, and the comments by each.

I just want to make sure we're clear on the state space here.

A JSPropertySpec stores a getter, setter, name, and flags.  getter and setter are unions of JSNativeWrapper and SelfHostedWrapper.  If JSPROP_GETTER is set, they should both be SelfHostedWrapper; otherwise they should be JSNativeWrapper.

Both JSNativeWrapper and SelfHostedWrapper are two words.  In JSNativeWrapper, the first word may be null (in the setter case, at least), and the second word may be null, but if the second word is not null the first one isn't either.  In SelfHostedWrapper, the first word is always null and the second word always non-null.

What these asserts are attempting to assert is that if JSPROP_GETTER is not set then both getter and setter are actually JSNativeWrapper, not SelfHostedWrapper.  And similarly, the assert in the other branch is trying to assert that both are SelfHostedWrapper.  As in, the assert is that people didn't screw up the correlation between JSPROP_GETTER and the types of the getter and setter.

It seems to me that there is value in having asserts to this effect.  Maybe we just need to make them be less confusing (e.g. by factoring them out into clearer methods on JSPropertySpec like getterIsSelfHosted()/getterIsJSNative() and similar for setter?

>And use JS_CAST_NATIVE_TO(pgetter, PropertyOp), or whatever it is.

JS_CAST_NATIVE_TO works on a JSNative, not a JSNative*.

I've made the rest of the changes.  The isSelfHosted thing was a good idea; it makes things clearer for sure.

Filed bug 1090749 as the followup to reverse the isSelfHosted tests.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8510300 [details] [diff] [review]
part 3.  Stop requiring JSPROP_PROPOP_ACCESSORS just to use stub accessors

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

::: js/src/jsapi.cpp
@@ +2821,5 @@
> +    // some sanity about it, though.
> +    MOZ_ASSERT_IF(getter == JS_PropertyStub,
> +                  setter == JS_StrictPropertyStub || (attrs & JSPROP_PROPOP_ACCESSORS));
> +    MOZ_ASSERT_IF(setter == JS_StrictPropertyStub,
> +                  getter == JS_PropertyStub || (attrs & JSPROP_PROPOP_ACCESSORS));

Blank line after this.

@@ +2827,5 @@
> +    // getter == JS_PropertyStub then setter == JS_PropertyStub and we
> +    // can skip over this whole bit.
> +    // Similarly if setter == JS_StrictPropertyStub.
> +    if (!(attrs & JSPROP_PROPOP_ACCESSORS) && (getter || setter) &&
> +        getter != JS_PropertyStub && setter != JS_StrictPropertyStub) {

Slightly better comment IMO:

"""
If !(attrs & JSPROP_PROPOP_ACCESSORS), then either getter/setter are both possibly-null JSNatives (or possibly-null JSFunction* if JSPROP_GETTER or JSPROP_SETTER is appropriately set), or both are the well-known property stubs.  The subsequent block must handle only the first of these cases, so carefully exclude the latter case.
"""

And so the condition could more clearly/simply be:

    if (!(attrs & JSPROP_PROPOP_ACCESSORS) &&
        (getter != JS_PropertyStub && setter != JS_StrictPropertyStub))
    {
        ...

Right?  (Note too: open-brace on new line when condition overflows a single line.)

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Blank line after this.

Done.

> Slightly better comment IMO:

Fair.  Done.

> And so the condition could more clearly/simply be:

Indeed.  The (getter || setter) was to avoid the RootedAtom bit when both are null, but I see now it's just a cast, not an atomization, so not worth the complexity.  Changed.

Comment 12

[Jeff Walden [:Waldo]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> I just want to make sure we're clear on the state space here.

Yep, I think I'm clear.  (And I was just this clear before reading that hunk of patch, to be honest -- its belaboring really was what made that bit harder than it should have been.)

> What these asserts are attempting to assert is that if JSPROP_GETTER is not
> set then both getter and setter are actually JSNativeWrapper, not
> SelfHostedWrapper.  And similarly, the assert in the other branch is trying
> to assert that both are SelfHostedWrapper.  As in, the assert is that people
> didn't screw up the correlation between JSPROP_GETTER and the types of the
> getter and setter.

Yeah.  Given the JS_PSG{,S} and JS_SELF_HOSTED_GET{,SET} macros that enforce all this without any work from the author, this seems useful only for the case of users creating JSPropertySpec at runtime.  Which Gecko anywhere may not even do at all.  So for *us*, it seems unnecessary.

> It seems to me that there is value in having asserts to this effect.  Maybe
> we just need to make them be less confusing (e.g. by factoring them out into
> clearer methods on JSPropertySpec like
> getterIsSelfHosted()/getterIsJSNative() and similar for setter?

Factoring out into methods would help a lot.  Hiding the guts of what they try to assert would help a ton.  If it's not implying things about structure that make things confusing, it's tolerable, at least.

(Note that the XrayWrapper code in the previous[?] patch had a place that basically duplicated the logic here -- sans assertions.  That code was much more readable than this, because of not asserting a bunch of things true by design with the macros.  Whatever assertions/abstraction is used, it could/should probably be added there as well, for consistency/sanity.)

Comment 13

[Boris Zbarsky [:bzbarsky]]

I ended up putting the asserts in isSelfHosted(), to make it very simple on consumers.

   https://hg.mozilla.org/integration/mozilla-inbound/rev/d0563150294b
   https://hg.mozilla.org/integration/mozilla-inbound/rev/66860992cd5e
   https://hg.mozilla.org/integration/mozilla-inbound/rev/bec6973477c6

Comment 14

[Alon Zakai (:azakai)]

Looks like either this or bug 1081274 regressed octane-box2d by around 40% on awfy.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0563150294b
https://hg.mozilla.org/mozilla-central/rev/66860992cd5e
https://hg.mozilla.org/mozilla-central/rev/bec6973477c6

Comment 16

[Hannes Verschore [:h4writer]]

(In reply to Alon Zakai (:azakai) from comment #14)
> Looks like either this or bug 1081274 regressed octane-box2d by around 40%
> on awfy.

The regression is quite severe. 3% on our total octane score. Can you investigate and search what caused this and hopefully find a fix? Or if you cannot fix it soonish maybe look into backing it out?

Comment 17

[Jeff Walden [:Waldo]]

Going by the last couple comments in bug 1081274 (and frankly by the nature of the changes made in this patch, which are not particularly substantive), it's that bug at fault, and investigation is happening, and this bug should be left alone.  :-)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Sorry, I was out most of the day, so just got to the bugmail from this bug.

Just to close the loop on stuff, the fix for the perf issue is in bug 1091795 and is on inbound, along with some more investigation into possible perf things on octane-box2d.  Graphs are back to normal.

Comment 19

[Boris Zbarsky [:bzbarsky]]

So as far as documenting this...  Neither https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JSPropertySpec nor its link for the flags (<https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_GetPropertyAttributes#Property_attributes>) talks about this stuff at all.  Do we have any documentation that does?

Comment 20

[Boris Zbarsky [:bzbarsky]]

And neither does https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_DefineProperty

Comment 21

[Hannes Verschore [:h4writer]]

(In reply to Boris Zbarsky [:bz] from comment #18)
> Sorry, I was out most of the day, so just got to the bugmail from this bug.
> 
> Just to close the loop on stuff, the fix for the perf issue is in bug
> 1091795 and is on inbound, along with some more investigation into possible
> perf things on octane-box2d.  Graphs are back to normal.

Thanks for the quick turnaround! I see you also filed some extra possible optimizations. Awesome. Are you looking into fixing them yourself? Else we might look into who can and has the time. Looks some nasty corner-cases that we might encounter on normal scripts.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> Are you looking into fixing them yourself?

I'm idly poking at bug 1089050, but would be quite happy for someone else to pick it up too.  Just let me know.

I have no plans to work on bug 1091978.