295606 - Reentry of JS_ResolveStandardClass due to fix for bug 289675

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

STEPS TO REPRODUCE:

1)  Start build
2)  Breakpoint in 'js_InitQNameClass'
3)  Open a new window
4)  Continue from breakpoint.

Note that now you're back at the breakpoint.  The stack to get here looks
something like (leaving out some parts for clarity):

#0  0xb7fe5f7c in js_InitQNameClass (cx=0x89076e8, obj=0x85f29d8)
    at ../../../mozilla/js/src/jsxml.c:7340
#1  0xb7f3b039 in JS_ResolveStandardClass (cx=0x89076e8, obj=0x85f29d8,
id=135445964, 
    resolved=0xbfff9758) at ../../../mozilla/js/src/jsapi.c:1425
#2  0xb6ecf24f in nsWindowSH::NewResolve (this=0x8510b70, wrapper=0x8a37870, 
    cx=0x89076e8, obj=0x85f29d8, id=135445964, flags=16, objp=0xbfff9810, 
    _retval=0xbfff9814) at ../../../../mozilla/dom/src/base/nsDOMClassInfo.cpp:5083
#3  0xb79a61e4 in XPC_WN_Helper_NewResolve (cx=0x89076e8, obj=0x85f29d8, 
    idval=135445964, flags=16, objp=0xbfff9954)
    at ../../../../../mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:966
#4  0xb7f9b5b1 in js_LookupPropertyWithFlags (cx=0x89076e8, obj=0x85f29d8,
id=135527808, 
    flags=16, objp=0xbfff99d4, propp=0xbfff99d0) at
../../../mozilla/js/src/jsobj.c:2542
#5  0xb7f99cfa in js_FindConstructor (cx=0x89076e8, start=0x0, 
    name=0xb7ff9a68 "AttributeName", vp=0xbfff9a1c)
    at ../../../mozilla/js/src/jsobj.c:1955
#6  0xb7f9f65b in GetClassPrototype (cx=0x89076e8, scope=0x85f29d8, 
    name=0xb7ff9a68 "AttributeName", protop=0xbfff9aa4)
    at ../../../mozilla/js/src/jsobj.c:3643
#7  0xb7f99663 in js_NewObject (cx=0x89076e8, clasp=0xb7fff480, proto=0x0, 
    parent=0x85f29d8) at ../../../mozilla/js/src/jsobj.c:1836
#8  0xb7f3bd74 in JS_InitClass (cx=0x89076e8, obj=0x85f29d8, parent_proto=0x0, 
    clasp=0xb7fff480, constructor=0xb7fd4f17 <AttributeName>, nargs=2,
ps=0xb7fff540, 
    fs=0xb7fff570, static_ps=0x0, static_fs=0x0) at
../../../mozilla/js/src/jsapi.c:1955
[...]
#11 0xb7f3ac48 in JS_InitStandardClasses (cx=0x89076e8, obj=0x85f29d8)
    at ../../../mozilla/js/src/jsapi.c:1182
#12 0xb795b37d in nsXPConnect::InitClassesWithNewWrappedGlobal (this=0x8128b88, 

Note that since js_InitQNameClass inits three classes, we'll reenter it three
times, so we'll init each of those classes three times.

Is that really desirable behavior?  It seems pretty wacky to me...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that BackstagePass::NewResolve leads to similar code flow...

Comment 2

[Brendan Eich [:brendan]]

I have a fix for this already under construction for bug 292903.

/be

Comment 3

[Brendan Eich [:brendan]]

Waitaminute -- why are we using both JS_InitStandardClasses (frame 11) and
JS_ResolveStandardClass (frame 1)?  One or the other, not both?

I am fixing js_InitQNameClass over in but 292903, but there seems to be a
separate bug here.  jst, didn't we at one point have no JS_InitStandardClasses
calls when (re-)initializing a window object, and only the
JS_ResolveStandardClass call in nsWindowSH::NewResolve?

/be

Comment 4

[Brendan Eich [:brendan]]

bz: what was frame #13?

/be

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Waitaminute -- why are we using both 

One's XPConnect initing a new global object, while the other is the resolve hook
in nsWindowSH classinfo.

> and only the JS_ResolveStandardClass call in nsWindowSH::NewResolve?

Right, but class init ends up resolving on the global, which calls into
nsWindowSH::NewResolve.  See stack.

> bz: what was frame #13?

There are two varieties of frame #13.  nsJSContext::InitContext and
mozJSComponentLoader::GlobalForLocation.  The former happens any time a new
window is created; the latter at startup.

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)

> > and only the JS_ResolveStandardClass call in nsWindowSH::NewResolve?
> 
> Right, but class init ends up resolving on the global, which calls into
> nsWindowSH::NewResolve.  See stack.

I know, but that's a bug (if not *the* bug).  If we're lazily resolving standard
classes, we must not call JS_InitStandardClasses.  If we are eagerly defining
standard classes, we must call JS_InitStandardClasses but not call
JS_ResolveStandardClass from our global class's resolve hook.  The JS APIs and
underlying engine code can't work with both eager and lazy code paths operating
for a given global.

> > bz: what was frame #13?
> 
> There are two varieties of frame #13.  nsJSContext::InitContext and
> mozJSComponentLoader::GlobalForLocation.  The former happens any time a new
> window is created; the latter at startup.

But neither calls JS_InitStandardClasses directly.  Only if the
nsIXPConnect::INIT_JS_STANDARD_CLASSES flag is passed into
nsIXPConnect::InitClassesWithNewWrappedGlobal would that method call
JS_InitStandardClasses.  I'm still having double-auto-reg woes, but I will try
to gdb in a bit.  If anyone could say what frame 13 was exactly (and frame 14,
given the layering), that would be swell.

/be

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> (In reply to comment #5)
> > There are two varieties of frame #13.  nsJSContext::InitContext and
> > mozJSComponentLoader::GlobalForLocation.  The former happens any time a new
> > window is created; the latter at startup.
> 
> But neither calls JS_InitStandardClasses directly. 

Duh, sorry -- pre-caffeine here.  I was off by one in my frame memory and didn't
even look at the stack in comment 0.

Still, unless someone is wrongly passing nsIXPConnect::INIT_JS_STANDARD_CLASSES
to nsIXPConnect::InitClassesWithNewWrappedGlobal, I don't see how that method
can call JS_InitStandardClasses.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: argh! thanks to jst for noticing this copy/paste bug

I'm going to check in with r+sr=jst.

/be

Comment 9

[Brendan Eich [:brendan]]

Checked in.  I'll fix the rest in 292903.

/be

Comment 10

[Daniel Veditz [:dveditz]]

bug 289675 landed on the aviary branch -- do we need this fix there as well?

Comment 11

[Boris Zbarsky [:bzbarsky]]

No, based on that patch I think that the XPCNativeWrapper changes were needed in
addition to bug 289675.  Brendan, correct me if I'm wrong please!

Comment 12

[Brendan Eich [:brendan]]

We didn't land any of the flag-system-object stuff on the branch.

/be