440558 - JS_Assert("*flagp != GCF_FINAL") when running with 'javascript.options.gczeal' = 2

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Stack trace

See attached stack trace.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Testcase

This is the page I have set to my homepage. When I have gczeal set to 2 I crash before the page loads.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

My debugging here was hampered by a lack of addr2line, but it appears that this is related to bug 438633. What I see happening is us creating a new function object for the function expression in the page's script. We then (from JS_EvaluateUCScriptForPrincipals) execute the script. In js_Execute, before we finish setting up the frame, we call OBJ_TO_OUTER_OBJECT, which causes us to create a new XOW, causing a GC (thanks to gczeal here).

We could either fix this by fixing bug 438633, by moving the call to outerObject to after we've set cx->fp (which seems more correct based on which scope chain we want for the new XOW) or by using a tvr in JS_EvaluateUCScriptForPrincipals. I've taken the second option.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Potential fix

Comment 4

[Jason Orendorff [:jorendorff]]

The fix in bug 438633 is only for the JS_Compile*{Script,File}* functions.  It intentionally doesn't touch JS_Evaluate*Script*, because the rooting scheme I'm using requires JS_LOCK_GC.  The patch seems like the right answer here regardless of what happens in 438633.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 326475 [details] [diff] [review]
Potential fix

>Don't cause a GC before the script is on the JS call stack. bug 440558
>
>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp
>+++ b/js/src/jsinterp.cpp
>@@ -1482,12 +1482,6 @@ js_Execute(JSContext *cx, JSObject *chai
>         frame.callee = NULL;
>         frame.fun = NULL;
>         frame.thisp = chain;
>-        OBJ_TO_OUTER_OBJECT(cx, frame.thisp);
>-        if (!frame.thisp) {
>-            ok = JS_FALSE;
>-            goto out;

Note here it's correct to goto out on early error.

>@@ -1537,6 +1531,14 @@ js_Execute(JSContext *cx, JSObject *chai
>     }
> 
>     cx->fp = &frame;
>+    if (!down) {
>+        OBJ_TO_OUTER_OBJECT(cx, frame.thisp);
>+        if (!frame.thisp) {
>+            ok = JS_FALSE;
>+            goto out;

Not so here -- need to unhook cx->fp and restore the set-aside frame chain.

/be

>+        }
>+        frame.flags |= JSFRAME_COMPUTED_THIS;
>+    }
>     if (hook) {
>         hookData = hook(cx, &frame, JS_TRUE, 0,
>                         cx->debugHooks->executeHookData);

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 327089 [details] [diff] [review]
patch v2

Thanks.

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Pushed as changeset 6d70e01afad2.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 327089 [details] [diff] [review]
patch v2

This fixes a GC hazard.

Comment 10

[Samuel Sidler (old account; do not CC)]

Let's get a test for this before landing it on 1.9.0.x.

Comment 11

[Bob Clary [:bc] (inactive)]

Note that this test will only show the assertion in the browser and only if gczeal is set to 2 before the browser starts. 

/cvsroot/mozilla/js/tests/js1_5/GC/regress-440558.js,v  <--  regress-440558.js
initial revision: 1.1

mozilla-central: changeset:   16476:b7bcbaf5bef2

Comment 12

[Samuel Sidler (old account; do not CC)]

Comment on attachment 327089 [details] [diff] [review]
patch v2

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.9 branch.

Comment 14

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0, 1.9.1 mac browser