Closed Bug 564954 Opened 10 years ago Closed 10 years ago

TM: redirecting into js_Enumerate does not invoke the old enumeration hook

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta1+

People

(Reporter: gal, Assigned: mrbkap)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

This was uncovered by the iterator patch. mrbkap and I will get together tomorrow to come up with a plan how to fix it.

s: moz2-linux-slave40
198 ERROR TEST-UNEXPECTED-FAIL | /tests/js/src/xpconnect/tests/mochitest/test_wrappers.html | enumeration over XOWs walks the prototype chain - got "foopy", expected "assign,foopy,hash,host,hostname,href,pathname,port,protocol,reload,replace,search"
199 ERROR TEST-UNEXPECTED-FAIL | /tests/js/src/xpconnect/tests/mochitest/test_wrappers.html | enumeration over XPCNWs walks the prototype chain - got "barpy", expected "assign,barpy,hash,host,hostname,href,pathname,port,protocol,reload,replace,search"
201 ERROR TEST-UNEXPECTED-FAIL | /tests/js/src/xpconnect/tests/mochitest/test_wrappers.html | enumeration over SJOWs walks the prototype chain and works over XOWs - got "foopy", expected "assign,foopy,hash,host,hostname,href,pathname,port,protocol,reload,replace,search"
blocking2.0: --- → beta1+
Attached patch patch (obsolete) — Splinter Review
Assignee: general → gal
The attached patch fixes a long-standing bug in the enumeration protocol: objects can't indicate what object to iterate next. In many cases this is the native prototype (object->fslots[JSSLOT_PROTO]), but for wrapped objects this is of course not always true. I fixed all mozilla uses of the API to tolerate and silently ignore a call to ->enumerate with JSENUMERATE_PROTO. The protocol driver will check whether the call was ignored, and if so, it follows the native prototype. Otherwise the ->enumerate hook can supply the next object to iterate.

XML objects use this new API to indicate that they don't want their native prototype to be enumerated. This allows removing a special case check from the generic iteration fast path.
This currently doesn't change wrappers. The changes I am proposing to our wrapper code are:
- delete current iteratorObject hook code
- implement a class JSCLASS_NEW_ENUMERATE hook that uses the above API to let the iterator code build an appropriate native enumerate
(In reply to comment #2)
> The attached patch fixes a long-standing bug in the enumeration protocol:
> objects can't indicate what object to iterate next.

The patch requires to change the current enumeration callbacks. That may be undesirable. What about calling JSENUMERATE_PROTO only if JSENUMERATE_NEXT returns a special value indicating that the callback want to alter the enumeration? This way JSENUMERATE_PROTO can remain private and not to leak into the public API.
I posted to jsdev. I want to understand the severity of pain we inflict on embedders before ruling out an API change. If nobody is really using this outside Mozilla, I prefer doing this the right way instead of yet another ugly internal hack in the code.
Note: if nobody or very few use the API, I might even drop the JSVAL_ZERO detection mechanism and require a proper answer at all times.
Posting to the js-engine group won't help understand severity of pain. People are not going to read who should; those who read won't know what to check; etc. etc.

Better if we can make this an engine-side extension that does not break the old API even by adding a new case in the enum.

/be
(In reply to comment #6)
> Note: if nobody or very few use the API, I might even drop the JSVAL_ZERO
> detection mechanism and require a proper answer at all times.

If the API breakage is OK, than indeed lets fix all the annoyances of the current protocol. For example, the enumeration hook could be forced to populate the id array itself to minimize the number of hook calls.
(In reply to comment #7)
> Better if we can make this an engine-side extension that does not break the old
> API even by adding a new case in the enum.

What about adding JSCLASS_NEW_NEW_ENUMERATE (I am not insisting on the name ;) ) that would provide the API that are most suitable for fast iteration and just keep JSCLASS_NEW_ENUMERATE for compatibility?
I am all for #9. The driver of the protocol basically does just that. My longer-term goal is to replace the objectop with a call that returns a JSIDArray and to delete the class NEW_ENUMERATE hook, but I was thinking to do this in steps.
I think #7 is a fallacy. We keep passing on known, understood improvements because of fear of unknown, not-understood compatibility problems of people we don't know, people who don't read jsdev, and very likely don't even exist.
If we take this change, it must happen before the beta.
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
I'm not against good changes including breaking API ones -- I'm questioning this change on its intrinsic costs and benefits, though. Those costs, plus it breaking content we host ("us" vs. "them" is bad for SpiderMonkey embedders), suggest an alternative (which Igor also proposed too).

Getting rid of JSCLASS_NEW_ENUMERATE would be easier, all else equal, if we did not extend it first.

So (I asked this elsewhere too) is this JSENUMERATE_PROTO required once we use proxies (bug 546590) with native handlers instead of the current wrappers?

/be
(In reply to comment #13)
> 
> So (I asked this elsewhere too) is this JSENUMERATE_PROTO required once we use
> proxies (bug 546590) with native handlers instead of the current wrappers?

I'd rather not back out bug 558754, which has a huge perf win (txul as well as sunspider) while we can wait for a 230k patch to land. Wouldn't it be better to break the API for a time?
Theoretically we're extending the API, whatever we do -- not breaking it. Before I get the bum's rush though, can we argue about the best way to extend it? A new JSENUMERATE_ cookie that's always called by InitNativeIterator is costlier than a special return protocol from JSENUMERATE_NEXT. The latter may be called uglier, but we want something ugly here. We want to rip this out "soon".

/be
I am going to disable some tests while we wait for a fix here... but, please, land a fix promptly.

http://hg.mozilla.org/tracemonkey/rev/8bad6b120614
Andreas convinced me that -- solely in the name of wrappers and e4x (I will not let him live this down :-P) -- the patch is the expedient fix. It'll all go away soon, we hope.

/be
Summary: TM: wrappers are not properly enumerated by JS_Enumerate → TM: redirecting into js_Enumerate does not invoke the old enumeration hook
We found a simple fix. I will file the protocol change in a different bug.
Attached patch FixSplinter Review
Much simpler: we can improve the API elsewhere.
Assignee: gal → mrbkap
Attachment #444700 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #445003 - Flags: review?(gal)
Comment on attachment 445003 [details] [diff] [review]
Fix

enumerate is mandatory so no NULL check
Attachment #445003 - Flags: review?(gal) → review+
http://hg.mozilla.org/mozilla-central/rev/1b90a71c6791
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
FWIW - this patch fixes my new-resolve-op jsapi-test.

A version of tracemonkey from May 4th was failing by not calling any code on the inside of my for...in even though the callback was sending back valid jsids.

I didn't bother digging to find out WHY the old jsapi failed, but I'm commenting here just in case this gets blamed for some other bug.  And so you know that the "make-it-faster" re-write appears to have also made-it-better.
You need to log in before you can comment on or make changes to this bug.