Closed Bug 1097267 Opened 5 years ago Closed 5 years ago

Remove NewEnumerate

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(4 files, 6 obsolete files)

It's pretty useless, because we enumerate all the properties in Snapshot anyway. Luckily almost it's also almost unused.
Attached patch Change plugin code to Enumerate. (obsolete) — Splinter Review
I verified manually that test_enumerate still does something sane.
Attachment #8520927 - Flags: review?(benjamin)
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?
Flags: needinfo?(bobbyholley)
Comment on attachment 8520927 [details] [diff] [review]
Change plugin code to Enumerate.

Taking back review request until I resolved some questions.
Attachment #8520927 - Flags: review?(benjamin)
(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.
Flags: needinfo?(bobbyholley)
Attached patch wip for xpconnect (obsolete) — Splinter Review
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.
Attachment #8521439 - Flags: feedback?(bobbyholley)
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)
Attached patch wip for js (obsolete) — Splinter Review
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.
Attachment #8521439 - Flags: feedback?(bobbyholley) → feedback+
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.
Attachment #8520927 - Attachment is obsolete: true
Attachment #8521449 - Attachment is obsolete: true
Attached patch remove-dont-enumSplinter Review
Per comment 8 and 9, removes DONT_ENUM_STATIC_PROPS.
Attachment #8525394 - Flags: review?(bobbyholley)
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?
Attachment #8525394 - Flags: review?(bobbyholley) → review+
This introduces the new hook in JS. I also removed the class flags, so that this is always just an ObjectOps hook.
Attachment #8525401 - Flags: review?(jorendorff)
Attachment #8521439 - Attachment is obsolete: true
Attachment #8525411 - Flags: review?(jorendorff)
Attachment #8525401 - Attachment is obsolete: true
Attachment #8525401 - Flags: review?(jorendorff)
Attachment #8525570 - Flags: review?(benjamin)
Attached patch Fix NewEnumerate in XPConnect (obsolete) — Splinter Review
Attachment #8525571 - Flags: review?(bobbyholley)
Oh there is some leak here. Maybe the nsISimpleEnumerator *, have to investigate.
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?
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.
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.
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.
(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?
TypedObject, because this isn't a native object. I guess we could also just always enumerate native properties for native objects.
(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 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.
Attachment #8525571 - Flags: review?(bobbyholley) → review-
We also want to enumerate native properties.
Attachment #8525411 - Attachment is obsolete: true
Attachment #8525411 - Flags: review?(jorendorff)
Attachment #8526676 - Flags: review?(jorendorff)
Just added the call of XPC_WN_Shared_Enumerate to XPC_WN_JSOp_Enumerate.
Attachment #8525571 - Attachment is obsolete: true
Attachment #8526677 - Flags: review?(bobbyholley)
Ah, a test fails, because we now enumerate QueryInterface: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a9dbc2f638b1.
(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.
(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 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.
Attachment #8526677 - Flags: review?(bobbyholley) → review+
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");
Attachment #8525411 - Flags: review+
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.
Attachment #8526676 - Flags: review?(jorendorff) → review+
Attachment #8525570 - Flags: review?(benjamin) → review?(jst)
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]].
Hey, can you please find somebody to review the last plugin related patch?
Flags: needinfo?(joshmoz)
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.
Attachment #8525570 - Flags: review?(jst) → review+
Flags: needinfo?(joshmoz)
You need to log in before you can comment on or make changes to this bug.