Closed Bug 418264 Opened 16 years ago Closed 10 years ago

resolve hook not fired for literal standard classes

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: MikeM, Unassigned)

Details

When declaring variables using literal method, the resolve hook is not fired.

These examples no not fire my globals resolver hook:
---------------------
  var recArray = [1, 2, 3]; 
  var recObj = {}; 
  var recRegexp = /i*/i;

These examples do:
---------------------
  var recArray  = new Array(1, 2, 3); 
  var recObj    = new Object();
  var recRegexp = new RegExp("*");

This causes problems since we have no control over where the standard classes are being resolved from.  
i.e. we have more than 1 global object in play (request and session globals)

Ultimately in a more complex example this causes "instanceof" to fail when using literals.
>These examples no not fire my..

should read:

These examples do not fire my...
Are you setting JSCLASS_GLOBAL_FLAGS in your global object's flags initialiser? You should be.

/be
>Are you setting JSCLASS_GLOBAL_FLAGS in your global object's flags initialiser?
Yes. See definition of globals below:

------------------------------------
JSClass session_global_class = {
    "session_global", 
    JSCLASS_HAS_PRIVATE|JSCLASS_GLOBAL_FLAGS|JSCLASS_NEW_RESOLVE,
    JS_PropertyStub,JS_PropertyStub,JS_PropertyStub, JS_PropertyStub,
    JS_EnumerateStub, (JSResolveOp) session_global_resolve, JS_ConvertStub,JS_FinalizeStub
};

JSClass request_global_class = {
    "request_global", 
    JSCLASS_GLOBAL_FLAGS|JSCLASS_NEW_RESOLVE,
    JS_PropertyStub,JS_PropertyStub,JS_PropertyStub,JS_PropertyStub,
    JS_EnumerateStub, (JSResolveOp) request_global_resolve, JS_ConvertStub, RequestGlobalDestructor,
    JSCLASS_NO_OPTIONAL_MEMBERS
};
---------------------------

I forgot to mention 1 other global we have in the mix.
We only create 1 of these for each script we compile and cache in memory.
It is ONLY used for compiling each script (JS_CompileUCScript) and never referenced again.  This global along with the compiled JSScript is rooted using JS_AddNamedRootRT and saved forever.

JSClass compile_global_class = {
    "compile_global", 
    JSCLASS_GLOBAL_FLAGS,
    JS_PropertyStub,JS_PropertyStub,JS_PropertyStub,JS_PropertyStub,
    JS_EnumerateStub,JS_ResolveStub,JS_ConvertStub,JS_FinalizeStub,
    JSCLASS_NO_OPTIONAL_MEMBERS
};

When defining an array like:
 var recArray = [1, 2, 3]; 
the JSOP_NEWINIT op code is used shown below:

--------------
         BEGIN_CASE(JSOP_NEWINIT)
            i = GET_INT8(pc);
            JS_ASSERT(i == JSProto_Array || i == JSProto_Object);
            SAVE_SP_AND_PC(fp);
            obj = (i == JSProto_Array)
                  ? js_NewArrayObject(cx, 0, NULL)
                  : js_NewObject(cx, &js_ObjectClass, NULL, NULL);
            if (!obj)
                goto out;
            PUSH_OPND(OBJECT_TO_JSVAL(obj));
            fp->sharpDepth++;
            LOAD_INTERRUPT_HANDLER(cx);
          END_CASE(JSOP_NEWINIT)
---------

How does the global resolver hook interact here?  
I'm over my head.
Any help would be appreciated...
Why do you want the resolve hook called there? It won't be if eager standard class init has been done, or after the first resolve has happened if you're using lazy standard class init.

On your compilation global, why not pass a null obj into the JS_Compile* APIs? I believe that works now (file bugs if not). No need for compilation globals these days.

/be
>Why do you want the resolve hook called there? 
I'm just trying to follow the code that is generated using literal versus "new Array()"

>It won't be if eager standard class init has been done, or after the first >resolve has happened if you're using lazy standard class init.
What do you mean by "eager standard class init"?  I've never initialized the stardard classes at all. Resolver hasn't done it yet either.  The array seems to be "magic" unless I use the "new" pattern.

>No need for compilation globals these days.
If that is true then great!
Back on 8/21/06 I asked the following question and you responded:

>> The compiled script has "pointers" to the global object for those  
>> anonymous functions?
>Yes.  Those are pointers-to, not pointers-from, so are not mutations  
>of the global.

So you are saying that is no longer the case? The script doesn't store anything in the global you compile it with?

The ES3 language spec says o = {p:42} desugars to o = new Object; o.p = 42 or someting like that, similarly for array initialiser vs. Array -- but not all browsers agree, and anyway that's a perf and security hazard, and ES4 is changing to say initialisers create memoized type instances.

So you shouldn't be bugged by this. It simply is a shift in language definition and you don't need to worry about resolve. If you think the wrong global's Array constructor or Object constructor is being used, please say why.

> So you are saying that is no longer the case? The script doesn't store
> anything in the global you compile it with?

Compiling has not mutated the compilation object in years, and should not. Hence the ability to pass null. Executing scripts is the only way to bind their vars and functions.

/be
About to mark this INVALID. Speak fast if there's a bug in which global's Array is used.

/be
>So you shouldn't be bugged by this. It simply is a shift in language definition
>and you don't need to worry about resolve. If you think the wrong global's
>Array constructor or Object constructor is being used, please say why.
I only define standard classes lazily on the "session global".  All accesses to any standard classes are resolved from that global using the resolve hook.

Here is a quick rundown of what is happening:
The session.include method actually evaluates the contents of the file on the session global. The idea is to keep function defs around for later use by other requests.
The remainder of the code is executed against the "request global"

The 1st instanceof test returns "yes" and the 2nd returns "no".
--------------------------------------
session.include("arraytest.mwsi");
var reqArray = [1, 2, 3, 4];

reqArray instanceof Array?"yes" : "no";  <--- RETURNS yes
isArray(reqArray);                       <--- RETURNS no
--------------------------------------

arraytest.mwsi file contents:
--------------------------------------
var sessArray = [1, 2, 3, 4];
function isArray(a){
  if(a instanceof Array) return "yes";
  return "no";
}
--------------------------------------

If I remove the the line "var sessArray = [1, 2, 3, 4];"
The both instanceof tests return yes.

Why??

If I replace 
  var reqArray = [1, 2, 3, 4];
with:
  var reqArray = new Array(1, 2, 3, 4);

Then both intanceof tests return yes. Presumably because "Array" is lazily defined and resolved on the session global...


>Compiling has not mutated the compilation object in years, and should not.
>Hence the ability to pass null. Executing scripts is the only way to bind their
>vars and functions.
You told me about a year ago I needed to have a separate "compile global" to keep things pointers from going out of scope if the script compiled and was rooted and lives past the compile global.
Which bug fixed this problem?

I think I found the root of the problem.
Because the array literal way of creating a array does not use the resolver hook we end up with 2 array prototypes, one on the "request global" and one on the "session global".

This causes js_IsDelegate() to fail because it it comparing pointer addresses for the prototypes which are different.  Meaning when calling the "isArray()" function it finds the array prototype on the session and is comparing it with the prototype on the request which is different.

js_IsDelegate code below:
--------------------------

JSBool
js_IsDelegate(JSContext *cx, JSObject *obj, jsval v, JSBool *bp)
{
    JSObject *obj2;

    *bp = JS_FALSE;
    if (JSVAL_IS_PRIMITIVE(v))
        return JS_TRUE;
    obj2 = JSVAL_TO_OBJECT(v);
    while ((obj2 = OBJ_GET_PROTO(cx, obj2)) != NULL) {
        if (obj2 == obj) {
            *bp = JS_TRUE;
            break;
        }
    }
    return JS_TRUE;
}
------------------------

Possible solutions:

1) Fix lazy class resolution to work with literal (Array, RegExp, Object) creation
or

2) Fix js_IsDelegate() and hence hasInstance to not compare pointer addresses.

Thoughts?
(In reply to comment #10)
> >Compiling has not mutated the compilation object in years, and should not.
> >Hence the ability to pass null. Executing scripts is the only way to bind their
> >vars and functions.
> You told me about a year ago I needed to have a separate "compile global" to
> keep things pointers from going out of scope if the script compiled and was
> rooted and lives past the compile global.
> Which bug fixed this problem?

I didn't say that, since it contradicts what you cited me saying in comment 7:

>Yes.  Those are pointers-to, not pointers-from, so are not mutations  
>of the global.

The only issues were bugs to fix where we didn't tolerate a null obj param to the JS_Compile* APIs, and where we needed compile-time function objects to have a non-null parent slot. Igor worked with dbaron to get rid of this, I believe, since they make for leaks.

Anyway, if I'm wrong, please file a bug. You should be able to JS_Compile* against a null scope obj.

(In reply to comment #11)
> I think I found the root of the problem.
> Because the array literal way of creating a array does not use the resolver
> hook we end up with 2 array prototypes, one on the "request global" and one on
> the "session global".

The array initialiser way does use a resolve hook, but it seems you are using two different globals (objects at the ends of scope chain) but expecting one. Do you have a non-stub resolve hook for both of those globals? If so, put a printf in each when "Array" is resolved and you should see it fire once for the first object initialiser evaluated, if nothing else has triggered lazy Array standard class init.

> This causes js_IsDelegate() to fail

That's one symptom. Don't treat symptoms.

> 1) Fix lazy class resolution to work with literal (Array, RegExp, Object)
> creation
> or

Lazy class resolution does work with initialisers, but you seem to have two scope chains ending in different globals, at different times. Let's dig into that to find the cause.

> 2) Fix js_IsDelegate() and hence hasInstance to not compare pointer addresses.

No, that is insufficient (there are other things that can tell the difference, such as prototype and __proto__ comparisons, and ad-hoc property gets and sets) as well as incorrect by the ECMA spec.

/be

Repeating something I suggested elsewhere: don't use multiple globals, it is complicated and confusing (for embedders and users). Anything used for session storage should be a non-global object that has to be addressed by "request" script. Any shared functions should be available to all requests. Session objects should be hashes, not actual scope (global) objects for running scripts.

/be
>You should be able to JS_Compile* against a null scope obj.
So far compiling with null scope is working. I have to check anonymous functions yet which I think were related to needing the "compile global" in the first place.

>The array initialiser way does use a resolve hook, but it seems you are using
>two different globals (objects at the ends of scope chain) but expecting one.
>Do you have a non-stub resolve hook for both of those globals? 
Yes. See comment #3.

>If so, put a
>printf in each when "Array" is resolved and you should see it fire once for the
>first object initialiser evaluated, if nothing else has triggered lazy Array
>standard class init.
The resolve hooks are not fired for "Array" on either global.  Which is why I filed this bug. If it was there would only be 1 Array protoyype instead of 2.
No lazy array standard class init has been called either.

JSOP_NEWINIT 
simply calls js_NewArrayObject(cx, 0, NULL)
which calls js_NewObject()
which calls js_GetClassPrototype() with id = 7
which calls OBJ_GET_PROPERTY(cx, ctor, ATOM_TO_JSID( cx->runtime->atomState.classPrototypeAtom),&v)
which calls js_FindClassObject()
which calls js_GetClassObject() which executes this:
    init = lazy_prototype_init[key];
    if (init) {
        if (!init(cx, obj)) {
with key = JSProto_Array and init set to js_InitArrayClass()

Nowhere in this call stack does either of the resolver hooks get fired.

>Repeating something I suggested elsewhere: don't use multiple globals, it is
>complicated and confusing (for embedders and users). Anything used for session
>storage should be a non-global object that has to be addressed by "request"
>script. Any shared functions should be available to all requests. Session
>objects should be hashes, not actual scope (global) objects for running
>scripts.
No exactly sure what you mean by this.  In order to save function definitions for later use in multiple requests I need a session global.  That's what the session.include() does.  Evaluates code on the session global which can be accessed by any request using the request resolver hook and JS_LookupUCProperty() on the session global.  I'm open to ideas here...

Please see my hack/fix below of js_IsDelegate() which looks at the class instead of the prototype. This is probably not done right but it does fix the problem.
Comments?
------------------------------------------
JSBool
js_IsDelegate(JSContext *cx, JSObject *obj, jsval v, JSBool *bp)
{
    JSObject *obj2;
    JSClass* pClass = OBJ_GET_CLASS(cx, obj);

    *bp = JS_FALSE;
    if (JSVAL_IS_PRIMITIVE(v))
        return JS_TRUE;
    obj2 = JSVAL_TO_OBJECT(v);

    while ((obj2 = OBJ_GET_PROTO(cx, obj2)) != NULL) {
        if(OBJ_GET_CLASS(cx, obj2) == pClass) {
            *bp = JS_TRUE;
            break;
        }
    }
    return JS_TRUE;
}
--------------------
(In reply to comment #14)
> >You should be able to JS_Compile* against a null scope obj.
> So far compiling with null scope is working. I have to check anonymous
> functions yet which I think were related to needing the "compile global" in the
> first place.

Yay, good news so far. Keep me posted. See bug 398219 where Igor's work was done with dbaron helping.

> >The array initialiser way does use a resolve hook, but it seems you are using

I'm wrong, I was skimming and misremembering -- sorry. If you use JSCLASS_GLOBAL_FLAGS then (although the resolving table is used to suppress runaway recursion, as when calling resolve hooks) it is the class init function that's lazily called, not a lookup=>resolve hook.

> Nowhere in this call stack does either of the resolver hooks get fired.

Right, and that's a feature. Let's get to the underlying bug, which is not this ES4/JS2 oriented, high-integrity-value change.

> No exactly sure what you mean by this.  In order to save function definitions
> for later use in multiple requests I need a session global.

No, you just need the functions to be available somehow. Could use a prototype of the request global, or resolve hook to fault them in. Several ways to skin this cat.

> Please see my hack/fix below of js_IsDelegate() which looks at the class
> instead of the prototype. This is probably not done right but it does fix the
> problem.

I already implored not to hack on symptoms. That's not going to land as a patch we can support. We need to find why you have two different scope chains, one used to find the global with a reserved slot initialized by js_InitArrayClass when the array initialiser is evaluated, and a different one in which 'Array' is found when a 'new Array' call is made.

/be
Removing JSCLASS_GLOBAL_FLAGS from the request global class seems to have solved the problem.  I left this flag in the session global and all works Ok.
Don't know if this is desirable but it works.

>No, you just need the functions to be available somehow. Could use a prototype
>of the request global, or resolve hook to fault them in. Several ways to skin
>this cat.
This is exactly what we are doing.  Functions are saved on the session global and are resolved from there using the request globals resolver hook.

The problem was the "reserved slots" were bypassing my resolver hook.


Glad you are happy turning off memo-ized class objects -- but do you really have coherent Array initialisers and new Array results? If someone does this:

Array.prototype.test = function () { print('hi!'); }
try { new Array().test(); } catch (e) { print('1:', e); }
try { [1,2,3].test(); } catch (e) { print('2:', e); }

do you get two prints of 'hi!'? If so, then add back JSCLASS_GLOBAL_FLAG and test again, let me know what you see.

/be
>Glad you are happy turning off memo-ized class objects
It just has to work.  I've never heard the term "memo-ized".  I'm happy since it works. However I have no clue what I've lost in the process.

If you define a resolver hook why would you ever want to bypass it?
What's the point of having a lazy class resolver if it only gets used sometimes and you maintain several copies of the prototype?

>do you get two prints of 'hi!'?
Yes I got two.

>If so, then add back JSCLASS_GLOBAL_FLAG and
>test again, let me know what you see.
I get "TypeError: [1, 2, 3].test is not a function"

FYI 

As a side effect I've noticed JS_CompileUCScript() is now calling my request resolver hook, which I've had to fix as it causes asserts in JS.
Seems to work now if I set a flag so the resolver does nothing when I'm initializing my objects and scripts etc.
Follow up to the FYI:
Heres the assert I hit inside js_InitFunctionAndObjectClasses():

JS_ASSERT(!entry->key.obj && entry->flags == 0);

I think this is fixed (wfm) or wontfix, not sure. Mike?

/be
Marking as INVALID based on the fact that the way the resolve stuff works has changed quite drastically, so it's very, very likely that this bug is either plain fixed, or has very different characteristics.

Also, without STR it's hard to investigate this further, anyway.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.