342625 - build does not start with WAY_TOO_MUCH_GC because js_NewContext fails

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Builds do not start with WAY_TOO_MUCH_GC because js_NewContext fails.  The reasons for this are described in bug 342180 comment 5, bug 342180 comment 4, and bug 342180 comment 0.  (It's in reverse order due to working backwards to figure out the real cause of the problem.)  But that bug got taken over by making us not crash when js_NewContext fails and ignored fixing the problem that it was failing, so I'm filing this bug on the real problem.

This blocks testing both the DOM Agnostic changes and new JS 1.7 features with WAY_TOO_MUCH_GC (although maybe I could with different combinations of branches).

I think this is a regression frum bug 341896, but I haven't actually tested that fact.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix for jsgc.c

This patch fixes WAY_TOO_MUCH_GC by not calling into the GC while we're still revving up the runtime. It also makes sure that we report out of memory in JS_GC when the last ditch GC fails from js_NewGCThing.

Comment 2

[Igor Bukanov]

Comment on attachment 227479 [details] [diff] [review]
Fix for jsgc.c

>Index: jsgc.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsgc.c,v
>retrieving revision 3.156
>diff -p -U8 -r3.156 jsgc.c
>--- jsgc.c	21 Jun 2006 09:13:24 -0000	3.156
>+++ jsgc.c	28 Jun 2006 23:18:30 -0000
>             if (!js_GC(cx, GC_KEEP_ATOMS | GC_LAST_DITCH)) {
>                 /* js_GC ensures that GC is unlocked after GC was canceled. */
>+                JS_ReportOutOfMemory(cx);
>                 return NULL;
>             }
>             METER(rt->gcStats.retry++);
>         }

We should not report error here when the branch callback cancels GC. In fact I realized that my changes to call the branch callback outside GC lock and earlier ones to check for malloc allocation barrier introduced a regression: when the JSGC_BEGIN callback does not want to run GC, NewGCThing should try to allocate things ignoring the barrier limit. I will fix this.

Comment 3

[Igor Bukanov]

Attachment: Fix v2

This should restore the fallout from the fixes for bug 338653 and bug 341896 - I should see this before :(

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 227510 [details] [diff] [review]
Fix v2

>             if (!js_GC(cx, GC_KEEP_ATOMS | GC_LAST_DITCH)) {
>-                /* js_GC ensures that GC is unlocked after GC was canceled. */
>+                /*
>+                 * js_GC ensures that GC is unlocked when the branch callback
>+                 * wants to stop execution and in this case we do not report
>+                 * out-of-memory.
>+                 */

Comma after "execution" before " and in this case...".

>+ * Return false when the branch callback wants to stop exeution ASAP or true
>+ * otherwise.

s/or/and/

>+++ js/src/jsgc.h	2006-06-29 08:16:02.000000000 +0200
>@@ -220,18 +220,17 @@ js_MarkStackFrame(JSContext *cx, JSStack
> #define GC_KEEP_ATOMS       0x1
> #define GC_LAST_CONTEXT     0x2
> #define GC_LAST_DITCH       0x4
> 
> extern void
> js_ForceGC(JSContext *cx, uintN gcflags);
> 
> /*
>- * Return false when GC was canceled or true when the full GC cycle was
>- * performed which may or may not free things.
>+ * Return false when the branch callback cancells GC or true otherwise.

s/cancells/cancels/

Sorry, I should have seen this -- js_GC returns true unless the null script branch callback cancels -- too, especially yesterday when mrbkap was asking about this bug.

/be

Comment 5

[Igor Bukanov]

Attachment: Fix v2b

Fixing misspellings.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 227556 [details] [diff] [review]
Fix v2b


>         if (checkBranchCallback &&
>             cx->branchCallback && /* gcCallback can reset the branchCallback */
>             !cx->branchCallback(cx, NULL)) {
>             METER(rt->gcStats.retryhalt++);
>+            /* Keep GC unlocked when canceled. */
>             return JS_FALSE;

Bonus for blank line before /* .... */ comment, just to make it and the return it applies to stand out.

/be

Comment 7

[Igor Bukanov]

Attachment: Patch to commit

This is the patch I will commit that includes the blank line before comments for clarity.

Comment 8

[Igor Bukanov]

I committed the patch from comment 7 to the trunk.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Verified.