Closed Bug 617485 Opened 11 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: jruderman, Assigned: gal)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [softblocker], fixed-in-tracemonkey)

Attachments

(2 files, 3 obsolete files)

Attached file 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.
Seems to have morphed to:

Assertion failure: global->isGlobal(),

Tested on TM changeset 284811f39ca6 on 32-bit js shell on Linux.
Blocks: 570169
blocking2.0: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
Attached patch patch (obsolete) — Splinter Review
Assignee: general → 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.
blocking2.0: ? → final+
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).
Attachment #505000 - Flags: review?(jwalden+bmo)
Whiteboard: [softblocker]
No beta needed, but we will fix asap.
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
Attachment #505000 - Flags: review?(jwalden+bmo) → review-
Attached patch patch (obsolete) — Splinter Review
Using a reserved slot seems strictly simpler here.
Attachment #505000 - Attachment is obsolete: true
Attachment #505120 - Flags: review?(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
Attached patch patch (obsolete) — Splinter Review
Attachment #505120 - Attachment is obsolete: true
Attachment #505120 - Flags: review?(brendan)
> 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
Attachment #505569 - Flags: review?(brendan)
Attached patch patchSplinter Review
Attachment #505569 - Attachment is obsolete: true
Attachment #505569 - Flags: review?(brendan)
Attachment #505622 - Flags: review?(brendan)
Fwiw, I get a null pointer dereference crash on an opt build, nothing weird.
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
Attachment #505622 - Flags: review?(brendan) → review+
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.
http://hg.mozilla.org/tracemonkey/rev/74190651d91a
Whiteboard: [softblocker] → [softblocker], fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/74190651d91a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/2e891e0db397
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.