1097267 - Remove NewEnumerate

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

It's pretty useless, because we enumerate all the properties in Snapshot anyway. Luckily almost it's also almost unused.

Comment 1

[Tom S [:evilpie]]

Attachment: Change plugin code to Enumerate.

I verified manually that test_enumerate still does something sane.

Comment 2

[Tom S [:evilpie]]

It seems like three places in XPCComponents use it. I think changing them to define every property on the object instead of just enumerating would be bad. I.e. Iterating over Components.interfaces would resolve all of them. Furthermore the code XPCWrappedNativeJSOps for Enumerate is kind of confusing.

Bobby what do you think? Do we need a new API that would allow you to return a list of jsids?

Comment 3

[Tom S [:evilpie]]

Comment on attachment 8520927 [details] [diff] [review]
Change plugin code to Enumerate.

Taking back review request until I resolved some questions.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Tom Schuster [:evilpie] from comment #2)
> Bobby what do you think? Do we need a new API that would allow you to return
> a list of jsids?

Yes.

Comment 5

[Tom S [:evilpie]]

Attachment: wip for xpconnect

I have a really hard time figuring out this code in XPConnect. I am not sure how DontEnumStaticProps fits in here. We don't use the JSops enumerate hook in most cases now, which means we should automatically pick up "native" properties on the JSObject.

Comment 6

[Tom S [:evilpie]]

An important note: If an object has a new enumerate hook now, for .. in won't return native properties, just those that are actually put into the vector by the hook. (We however still take properties from the proto)

Comment 7

[Tom S [:evilpie]]

Attachment: wip for js

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8521439 [details] [diff] [review]
wip for xpconnect

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

Can you try just removing DONT_ENUM_STATIC_PROPS and see if anything breaks on try? It doesn't affect anything web-exposed AFAICT, so I'd be happy to just get rid of it (in a separate patch), which will simplify the XPCWrappedNativeJSOps.cpp piece.

r+ on all of the other pieces with comments addressed, but f+ for now given the XPCWNJSOps piece (which I didn't look at at all).

::: js/xpconnect/idl/nsIXPCScriptable.idl
@@ +23,5 @@
>  [ptr] native JSObjectPtr(JSObject);
>  [ptr] native JSValPtr(JS::Value);
>  [ptr] native JSFreeOpPtr(JSFreeOp);
>  [ref] native JSCallArgsRef(const JS::CallArgs);
> +[ptr] native JSAutoIdVector(JS::AutoIdVector);

Can we pass this as a reference instead?

::: js/xpconnect/src/XPCComponents.cpp
@@ +272,5 @@
> +                *_retval = false;
> +                return NS_OK;
> +            }
> +
> +            if (!properties->append(id)) {

I think we should reserve above based on Length() and then use infallibleAppend, here and below.

::: storage/src/mozStorageStatementParams.cpp
@@ +115,2 @@
>  
> +    if (!aProperties->append(id)) {

We can pre-reserve here as well with mParamCount.

Comment 9

[Tom S [:evilpie]]

I am happy to try that: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6429ff55d7a4. Even when this doesn't work right now. We could change the new_enumerate case to never call XPC_WN_Shared_Enumerate, which should give those hooks that want a special behavior total control.

Comment 10

[Tom S [:evilpie]]

Attachment: remove-dont-enum

Per comment 8 and 9, removes DONT_ENUM_STATIC_PROPS.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8525394 [details] [diff] [review]
remove-dont-enum

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

::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp
@@ +991,5 @@
>  
>      if (si->GetFlags().WantNewEnumerate()) {
> +        // I am like 80% sure that has no effect on what is actually enumerated.
> +        // The loop in jsiter.cpp will only put stuff that is actually in idp
> +        // into the property id array.

Please mark this as XXXevilpies for now - it gets cleaned up in the next patch right?

Comment 12

[Tom S [:evilpie]]

Attachment: Change the "new" enumerate hook in JS

This introduces the new hook in JS. I also removed the class flags, so that this is always just an ObjectOps hook.

Comment 13

[Tom S [:evilpie]]

Attachment: Change the "new" enumerate hook in JS

Comment 14

[Tom S [:evilpie]]

Attachment: Fix nsJSNPRuntime

Comment 15

[Tom S [:evilpie]]

Attachment: Fix NewEnumerate in XPConnect

Comment 16

[Tom S [:evilpie]]

Oh there is some leak here. Maybe the nsISimpleEnumerator *, have to investigate.

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 8525571 [details] [diff] [review]
Fix NewEnumerate in XPConnect

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

::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp
@@ +951,5 @@
> +
> +/***************************************************************************/
> +
> +static bool
> +XPC_WN_JSOp_Enumerate(JSContext *cx, HandleObject obj, AutoIdVector &properties)

Shouldn't we call this NewEnumerate now?

@@ +956,5 @@
> +{
> +    XPCCallContext ccx(JS_CALLER, cx, obj);
> +    XPCWrappedNative* wrapper = ccx.GetWrapper();
> +    THROW_AND_RETURN_IF_BAD_WRAPPER(cx, wrapper);
> +

Don't we still need to call XPC_WN_Shared_Enumerate here like we did in the old WantEnumerate branch? If not, why not?

Comment 18

[Tom S [:evilpie]]

We should probably come up with a better name for this hook altogether.

I don't really see why we should call XPC_WN_Shared_Enumerate. So let's say WantNewEnumerate() was true, we would always return in that block so the JS_EnumerateState function wouldn't be called. Snapshot in jsiter.cpp checks for JS_NATIVE_ENUMERATE after the first call to JSObject::enumerate, which would enumerate the native properties resolved by XPC_WN_Shared_Enumerate. However we never set this state and return the properties that we want to have enumerated.

If you were actually talking about WantEnumerate(), XPC_WN_Helper_Enumerate still calls XPC_WN_Shared_Enumerate.

Comment 19

[Bobby Holley (:bholley)]

Oh I see - this is the 80% sure thing.

Really though, the current behavior seems like a bug - we should always enumerate the XPCWN properties that aren't inherited from the proto (the MutatedSet parts), regardless of whatever the XPCScriptable hook does.

I'm ok with this as an intermediate state if the plan is to immediately refactor this stuff more such that this concern goes away. But otherwise I think we should fix this while we're here.

Comment 20

[Tom S [:evilpie]]

Okay, good that you mention this. So it seems like we actually need an outparam in NewEnumerate that tells us to also enumerate the native properties, so that we pick up those unlazified with Shared_Enumerate.

Comment 21

[Bobby Holley (:bholley)]

(In reply to Tom Schuster [:evilpie] from comment #20)
> Okay, good that you mention this. So it seems like we actually need an
> outparam in NewEnumerate that tells us to also enumerate the native
> properties, so that we pick up those unlazified with Shared_Enumerate.

Is there ever a case where we _don't_ want to enumerate the native properties?

Comment 22

[Tom S [:evilpie]]

TypedObject, because this isn't a native object. I guess we could also just always enumerate native properties for native objects.

Comment 23

[Bobby Holley (:bholley)]

(In reply to Tom Schuster [:evilpie] from comment #22)
> TypedObject, because this isn't a native object. I guess we could also just
> always enumerate native properties for native objects.

That seems better, though you should probably check with jorendorff.

Comment 24

[Bobby Holley (:bholley)]

Comment on attachment 8525571 [details] [diff] [review]
Fix NewEnumerate in XPConnect

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

Per IRC discussion, we should fix the JS engine here to always enumerate the shape for native objects, so that we enumerate the mutated set in the WantNewEnumerate case.

Comment 25

[Tom S [:evilpie]]

Attachment: Change the "new" enumerate hook in JS

We also want to enumerate native properties.

Comment 26

[Tom S [:evilpie]]

Attachment: Fix NewEnumerate in XPConnect

Just added the call of XPC_WN_Shared_Enumerate to XPC_WN_JSOp_Enumerate.

Comment 27

[Tom S [:evilpie]]

Ah, a test fails, because we now enumerate QueryInterface: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a9dbc2f638b1.

Comment 28

[Bobby Holley (:bholley)]

(In reply to Tom Schuster [:evilpie] from comment #27)
> Ah, a test fails, because we now enumerate QueryInterface:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a9dbc2f638b1.

You can probably fix this with DONT_ENUM_QUERY_INTERFACE, or just change the test.

Comment 29

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #28)
> (In reply to Tom Schuster [:evilpie] from comment #27)
> > Ah, a test fails, because we now enumerate QueryInterface:
> > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a9dbc2f638b1.
> 
> You can probably fix this with DONT_ENUM_QUERY_INTERFACE, or just change the
> test.

If it's only storage I'd be for changing the test, since that would let us kill DONT_ENUM_QUERY_INTERFACE when nsDOMClassInfo goes away.

Comment 30

[Bobby Holley (:bholley)]

Comment on attachment 8526677 [details] [diff] [review]
Fix NewEnumerate in XPConnect

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

::: js/xpconnect/src/XPCWrappedNativeJSOps.cpp
@@ +1068,1 @@
>          mJSClass.base.enumerate = JS_EnumerateStub;

Add a comment indicating that this causes us to defer to the ops version below.

Comment 31

[Jason Orendorff [:jorendorff]]

Comment on attachment 8525411 [details] [diff] [review]
Change the "new" enumerate hook in JS

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

Stamping the obsolete patch because I'm too lazy to copy all the comments to the newer one... :)

::: js/public/Class.h
@@ +83,5 @@
>                         bool *succeeded);
>  
>  // This function type is used for callbacks that enumerate the properties of
> +// a JSObject. properties should contain _all_ properties that should
> +// be enumerated for this object.

I think the comment can be improved. How about:

// The type of ObjectOps::enumerate. This callback overrides a portion of SpiderMonkey's default
// [[Enumerate]] internal method. When an ordinary object is enumerated, that object and each object
// on its prototype chain is tested for an enumerate op, and those ops are called in order.
// The properties each op adds to the 'properties' vector are added to the set of values the
// for-in loop will iterate over. All of this is nonstandard.
//
// An object is "enumerated" when it's the target of a for-in loop or JS_Enumerate().
// All other property inspection, including Object.keys(obj), goes through [[OwnKeys]].
//
// The callback's job is to populate 'properties' with all property keys that the for-in loop
// should visit.

::: js/src/jsiter.cpp
@@ +289,5 @@
> +                 return false;
> +
> +            for (size_t n = 0; n < properties.length(); n++) {
> +                if (!Enumerate(cx, pobj, properties[n], true, flags, ht, props))
> +                    return false;

Yeah. OK.  The only painful thing here is that while this thing is called "enumerate", it does not exactly correspond to "overriding the ES6 [[Enumerate]] method". (ES6 [[Enumerate]], per spec, iterates over all properties, not just own properties. It is only called on the exact object being enumerated, not on each of its prototypes in turn.)

But this is an improvement. r=me.

@@ +324,5 @@
> +            // Proxy objects enumerate the prototype on their own, so we're
> +            // done here.
> +            break;
> +        } else {
> +            MOZ_CRASH("This should not happen!");

MOZ_CRASH("non-native objects must have an enumerate op");

Comment 32

[Jason Orendorff [:jorendorff]]

Comment on attachment 8526676 [details] [diff] [review]
Change the "new" enumerate hook in JS

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

::: js/src/jsiter.cpp
@@ +295,5 @@
> +
> +            if (pobj->isNative()) {
> +                if (!EnumerateNativeProperties(cx, pobj.as<NativeObject>(), flags, ht, props))
> +                    return false;
> +            }

OK, this is fine.

Comment 33

[Tom S [:evilpie]]

I need somebody to review the plugin patch,

Jason: In the comment you suggested you write: "All other property inspection, including Object.keys(obj), goes through [[OwnKeys]]." This is not actually true, should I just remove that part? I think it's kind of confusing because the "enumerate" hook doesn't map to [[enumerate]].

Comment 34

[Tom S [:evilpie]]

Hey, can you please find somebody to review the last plugin related patch?

Comment 35

[Josh Aas]

Comment on attachment 8525570 [details] [diff] [review]
Fix nsJSNPRuntime

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

I don't know much about what's going on here, but assuming you do I don't see anything generally wrong with this patch. jst is probably the reviewer you really want here, but since he's hard to get ahold of and nobody knows this code very well, I'm happy to give r+ to unblock you. I think we have pretty good tests for this code.

Comment 36

[Tom S [:evilpie]]

Thank you Josh!

https://hg.mozilla.org/integration/mozilla-inbound/rev/339befde1a62
https://hg.mozilla.org/integration/mozilla-inbound/rev/906c45183f30
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a8e7d35dd45
https://hg.mozilla.org/integration/mozilla-inbound/rev/7066f0d9c110

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/339befde1a62
https://hg.mozilla.org/mozilla-central/rev/906c45183f30
https://hg.mozilla.org/mozilla-central/rev/5a8e7d35dd45
https://hg.mozilla.org/mozilla-central/rev/7066f0d9c110