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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Andreas Gal :gal]

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"

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :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.

Comment 3

[Andreas Gal :gal]

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

Comment 4

[Igor Bukanov]

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

Comment 5

[Andreas Gal :gal]

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.

Comment 6

[Andreas Gal :gal]

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.

Comment 7

[Brendan Eich [:brendan]]

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

Comment 8

[Igor Bukanov]

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

Comment 9

[Igor Bukanov]

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

Comment 10

[Andreas Gal :gal]

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.

Comment 11

[Andreas Gal :gal]

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.

Comment 12

[Andreas Gal :gal]

If we take this change, it must happen before the beta.

Comment 13

[Brendan Eich [:brendan]]

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

Comment 14

[Robert Sayre]

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

Comment 15

[Brendan Eich [:brendan]]

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

Comment 16

[Robert Sayre]

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

Comment 17

[Brendan Eich [:brendan]]

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

Comment 18

[Andreas Gal :gal]

We found a simple fix. I will file the protocol change in a different bug.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

Much simpler: we can improve the API elsewhere.

Comment 20

[Andreas Gal :gal]

Comment on attachment 445003 [details] [diff] [review]
Fix

enumerate is mandatory so no NULL check

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/1b90a71c6791

Comment 22

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/1b90a71c6791

Comment 23

[Wesley W. Garland]

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.