651214 - Assertion failure: obj->isGlobal() (JSD sees JSFunctions with no global object)

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

I'm getting a fast assertion failure when running current TM (59325b2ca38b) against Firebug 1.7:

Assertion failure: obj->isGlobal(), at /home/sfink/src/TM-singlestep/js/src/jsobj.cpp:6575

This is because JSD is getting a js_CallNewScriptHook with a JSFunction that has no parent. Then when jsdScript::GetFunctionSource (in jsd_xpc.cpp) tries to use the function to enter a cross-compartment call, it asserts because it's trying to get a global object for a scope chain off of that function.

I tried using IsInternalFunctionObject() to conceal some function objects from JSD, which "fixed" some paths, but there are other ways to get the same thing.

I tried parenting fun in JSScript::NewScriptFromCG to cx->globalObject, but now I'm getting a failure in AssertScopeChainValidity in the DirectEval path:

Assertion failure: op(cx, o) == &scopeobj, at /home/sfink/src/TM-singlestep/js/src/jsobj.cpp:952

Program received signal SIGABRT, Aborted.
0x000000379e20ed8b in raise () from /lib64/libpthread.so.0
(gdb) up
#1  0x00007ffff3a174ac in JS_Assert (s=0x7ffff3bce566 "op(cx, o) == &scopeobj", file=0x7ffff3bce470 "/home/sfink/src/TM-singlestep/js/src/jsobj.cpp", ln=952) at /home/sfink/src/TM-singlestep/js/src/jsutil.cpp:89
(gdb) 
#2  0x00007ffff395de07 in AssertScopeChainValidity (cx=0x7fffdd5bb400, scopeobj=...) at /home/sfink/src/TM-singlestep/js/src/jsobj.cpp:952
(gdb) p &scopeobj
$31 = 0x7fffe81023a8 [Object Call]
(gdb) p o
$32 = 0x7fffe8102068 [Object Proxy]
(gdb) p op(cx, o)
$33 = 0x7fffe6747090 [Object Window]

I don't know if this is caused by my fun->setParent(cx->globalObject), or if I just made it past that problem to the next.

Comment 1

[Jim Blandy :jimb]

You almost certainly ,

Comment 2

[Luke Wagner [:luke]]

So:
1) there was a typo in AssertScopeChainValidity, just fixed on TM, so update may make that assert go away.
2) I don't think its right to parent anything to cx->globalObject.  In fact, the field is going away soon because it has a very different meaning that what its name implies.

Instead, perhaps you should set the parent of the JSFunction's object to be JS_GetCurrentScopeChain(cx)?  Does that sound right Blake?  I know we do special dances for the magic JSObject-in-JSFunction in other places...

Comment 3

[Luke Wagner [:luke]]

I seem to have cut jimb off mid-sentence ;)

Comment 4

[Jim Blandy :jimb]

[How helpful.]
You almost certainly don't want to be changing anything's parent. The way that stuff is set up is delicate. I think GetFunctionSource needs to find a way to do what it needs without assuming that there's a global there.

Comment 5

[Steve Fink [:sfink] [:s:]]

I am working around this issue for now by always using the script to enter the compartment, never its function object. Using the script means constructing a dummy global object purely for the purpose of doing the cross-compartment call, which is gross. But if it's really ok for JSD-observed JSFunctions to have NULL parents, then we'd probably have to do the same thing with JSFunction in addition to JSScript. (But here, they're always in the same compartment, so using the JSScript is vaguely reasonable. I just don't like it.)

Comment 6

[Steve Fink [:sfink] [:s:]]

(In reply to comment #4)
> [How helpful.]
> You almost certainly don't want to be changing anything's parent. The way that
> stuff is set up is delicate. I think GetFunctionSource needs to find a way to
> do what it needs without assuming that there's a global there.

In this case, I'm adding a parent to something that doesn't have one: a newborn JSFunction from NewScriptFromCG. Still think it's bad?

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Set the parent of CG-created JSFunctions

Luke, you were right, so I'll stick you with the review. If I parent it to JS_GetScopeChain(cx) and update my tree, it all pretends to be happy.

I've no idea what the difference is between JS_GetScopeChain vs JS_GetGlobalForScopeChain. And JS_GetCurrentScopeChain failed its saving throw vs existence.

Comment 8

[Luke Wagner [:luke]]

OOC, is this still necessary in light of bug 645160 comment 55?

Comment 9

[Steve Fink [:sfink] [:s:]]

(In reply to comment #8)
> OOC, is this still necessary in light of bug 645160 comment 55?

Yes. I hit this assertion with the current TM tip. I don't know why it doesn't always hit everyone who runs a debug build, but I *am* running some weirdo intermediate Firebug 1.7, so perhaps it is specific to that. (For all I know, Firebug *was* hitting it and modified the code to avoid the problem before the 1.7 release.)

When I get back to a computer with source code, I'll check if maybe .functionSource got removed at some point. Also CC'ing Firebug people.

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 527139 [details] [diff] [review]
Set the parent of CG-created JSFunctions

Ah.  In that case, I'd like to defer to mrbkap as to whether this is actually a valid thing to do.

Comment 11

[Jim Blandy :jimb]

I don't have any actual knowledge to share, but I'd say, if something doesn't have a parent now, and giving it one doesn't work straight out of the box, it's best to leave it alone. For whatever it's worth.

Comment 12

[Steve Fink [:sfink] [:s:]]

(In reply to comment #9)
> (In reply to comment #8)
> > OOC, is this still necessary in light of bug 645160 comment 55?
> 
> Yes. I hit this assertion with the current TM tip. I don't know why it doesn't
> always hit everyone who runs a debug build, but I *am* running some weirdo
> intermediate Firebug 1.7, so perhaps it is specific to that. (For all I know,
> Firebug *was* hitting it and modified the code to avoid the problem before the
> 1.7 release.)

So I get this very easily with my slightly modified copy of an intermediate version of Firebug 1.7, but I've gotten it once now with the unmodified current latest 1.7.

Comment 13

[Steve Fink [:sfink] [:s:]]

Ugh. In attempting to investigate a different bug, I'm getting a very similar problem with another object with no global. This time, it's a 'Block' object, and it happens with multiple version of Firebug. I will attach the test code and the stack.

In this case, an object that does not descend from a global is again used to make a cross-compartment call, but this time it was triggered from jsdValue::GetWrappedValue. So the attachment above doesn't fix this one (it parents Function objects; this crash would need to parent Block objects, whatever they are.)

I'm guessing that the common thread here is that JSD is observing internal values. But I'm not sure whether the bug is that JSD shouldn't see (or should explicitly ignore) those values, or in this post-compartments world those internal values are now unsafe (because something might use them to enter a compartment).

It does strike me that if compartments had a single global object, then this particular problem would be solved.

Comment 14

[Steve Fink [:sfink] [:s:]]

Attachment: test case

STR:
1. Save this attachment in /tmp/error2.html
2. Go to file:///tmp/error2.html
3. Enable Firebug
4. Click the link

Give it a little while to run out of stack.

Comment 15

[Steve Fink [:sfink] [:s:]]

Attachment: Block crash backtrace

Backtrace for the assertion failure with the Block object.

Comment 16

[Steve Fink [:sfink] [:s:]]

Attachment: Set the parent of CG-created JSFunctions

In weird cases, the context doesn't have the right scope. Fortunately, the parent appears to already be set in such cases. Ok, one such case.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 528778 [details] [diff] [review]
Set the parent of CG-created JSFunctions

Review of attachment 528778 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay in review here :-/

::: js/src/jsscript.cpp
@@ +1366,5 @@
>  #endif
>          if (cg->flags & TCF_FUN_HEAVYWEIGHT)
>              fun->flags |= JSFUN_HEAVYWEIGHT;
> +
> +        if (!fun->getParent())

My recollection is that Igor did this to fix leaks via CG-created functions. Have you tested Firefox (without Firebug, which has known leaks) to see if Firefox leaks with this patch?

How expensive would it be to clone CG-created functions before handing them out to the debugger? The reason this hasn't bitten us in the past has been because nobody can usually get their hands on them...

Comment 18

[Steve Fink [:sfink] [:s:]]

(In reply to comment #17)
> Comment on attachment 528778 [details] [diff] [review] [review]
> Set the parent of CG-created JSFunctions
> 
> My recollection is that Igor did this to fix leaks via CG-created functions.
> Have you tested Firefox (without Firebug, which has known leaks) to see if
> Firefox leaks with this patch?

Ah, that makes sense. No, I haven't tested for leaks. The patch was incomplete anyway; even after handling those functions and one or two other things, I was still seeing the problem. So it seems that not parenting internal objects must be a reasonably common pattern.

> How expensive would it be to clone CG-created functions before handing them
> out to the debugger? The reason this hasn't bitten us in the past has been
> because nobody can usually get their hands on them...

That's a good idea. Right now, I'm thinking I'll either do that or just prevent JSD from looking at these things. I attempted to do the latter but it somehow still failed, oddly enough. I wonder if we're *removing* parents somewhere?

Or maybe both, depending on the type of object, because these function seem like they might be potentially important to be able to debug.

Comment 19

[Steve Fink [:sfink] [:s:]]

JSD is gone.