617485 - "Assertion failure: global->getClass()->flags & JSCLASS_IS_GLOBAL"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: stack trace

Iterator(evalcx('#2=*'))

Assertion failure: global->getClass()->flags & JSCLASS_IS_GLOBAL, at /Users/jruderman/tracemonkey/js/src/jsobjinlines.h:868

The first bad revision is:
changeset:   4c1fbfcf1d0d
user:        Jason Orendorff
date:        Wed Jun 16 16:13:28 2010 -0500
summary:     Bug 570169 - Part 2, add assertions that gcthings do not leak across compartments. r=gal.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Seems to have morphed to:

Assertion failure: global->isGlobal(),

Tested on TM changeset 284811f39ca6 on 32-bit js shell on Linux.

Comment 2

[Andreas Gal :gal]

Attachment: patch

Comment 3

[Andreas Gal :gal]

This is actually not shell-only, but the given test case is shell-only. I don't think this is a security bug. And also the bug fingered above is a red herring.

Comment 4

[Andreas Gal :gal]

Comment on attachment 505000 [details] [diff] [review]
patch

Sorry Waldo, but you know xml and Brendan is pretty busy. Note that the any object is now per compartment so this is not a security issue (it would have been before we did that with __parent__ still around).

Comment 5

[Andreas Gal :gal]

No beta needed, but we will fix asap.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 505000 [details] [diff] [review]
patch

>+    /* Scope chain accessors. */
>+

No newline here. Use major style if you need it, but see below in context and further in the file -- no need.

>+    JSObject &scopeChain() {
>+        return fp()->scopeChain();
>+    }
>+
>+    JSObject &getGlobal() {
>+        return hasfp() ? *fp()->scopeChain().getGlobal() : *globalObject;
>+    }

This is a nice cleanup IMHO but not needed here and it won't get used where it should without a separate bug. So factor it out and file it fresh, with a grep of all the use case opportunities.

>+        obj = NewNonFunction<WithProto::Given>(cx, &js_AnyNameClass, NULL, &cx->getGlobal());
...
>         cx->compartment->anynameObject = obj;

This is a bad cross-scope leak (with (*) x = y; where x is bound in the anyname's global but the code runs in a different global scope sharing the same compartment). You need anyname per global, not per compartment. Use jsproto.tbl.

/be

Comment 8

[Andreas Gal :gal]

Attachment: patch

Using a reserved slot seems strictly simpler here.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 505120 [details] [diff] [review]
patch

>diff --git a/js/src/jsapi.h b/js/src/jsapi.h
>--- a/js/src/jsapi.h
>+++ b/js/src/jsapi.h
>@@ -1934,20 +1934,22 @@ struct JSClass {
> #define JSCLASS_MARK_IS_TRACE           (1<<(JSCLASS_HIGH_FLAGS_SHIFT+3))
> #define JSCLASS_INTERNAL_FLAG2          (1<<(JSCLASS_HIGH_FLAGS_SHIFT+4))
> 
> /* Indicate whether the proto or ctor should be frozen. */
> #define JSCLASS_FREEZE_PROTO            (1<<(JSCLASS_HIGH_FLAGS_SHIFT+5))
> #define JSCLASS_FREEZE_CTOR             (1<<(JSCLASS_HIGH_FLAGS_SHIFT+6))
> 
> /* Additional global reserved slots, beyond those for standard prototypes. */
>-#define JSRESERVED_GLOBAL_SLOTS_COUNT     3
>+#define JSRESERVED_GLOBAL_SLOTS_COUNT     5
> #define JSRESERVED_GLOBAL_THIS            (JSProto_LIMIT * 3)

Urgh, you didn't fix bug 594221 yet? It is pure but minor bloat, and the redundant proto slots are bug-bait.

> #define JSRESERVED_GLOBAL_THROWTYPEERROR  (JSRESERVED_GLOBAL_THIS + 1)
> #define JSRESERVED_GLOBAL_REGEXP_STATICS  (JSRESERVED_GLOBAL_THROWTYPEERROR + 1)
>+#define JSRESERVED_GLOBAL_ANYNAME         (JSRESERVED_GLOBAL_REGEXP_STATICS + 1)
>+#define JSRESERVED_GLOBAL_FUNCTION_NS     (JSRESERVED_GLOBAL_ANYNAME + 1)

On IRC I suggested using jsproto.tbl, giving js_AnyNameClass a JSCLASS_HAS_CACHED_PROTO(JSProto_AnyName). See jsmath.cpp for how this works with a singleton class object that's not a constructor with a class prototype.

>+    JSObject *global = cx->hasfp() ? cx->fp()->scopeChain().getGlobal() : cx->globalObject;

Bug asking a JSContext inline helper still wanted?

>         /*
>          * Avoid entraining any in-scope Object.prototype.  The loss of
>          * Namespace.prototype is not detectable, as there is no way to
>          * refer to this instance in scripts.  When used to qualify method
>          * names, its prefix and uri references are copied to the QName.
>          */
>         obj->clearProto();
>-        obj->clearParent();

Might want to comment about leaving parent linked to the global that links back to this function:: nsobj.

/be

Comment 10

[Andreas Gal :gal]

Attachment: patch

Comment 11

[Andreas Gal :gal]

> Urgh, you didn't fix bug 594221 yet? It is pure but minor bloat, and the
> redundant proto slots are bug-bait.

If the bug is valid, we will fix that issue there.

> On IRC I suggested using jsproto.tbl, giving js_AnyNameClass a
> JSCLASS_HAS_CACHED_PROTO(JSProto_AnyName). See jsmath.cpp for how this works
> with a singleton class object that's not a constructor with a class prototype.

I looked at it and this makes the patch strictly longer and more complicated. AnyName is not a good match for jsproto.tbl because AnyName is not the correct name for this, its *. prototbl works well for standard classes with a standard constructor. Anyname is so special there is no advantage in sticking it in there. We already have functionNS, we can as well have anyname right next to it.

> 
> >+    JSObject *global = cx->hasfp() ? cx->fp()->scopeChain().getGlobal() : cx->globalObject;
> 
> Bug asking a JSContext inline helper still wanted?

Filed as bug 627545.

> 
> >         /*
> >          * Avoid entraining any in-scope Object.prototype.  The loss of
> >          * Namespace.prototype is not detectable, as there is no way to
> >          * refer to this instance in scripts.  When used to qualify method
> >          * names, its prefix and uri references are copied to the QName.
> >          */
> >         obj->clearProto();
> >-        obj->clearParent();
> 
> Might want to comment about leaving parent linked to the global that links back
> to this function:: nsobj.

Done.

> 
> /be

Comment 12

[Andreas Gal :gal]

Attachment: patch

Comment 13

[Alex Miller]

Fwiw, I get a null pointer dereference crash on an opt build, nothing weird.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 505622 [details] [diff] [review]
patch

>diff --git a/js/src/jsapi.h b/js/src/jsapi.h
>--- a/js/src/jsapi.h
>+++ b/js/src/jsapi.h
>@@ -1934,20 +1934,21 @@ struct JSClass {
> #define JSCLASS_MARK_IS_TRACE           (1<<(JSCLASS_HIGH_FLAGS_SHIFT+3))
> #define JSCLASS_INTERNAL_FLAG2          (1<<(JSCLASS_HIGH_FLAGS_SHIFT+4))
> 
> /* Indicate whether the proto or ctor should be frozen. */
> #define JSCLASS_FREEZE_PROTO            (1<<(JSCLASS_HIGH_FLAGS_SHIFT+5))
> #define JSCLASS_FREEZE_CTOR             (1<<(JSCLASS_HIGH_FLAGS_SHIFT+6))
> 
> /* Additional global reserved slots, beyond those for standard prototypes. */
>-#define JSRESERVED_GLOBAL_SLOTS_COUNT     3
>+#define JSRESERVED_GLOBAL_SLOTS_COUNT     4

3 was wrong before this patch, one too high, AFAICT. Leave at 3.

> #define JSRESERVED_GLOBAL_THIS            (JSProto_LIMIT * 3)
> #define JSRESERVED_GLOBAL_THROWTYPEERROR  (JSRESERVED_GLOBAL_THIS + 1)
> #define JSRESERVED_GLOBAL_REGEXP_STATICS  (JSRESERVED_GLOBAL_THROWTYPEERROR + 1)
>+#define JSRESERVED_GLOBAL_FUNCTION_NS     (JSRESERVED_GLOBAL_REGEXP_STATICS + 1)

Looks great otherwise, thanks.

/be

Comment 15

[Andreas Gal :gal]

I count 4 slots (this, throwtypeerror, regexpstatics, functionns).

/* Additional global reserved slots, beyond those for standard prototypes. */
#define JSRESERVED_GLOBAL_SLOTS_COUNT     4
#define JSRESERVED_GLOBAL_THIS            (JSProto_LIMIT * 3)
#define JSRESERVED_GLOBAL_THROWTYPEERROR  (JSRESERVED_GLOBAL_THIS + 1)
#define JSRESERVED_GLOBAL_REGEXP_STATICS  (JSRESERVED_GLOBAL_THROWTYPEERROR + 1)
#define JSRESERVED_GLOBAL_FUNCTION_NS     (JSRESERVED_GLOBAL_REGEXP_STATICS + 1)

I will commit with 4 but we can do a follow-up patch if I am wrong.

Comment 16

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/74190651d91a

Comment 17

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/74190651d91a

Comment 18

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397