346144 - for-in iterator fails on JS 1.7

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Mike Moening]

There is a bug in the for-in iterator which causes scripts to throw exceptions.
Exception is rendered meaningless by bug #345734

This bug involves registering prototypes to persist across multiple executions or a script. This is key functionilty in any RPC framwork.
This bug it NOT related to threads or synronization.

This functionality works fine in JS 1.5 and JS 1.6.
Failure occurs in JS 1.8 HEAD.

The script that causes the failure is only 20 lines long (see attachment)
A single threaded tester app will be attached shortly.

Comment 1

[Mike Moening]

Attachment: main.cpp and main.h for testing the bug

These two files should compile on any platform.  The program is single threaded.

Comment 2

[Brendan Eich [:brendan]]

JS1.7 is the new version -- confusing in light of the MOZILLA_1_8_BRANCH.  But the HEAD is Mozilla 1.9, so the numbers are all just confusing.

/be

Comment 3

[Brendan Eich [:brendan]]

We lost API compat, that's bad.  Blake, you remember how js_SetClassObject and js_GetClassObject are supposed to work in the face of no JSCLASS_IS_GLOBAL?

MikeM: Short-term fix is to add JSCLASS_GLOBAL_FLAGS to your globa object class's flags initialiser.

/be

Comment 4

[Mike Moening]

Ok. I'll look at JSCLASS_GLOBAL_FLAGS and see what impact that has.

It appears the compiled script will only work on the JSContext it was initially compiled with. Even after doing the following to wipe it and the global clean:

JS_ClearNewbornRoots
JS_ClearRegExpStatics
JS_ClearPendingException
JS_ClearScope

My question then is: What if anything is different about the 2nd context & global that makes the iterator fail when executing the compiled script with the pair?

Comment 5

[Mike Moening]

>MikeM: Short-term fix is to add JSCLASS_GLOBAL_FLAGS to your globa object
>class's flags initialiser.

I tried that.  Now when I attempt to initialize standard classes on Context #1 for the 2nd time (after wiping the global and context) I get an assertion failure in JS_InitClass() for the js_FunctionClass. Assertion code below:

------------
/* Bootstrap Function.prototype (see also JS_InitStandardClasses). */
if (OBJ_GET_CLASS(cx, ctor) == clasp) {
     JS_ASSERT(!OBJ_GET_PROTO(cx, ctor));    <- ASSERTS HERE
-----------
The only good thing is that execution on the 2nd context now works (at least once)

Comment 6

[Brendan Eich [:brendan]]

It's a regression due to our attempt to fix an old ECMA-262 standards bug (bug 304376). More in a bit.

/be

Comment 7

[Brendan Eich [:brendan]]

This bug is invalid.  The problem is that, while you properly precompile the script, root its object, etc., and execute it twice, the script itself gets a global oSavedBroker via a truly (C++) global attribute map, and the second time that reference is non-null, which means you never initialize tester.prototype methods from their properly-cloned anonymous functions.

So we get the value of the 'testbroker' attribute, also from the truly global attribute map, and that's an object scoped by context #1's global object, which has been clear-scoped.  This object is stored in page.broker, aka broker.  Then the script calls (last line) broker.loadClasses(page), which is calling the old context/global pair's broker object's loadClasses method, again scoped by the old global (context #1's global).

At this point you are doomed.  You've crossed the streams.

With JSCLASS_GLOBAL_FLAGS, you get further, but the third execute fails when initializing standard classes, because the streams were crossed and the global flags enabled magic creation of Iterator in the scope of the function (global #1) for the second execute (on context #2), but that messed up the state of global #1.

If you need to depend on truly-global data across JS globals, you'll need to do the JS_CloneFunctionObject calls yourself -- you can't write anonymous function initializers for prototype methods where the initializing assignments do not execute on the 2nd thru Nth executes, because the global oSavedBroker is non-null.

Hope this helps. :-)

Thanks to mrbkap for helping walk through all this.

/be

Comment 8

[Mike Moening]

I'm not sure I understand 100% of what you wrote.
One question: 
Why does this work on JS 1.5 AND 1.6 then?

>If you need to depend on truly-global data across JS globals, you'll need to do
>the JS_CloneFunctionObject calls yourself

Given the sample code provided can you show me how to properly call JS_CloneFunctionObject() and where to implement this call to make it work?
Thanks for the help.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> I'm not sure I understand 100% of what you wrote.
> One question: 
> Why does this work on JS 1.5 AND 1.6 then?

Because they don't depend on looking up "Iterator" in the scope chain to implement for-in loops.  This is a new feature of JS1.7.  You could argue (and you probably should) that we still should find a way to keep compatibility.  Let me think about that for a minute...

> >If you need to depend on truly-global data across JS globals, you'll need to do
> >the JS_CloneFunctionObject calls yourself
> 
> Given the sample code provided can you show me how to properly call
> JS_CloneFunctionObject() and where to implement this call to make it work?
> Thanks for the help.

I wouldn't do it.  It's odd to have to do it by hand.  I would just initialize the tester.prototype methods on every script execute.

/be

Comment 10

[Brendan Eich [:brendan]]

Another resolution, not that it matters much, would be WONTFIX.  We are breaking compatibility, but for a buggy case that had other problems (the wrong scope for functions, e.g.).  There's just no way to keep compatibility and support the new iteration protocol.  This is a case of breaking bug-for-bug compatibility; I'm sorry we're doing it, but sometimes it's necessary.

/be

Comment 11

[Mike Moening]

>If you need to depend on truly-global data across JS globals, you'll need to do
>the JS_CloneFunctionObject calls yourself -- you can't write anonymous function
>initializers for prototype methods where the initializing assignments do not
>execute on the 2nd thru Nth executes, because the global oSavedBroker is
>non-null.

We are trying to create a simple mechanism for scripts to save objects & functions and re-use them in later contexts/threads in different script executions. We have no control over what kind of JS objects are saved by clients.  Can you suggest a general mechanism for accomplishing this task?  
If JS_CloneFunctionObject is the secret...exactly how and when do we use it?

I've since tried using a 2nd global object for resolving the standard classes which lives as long as the attribute map.  That at least keeps the standard classes the same for all objects regardless of which context and execution its being run on.  That seems to help but I doubt that solves the problem you described above.

Your help would be greatly appreciated.