341821 - Case for running close hooks outside GC

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Consider the core of the test case from bug 341815:

function make_iterator()
{
	var iter = (function() { yield 0; })();
	iter.close = make_iterator;
	alert(1);
}

make_iterator();

for (var i = 0; i != 50000; ++i) {
    var x = {};
}

The loop at the end triggers GC which in turn leads to calling alert from the close hook. As a part of alert implementation XPConnect calls   JS_SuspendRequest which would allow other threads to execute GC leading to either a deadlock and/or other bad things. 

Since calling JS_SuspendRequest is a legitimate thing to do, this has to be fixed. I think the best way to fix is to move execution of close handlers outside of GC critical section.

Comment 1

[Igor Bukanov]

*** Bug 341815 has been marked as a duplicate of this bug. ***

Comment 2

[Igor Bukanov]

Attachment: Work in progress

This is an untested work-in-progress sort of patch. 

The main idea is to execute the close hooks outside GC critical section without imposing any restrictions on the code. In particular, running GC inside close hooks is allowed and cancellation of long-running scripts is supported.

The patch switched the close handler back to a linked list to simplify transfer of objects with close hooks between different lists.

Comment 3

[Igor Bukanov]

Attachment: Implementation v2

Few notable fixes/changes from the previous patch:

1. The close hooks is always executed outside GC lock and a potential GC call inside a close hook would preserve atoms when the initial GC would have GC_KEEP_ATOMS flag.

2. On shutdown all close hooks registered before the shutdown are executed.

Comment 4

[Bob Clary [:bc] (inactive)]

patch v2 failed to apply to today's trunk. Igor, can I get a new version updated for the trunk ?

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> patch v2 failed to apply to today's trunk. Igor, can I get a new version
> updated for the trunk ?
> 

I could not update it until the end of vacation ;)

Comment 6

[Brendan Eich [:brendan]]

I'll update the patch.

Comment 7

[Igor Bukanov]

Comment on attachment 227398 [details] [diff] [review]
Implementation v2

>--- .pc/close_outside_gc.diff/js/src/jsgc.c	2006-06-28 11:47:53.000000000 +0200
>+/*
>+ * First half of the close phase: loop over the objects that we set aside
>+ * in the close table and mark them to protect them against finalization
>+ * during sweeping phase.
>+ */
>+static void
>+FindAndMarkObjectsToClose(JSContext *cx)
> {
>     JSRuntime *rt;
>+    JSGCCloseListItem *todo, **itemp, *item;
>+    if (cx->gcExecutingCloseHooks) {

This condition has to be extended to check for the last ditch GC to fix another problem introduced by generators. Without replacing this by "cx->gcExecutingCloseHooks || lastDitchGC" any code that assumes that allocation of a GC thing of one kind does not affect cx->newbord[anotherKind] remians a GC hazard. Example: string XML constant folding code in jsparse.c

Comment 8

[Igor Bukanov]

Attachment: Implementation v3

Patch update to sync with the trunk and reflect comment 7.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 230709 [details] [diff] [review]
Implementation v3

Hey, I'm in Oslo with Graydon and the rest of ECMA TG1, jetlagged.  Thanks for patching this, sorry I didn't get to it.  Nits and a question or two:

>     /*
>-     * NB: do not pack another flag here by claiming gcPadding unless the new
>-     * flag is written only by the GC thread.  Atomic updates to packed bytes
>-     * are not guaranteed, so stores issued by one thread may be lost due to
>-     * unsynchronized read-modify-write cycles on other threads.
>+     * NB: do not pack another flag here unless the new flag is written only
>+     * by the GC thread. Atomic updates to packed bytes are not guaranteed, so
>+     * stores issued by one thread may be lost due to unsynchronized
>+     * read-modify-write cycles on other threads.
>      */
>-    JSPackedBool        gcPoke;
>     JSPackedBool        gcRunning;
>-    JSPackedBool        gcClosePhase;
>-    uint8               gcPadding;
>+    JSBool              gcPoke;

Isn't gcPoke only set when the GC is not running?  And gcRunning set only after the GC has waited for all requests in progress to end or suspend?  I think gcPoke can be a JSPackedBool packed with gcRunning as before. What am I missing?

>+    /*
>+     * Singly linked list of items tracking objects of extended classes that
>+     * have non-null close hooks.
>+     */
>+    JSGCCloseListItem   *gcCloseList;

D'oh, too bad we can't use a JSPtrTable for lower space overhead.

>+    /*
>+     * List of objects with close hooks that the the current thread trying to
>+     * close.

"the the" typo and missing "is" before "trying".

"current context" would be more precise in light of JSThread.

>+"JS engine warning: Some close hooks were not executed before destroying the\n"
>+"JSRuntime. This may result in a memory leak.\n");

How about "JSRuntime at %p" and pass rt, in case of multi-runtime embeddings?

>+static JSGCCloseListItem **
>+GetCloseListTail(JSGCCloseListItem **itemp)
>+{
>+    while (*itemp)
>+        itemp = &(*itemp)->next;
>+    return itemp;
>+}

Maybe meter to see if this list can get too long?  Or is it bounded in length?

>+static JSBool
>+ExecuteCloseHooks(JSContext *cx)

Do we need an API for this, or for dropping items from cx->gcObjectsToClose, for properly sandboxed page transitions?

>+        JS_ASSERT(xclasp->close);
>+        ok = xclasp->close(cx, obj);

Blank line here, before comment, would be nice.

>+        /* One more object with a close hook becomes unreachable. */
>+        rt->gcPoke = JS_TRUE;
>+        if (cx->throwing) {
>+            if (!js_ReportUncaughtException(cx))
>+                JS_ClearPendingException(cx);
>+            /*
>+             * XXX: This assumes that if the execution is cancelled through the

"canceled" misspelled.

>+             * branch callback, then cx->throwing should be false. What
>+             * about branch callback that cancel after cx->throwing is set?

API usage bug.  A branch callback should not throw on its cx.

>+    /*
>+     * Run close hooks if any unless GC is called from inside a close hook.
>+     */
>+    shouldRunCloseHooks = (cx->gcObjectsToClose && !cx->gcExecutingCloseHooks);

The variable name suggests slightly shorter "gcRunningCloseHooks" cx member name.

>+++ js/src/jspubtd.h	2006-07-26 08:39:43.000000000 +0200
>@@ -259,16 +259,19 @@ typedef JSBool
>  * store a reference to obj.
>  *
>  * This is also the type of the JSExtendedClass.close hook, which is stubbed
>  * with NULL if not needed.
>  */
> typedef void
> (* JS_DLL_CALLBACK JSFinalizeOp)(JSContext *cx, JSObject *obj);
> 
>+typedef JSBool
>+(* JS_DLL_CALLBACK JSCloseOp)(JSContext *cx, JSObject *obj);

The comment before JSFinalizeOp's typedef ends with the paragraph that should be revised, separated and moved down to the JSCloseOp typedef.

/be

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> (From update of attachment 230709 [details] [diff] [review] [edit])
> Hey, I'm in Oslo with Graydon and the rest of ECMA TG1, jetlagged.

I have been in Oslo for few hours yesturday waiting for delayed plain to Bergen ;).

> Isn't gcPoke only set when the GC is not running?  And gcRunning set only after
> the GC has waited for all requests in progress to end or suspend?  I think
> gcPoke can be a JSPackedBool packed with gcRunning as before. What am I
> missing?

gcPoke is set outside any locks in, for example, the interpreter loop. What happens when multiple threads set gcPoke at the same time?

> Maybe meter to see if this list can get too long?  Or is it bounded in length?

Now, the lists are not bounded.

> 
> >+static JSBool
> >+ExecuteCloseHooks(JSContext *cx)
> 
> Do we need an API for this, or for dropping items from cx->gcObjectsToClose,
> for properly sandboxed page transitions?

I do not think a simple API is enough. In fact I would suggest to drop close hooks from JSClass altogether and just have them for generators until all issues regarding API to control them based on their domain are ironed out.

> >+            /*
> >+             * XXX: This assumes that if the execution is canceled through > >+             * branch callback, then cx->throwing should be false. What
> >+             * about branch callback that cancel after cx->throwing is set?
> 
> API usage bug.  A branch callback should not throw on its cx.

I mean a branch callback that is called from, for example, a finally block that is excuted in a response to exception. Would cx->throwing be true in this case?

Comment 11

[Igor Bukanov]

(In reply to comment #10)
> (In reply to comment #9)
> > 
> > >+static JSBool
> > >+ExecuteCloseHooks(JSContext *cx)
> > 
> > Do we need an API for this, or for dropping items from cx->gcObjectsToClose,
> > for properly sandboxed page transitions?
> 
> I do not think a simple API is enough. In fact I would suggest to drop close
> hooks from JSClass altogether and just have them for generators until all
> issues regarding API to control them based on their domain are ironed out.
> 

Note that this patch simply tries just to fix bad bugs with generators, it is not about API to control them.

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #10)
> I have been in Oslo for few hours yesturday waiting for delayed plain to Bergen
> ;).

Sorry we missed you -- we were around yesterday, more jetlagged. Better today, but it's late in local time so I'll be brief.

> > Isn't gcPoke only set when the GC is not running?  And gcRunning set only after
> > the GC has waited for all requests in progress to end or suspend?  I think
> > gcPoke can be a JSPackedBool packed with gcRunning as before. What am I
> > missing?
> 
> gcPoke is set outside any locks in, for example, the interpreter loop. What
> happens when multiple threads set gcPoke at the same time?

They all store 1 in the byte, and all read 0 in the other byte, so races should be benign.  I know of no actual bug.  I'd pad the remaining space with uint16 at least, uint8[2] maybe.
 
> I do not think a simple API is enough. In fact I would suggest to drop close
> hooks from JSClass altogether and just have them for generators until all
> issues regarding API to control them based on their domain are ironed out.

Could do that, but generators alone raise the issues, don't they?  That is, so long as generators are user-defined by scripted functions containing yield operator usage, their close hooks can be run or cleared without leaks.

> I mean a branch callback that is called from, for example, a finally block that
> is excuted in a response to exception. Would cx->throwing be true in this case?

A try-finally with no catch, you're right. That case is broken right now in some ways -- if the finally calls eval, the js_Interpret nested from obj_eval will find cx->throwing and fail immediately. I've filed bug 346029 on this and will fix it shortly.

/be

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (In reply to comment #10)
> > I do not think a simple API is enough. In fact I would suggest to drop close
> > hooks from JSClass altogether and just have them for generators until all
> > issues regarding API to control them based on their domain are ironed out.
> 
> Could do that, but generators alone raise the issues, don't they?  That is, so
> long as generators are user-defined by scripted functions containing yield
> operator usage, their close hooks can be run or cleared without leaks.

That was confusing.  I meant to say we have no current problem with only scripted close hooks.  But you may be saying the same thing, and pointing to the potential for native close hooks to lead to leaks if not run after a page unloads. That's fair enough, but I suggest that it's a hazard we could document.  If you want to remove JSExtendedClass.close, that would eliminate the hazard and we could get on with our lives with one fewer hazard ;-). What do you think?

/be

Comment 14

[Igor Bukanov]

Attachment: Implementation v4

Besides addressing the nits and metering close hook lists, the patch changes gcflags argument for js_GC to gckind which is a enumeration. Since the patch does not execute close hooks for the last ditch GC, it does not need to worry about keeping atoms when a close hook trigger GC. 

But that required to much comments and I figured out that changing gcflags makes that self-commenting while giving an opportunity to optimize marking of atoms for the last ditch GC.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 230865 [details] [diff] [review]
Implementation v4

>+    /*
>+     * Keep atoms if a suspended compile is running on another context or we
>+     * are doing the last ditch GC.
>+     */
>+    keepAtoms = (JSBool)rt->gcKeepAtoms || gckind == GC_LAST_DITCH;

(JSBool) is just (int), so not a useful cast here.  Could use != 0 if you want to show gcKeepAtoms' type to readers, but I'd just say rt->gcKeepAtoms || ....

>+    /*
>+     * Run close hooks unless GC is called from inside a close hook or
>+     * when GC is the last ditch (see comments in FindAndMarkObjectsToClose).
>+     */
>+    if (!cx->gcRunningCloseHooks && gckind != GC_LAST_DITCH) {
>+        if (cx->gcObjectsToClose && !RunCloseHooks(cx)) {
>+            METER(rt->gcStats.retryhalt++);
>+            return JS_FALSE;
>+        }
>+    }

if (A && B) { if (C && D) { ... } } could be if (A && B && C && D) { ... } -- won't fit on one line, so prefer one && operand per line if you can stand it ;-).

>+/* Structure to track an object of extended class with a non-null close hook. */
>+
>+typedef struct JSGCCloseListItem JSGCCloseListItem;
>+
>+struct JSGCCloseListItem {
>+    JSGCCloseListItem *next;
>+    JSObject          *object;
>+};
>+
> extern JSBool
>-js_AddObjectToCloseTable(JSContext *cx, JSObject *obj);
>+js_RegisterObjectWithCloseHook(JSContext *cx, JSObject *obj);
> 
> /*
>  * The private JSGCThing struct, which describes a gcFreeList element.
>  */
> struct JSGCThing {

Nit: the comment in front of typedef struct JSGCCloseListItem line really wants to be a major-style comment, like the one before struct JSGCThing.

>-/*
>- * Flags to modify how a GC marks and sweeps:
>- *   GC_KEEP_ATOMS      Don't sweep unmarked atoms, they may be in use by the
>- *                      compiler, or by an API function that calls js_Atomize,
>- *                      when the GC is called from js_NewGCThing, due to a
>- *                      malloc failure or the runtime GC-thing limit.
>- *   GC_LAST_CONTEXT    Called from js_DestroyContext for last JSContext in a
>- *                      JSRuntime, when it is imperative that rt->gcPoke gets
>- *                      cleared early in js_GC, if it is set.
>- *   GC_LAST_DITCH      Called from js_NewGCThing as a last-ditch GC attempt.
>- *                      See comments before js_GC definition for details.
>- */
>-#define GC_KEEP_ATOMS       0x1
>-#define GC_LAST_CONTEXT     0x2
>-#define GC_LAST_DITCH       0x4
>+
>+/* Kinds of js_GC invocation. */
>+
>+typedef enum JSGCInvocationKind {
>+
>+    /* Normal invocation. */
>+    GC_NORMAL,
>+
>+    /*
>+     * Called from js_DestroyContext for last JSContext in a JSRuntime, when
>+     * it is imperative that rt->gcPoke gets cleared early in js_GC, if it is
>+     * set.

", if it is set" seems unnecessary to me, since if rt->gcPoke is not set, js_GC returns early.

>+     */
>+    GC_LAST_CONTEXT,
>+
>+    /*
>+     * Called from js_NewGCThing as a last-ditch GC attempt. See comments
>+     * before js_GC definition for details.
>+     */
>+    GC_LAST_DITCH
>+
>+} JSGCInvocationKind;
> /*
>+ * Close hook for obj.

"where obj has extended class" or words to that effect.

> When non-null, it is executed after the garbage
>+ * collector has determined the object to be unreachable from other live
>+ * objects or from GC roots. The hook can execute arbitrary code. GC runs the
>+ * hook only once even if it makes the object reachable again.
>+ */

Looks great, thanks again.  r=me with nits picked.

/be

Comment 16

[Igor Bukanov]

Attachment: Implementation v5

Addressing the nits. Not to have if(A && B && C && !D) spread over 4 lines I moved  A && B checks into RunCloseHooks. It looks better.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 230881 [details] [diff] [review]
Implementation v5

>+     * assumption that such GC should keep all atoms. A recursive invocation of

+01234567890123456789012345678901234567890123456789012345678901234567890123456789

Uber-nit: vim tw=78 violation, wrap with Qj when sitting on this line.

r=me again.

/be

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 230881 [details] [diff] [review]
Implementation v5

More comment nits coming, but this looks great.


>+     * We delay the execution of close hooks for unreachable objects from
>+     * rt->gcCloseList when the current thread has not yet finish execution of

"finished"

>+     * GC thing of one kind should not affect affect cx->newbord[anotherKind].

"cx->newborn"

>+     * A close hook can violate it since it can allocate arbitrary things.
>+     *
>+     * Not running close hooks when GC is the last ditch also preserve the

"preserves"

>+    /*
>+     * Comments in FindAndMarkObjectsToClose explain why we checks for the last

"check"

r=mrbkap with those nits picked.

Comment 19

[Igor Bukanov]

Attachment: Implementation v5b

Patch to commit with nits addressed.

Comment 20

[Igor Bukanov]

I committed the patch from comment 19 t the trunk.

Comment 21

[Igor Bukanov]

(In reply to comment #13)
> If you want to remove JSExtendedClass.close, that would eliminate
> the hazard and we could get on with our lives with one fewer hazard ;-). What
> do you think?

This would be the best option IMO. With only scriptable hooks skipping them is similar to skipping JS finally blocks when a branch callback want to stop script.

Comment 22

[Igor Bukanov]

I backed out the patch: it was broken as its optimization to avoid enumeration of atoms and scriptnames when keepAStom is set was premature. There I did not took into account that for optimization to work js_MarkScriptFilename and js_markAtom has to be patched as well not to set mark bit when keepAtoms is true. 

So for now I will make a patch without optimization and leave that for later.

Comment 23

[Igor Bukanov]

Attachment: Implementation v6

The patch is effectively the previous version without brokem optimization.

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 231093 [details] [diff] [review]
Implementation v6

Sorry, I should have seen that.

/be

Comment 25

[Igor Bukanov]

Attachment: Implementation v6b

Patch to commit

The only difference with the previous version is extra parenthesis around long boolean expressions:

-    keepAtoms = rt->gcKeepAtoms != 0 || gckind == GC_LAST_DITCH;
+    keepAtoms = (rt->gcKeepAtoms != 0 || gckind == GC_LAST_DITCH);

...

-    checkBranchCallback = gckind == GC_LAST_DITCH &&
-                          JS_HAS_NATIVE_BRANCH_CALLBACK_OPTION(cx) &&
-                          cx->branchCallback;
-
+    checkBranchCallback = (gckind == GC_LAST_DITCH &&
+                           JS_HAS_NATIVE_BRANCH_CALLBACK_OPTION(cx) &&
+                           cx->branchCallback);
+

Comment 26

[Igor Bukanov]

I committed the patch from comment 25 t the trunk.

Comment 27

[Mike Schroepfer]

Igor/Brendan - can you comment on safety/necessity for branch?

Comment 28

[Brendan Eich [:brendan]]

See the dependency -- this is a crash bug that we must fix for 1.8.1.

/be

Comment 29

[Mike Schroepfer]

Comment on attachment 231140 [details] [diff] [review]
Implementation v6b

approved by schrep for drivers

Comment 30

[Igor Bukanov]

I took the patch out again: the combined patch to address this bug and the bug 341815 would be smaller then 2 separated patches.

Comment 31

[Igor Bukanov]

Attachment: Implementation v7

Patch that moves execution of close hooks outside GC locks and skips close hooks execution when GC is invoked from DestroyContext.

Since DestroyContext can be a single source of js_GC calls when one open and close windows frequently, the patch executes close hooks  if any also from MaybeGC when GC is not run. Otherwise they would not get a chance to run.

Comment 32

[Igor Bukanov]

Attachment: Implementation v8

Comment 33

[Igor Bukanov]

This patch from comment 32 assumes that the patch from bug is applied.

Notable changes compared with the previous version:

1. Since only generators can have close hooks, the patch extends JSGenerator with fields for the global list avoiding allocation of list items.

2. js_ForceGC is merged with js_GC since with explicit JSGCInvocationKind argument checks for newborn cleanup are very explicit.

Comment 34

[Igor Bukanov]

Attachment: Implementation v9

Patch update to reflect changes in the committed form of the patch from bug  346450.

Comment 35

[Brendan Eich [:brendan]]

Comment on attachment 231656 [details] [diff] [review]
Implementation v9

>+    /*
>+     * Run close hooks for objects that becomes unreachable after the last
>+     * GC.

This sentence fits on one line in the given textwidth (vim tw=78) if you leave the comment style "major" (multiline).

>     JSPtrTable          gcIteratorTable;
> 
>+    /* Runtime data to support close hooks. */
>+    JSGCCloseData       gcClose;

Nit: gcCloseData would be more consistent for the member name.

>+    rt->gcClose.runTailp = &rt->gcClose.runHead;

Couple of nits here: "todo" seems more accurate than "run" (torun?  ugh), and the p on the end of runTailp always throws old lisp hackers -- they think you mean "predicate".  I personally think "runTail" or "todoTail" is fine without the final p.

>+    rt->gcClose.reachableList = NULL;
>+    rt->gcClose.runHead = NULL;
>+    rt->gcClose.runTailp = &rt->gcClose.runHead;
>+    rt->gcClose.nrun = 0;

Given the other names, "nrun" seems a bit Unix-y and brief.  How about runCount (or todoCount if you go for todo instead of run suggestion)?

>+    JS_ASSERT(obj);
>+    JS_ASSERT(item);

Null assertions are vacuous given fatal assert -- null deref crashes just as nicely and with as good or better blame in a debugger, and without the source code bloat.

>+    if (gckind != GC_LAST_CONTEXT) {
>+        if (firstFound) {
>+            /*
>+             * Mark just found unreachable objects with close hooks *after* we
>+             * scan gcCloseList so a object that refer to other objects with

"an object that refers"

>+             * close hooks would not keep them on gcCloseList.

s/would not/cannot/

>+             */
>+            MarkCloseList(cx, firstFound);
>+        }
>+    } else {
>+        /*
>+         * Remove scheduled hooks on shutdown as it is too late to run them:
>+         * we do not allow to excute arbitrary scripts at this point.

s/to excute/execution of/

>+         */
>+        if (rt->gcClose.nrun != 0) {
>+            JS_ASSERT(rt->gcClose.runHead);
>+            JS_ASSERT(rt->gcClose.runTailp != &rt->gcClose.runHead);
>+            rt->gcClose.runHead = NULL;
>+            rt->gcClose.runTailp = &rt->gcClose.runHead;
>+            rt->gcClose.nrun = 0;
>+        }
>     }

Perhaps invert if (gckind != GC_LAST_CONTEXT) test to make the shutdown case clearer? Not sure why this struck me as less desirable order of code -- your call.

>+typedef struct JSGCCloseData {

Just thought of "JSGCCloseState" and "gcCloseState" as more consistent and precise names.

>+    /*
>+     * Singly linked list of items tracking objects of extended classes that
>+     * have non-null close hooks wich are reachable from GC roots or were

Typo: "wich"

Wondering if the abstraction here is worth it, given "objects" above (of extended classes seems out of date, btw).  Could have JSPtrListItem be JSObjListItem, with ptr => obj.  Or could just put JSGenerator in jsiter.h and use it concretely.  Latter is too concrete, in case we need another closeable object.  But is Ptr too abstract?  We'd never have a string or other primitive GC-thing on the list.  We could have an XML or similar, I suppose, maybe.

>+typedef enum JSGCInvocationKind {
> 
>+    /* Normal invocation. */
>+    GC_NORMAL,
>+
>+    /*
>+     * Called from js_DestroyContext for last JSContext in a JSRuntime, when
>+     * it is imperative that rt->gcPoke gets cleared early in js_GC.
>+     */
>+    GC_LAST_CONTEXT,
>+
>+    /*
>+     * Called from js_NewGCThing as a last-ditch GC attempt. See comments
>+     * before js_GC definition for details.
>+     */
>+    GC_LAST_DITCH
>+
>+} JSGCInvocationKind;

The blank lines after the { and before the } seem unnecessary to me -- total style nit, forgot to mention it in a previous review.

Looks good, sorry for all the nits.

/be

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 231656 [details] [diff] [review]
Implementation v9

>+     * Run close hooks for objects that becomes unreachable after the last
>+     * GC.

I'll wait for the next patch to stamp this, but it looks good. One nit: "becomes" -> "became" in the above comment.

Comment 37

[Igor Bukanov]

Attachment: Implementation v10

Changes compared with the previous version:

1. Close hook code now lives inside "#if JS_HAS_GENERATORS" and one can compile and run SM with JS_HAS_GENERATORS set to 0. The code use in several places style like:

#if JS_HAS_GENERATORS
    /* Comments. */
    XXX;
#endif

where there is no blank line before comments. I think this confirm to that "no blank line before comments after  {}" rule.  

2. Useless JSPtrLinkItem abstraction is removed in favor of using JSGenarator explicitly.

3. Nits are addressed: "todoHead" looks much better than "runHead" indeed.

Comment 38

[Igor Bukanov]

Attachment: Implementation v11

Another update to add missed METER(...nclose = 0) to FinishGC and use better descriptions in gc-dump stats.

Comment 39

[Brendan Eich [:brendan]]

Comment on attachment 231723 [details] [diff] [review]
Implementation v11

>+/*
>+ * First half of the close phase: loop over the objects that we set aside
>+ * in the close table and mark them to protect them against finalization

s/close table/reachable list/ or words to that effect.

>+    JSGenerator *firstFound, **itemp, *item;

Wondering whether todo, genp, and gen wouldn't be better names?

>+    JSGenerator *item;

Same naming convention thought.

>+    /*
>+     * It is OK to access todoSize outside the lock here. When many threads
>+     * update the todo list, accessing some older value of todoSize in the
>+     * worst case just delays the excution of close hooks.
>+     */
>+    if (rt->gcCloseState.todoSize == 0)
>+        return JS_TRUE;

Nit: Size/size usually means byte-count, "length" or "count" is better for quantities abstracted over bytes, or list lengths, set populations, etc.  Of course todoCount is one char longer, but the loss of symmetric length w.r.t. Head and Tail may be worth it ;-).

>+#if JS_HAS_GENERATORS
>+
>+typedef enum JSGeneratorState {
>+    JSGEN_NEWBORN,
>+    JSGEN_RUNNING,
>+    JSGEN_CLOSED
>+} JSGeneratorState;
>+
>+struct JSGenerator {
>+    JSGenerator         *next;
>+    JSObject            *obj;
>+    JSGeneratorState    state;
>+    JSStackFrame        frame;
>+    JSArena             arena;
>+    jsval               stack[1];
>+};
>+
> extern JSObject *
> js_NewGenerator(JSContext *cx, JSStackFrame *fp);
> 
> extern JSBool
>-js_CloseGeneratorObject(JSContext *cx, JSObject *obj);
>+js_CloseGeneratorObject(JSContext *cx, JSGenerator *gen);
>+
>+#endif
> 
>-extern JSClass          js_GeneratorClass;
> extern JSClass          js_IteratorClass;
> extern JSClass          js_StopIterationClass;
>+
>+#if JS_HAS_GENERATORS
>+extern JSClass          js_GeneratorClass;
> extern JSClass          js_GeneratorExitClass;
>+#endif

I wouldn't mind if you moved the un#if'ed iteration class decls up above the big #if JS_HAS_GENERATORS and fused the little one at the end to the big one.

>+#if JS_HAS_GENERATORS
>+typedef struct JSGenerator          JSGenerator;
>+#endif

jsprvtd.h doesn't include jsconfig.h, and not all includers do it, plus (IIRC) C doesn't mind anonymous struct typedefs.  So lose the #if here.

/be

Comment 40

[Igor Bukanov]

Attachment: Implementation v12

Patch update to reflect the nits from comment 39:

1. todoSize -> todoCount
2. I moved JS_HAS_GENERATORS-only definitions in jsiter.c to single ifdef
3. I changed commnents to use consistently "generator" instead of "object with close hook".

Comment 41

[Brendan Eich [:brendan]]

Comment on attachment 231783 [details] [diff] [review]
Implementation v12

Oops, Iterator, iterator_self, etc. are not dependent on #if JS_HAS_GENERATORS -- they are needed for the for-in loop's implementation.  Please revert most or all of the jsiter.c change with respect to the v11 patch.

/be

Comment 42

[Brendan Eich [:brendan]]

(In reply to comment #41)
> (From update of attachment 231783 [details] [diff] [review] [edit])
> Oops, Iterator, iterator_self, etc. are not dependent on #if JS_HAS_GENERATORS
> -- they are needed for the for-in loop's implementation.  Please revert most or
> all of the jsiter.c change with respect to the v11 patch.

Or tell me if you have tested for-in, for-each-in loops without JS_HAS_GENERATORS and with the v12 #if's and it all works -- my memory says it should not, but I could be wrong.

/be

Comment 43

[Igor Bukanov]

(In reply to comment #42)
> (In reply to comment #41)
> > (From update of attachment 231783 [details] [diff] [review] [edit] [edit])
> > Oops, Iterator, iterator_self, etc. are not dependent on #if JS_HAS_GENERATORS
> > -- they are needed for the for-in loop's implementation.  Please revert most or
> > all of the jsiter.c change with respect to the v11 patch.
> 
> Or tell me if you have tested for-in, for-each-in loops without
> JS_HAS_GENERATORS and with the v12 #if's and it all works -- my memory says it
> should not, but I could be wrong.

It compiles, I will run some tests.

Comment 44

[Igor Bukanov]

It does not run, 
for (i in [1,2,3]) print(i)
generates:
InternalError: unimplemented JavaScript bytecode 208

But *without* the patch SM does not compile at all with JS_HAS_GENERATORS == 0.
So the patch improves the situation. Moreover, to test that GC works with
JS_HAS_GENERATORS == 0 I have to make sure that at least SM compiles.

Comment 45

[Brendan Eich [:brendan]]

(In reply to comment #44)
> It does not run, 
> for (i in [1,2,3]) print(i)
> generates:
> InternalError: unimplemented JavaScript bytecode 208
> 
> But *without* the patch SM does not compile at all with JS_HAS_GENERATORS == 0.
> So the patch improves the situation. Moreover, to test that GC works with
> JS_HAS_GENERATORS == 0 I have to make sure that at least SM compiles.

Still, it ought to be possible to leave the iteration support un#if'ed and have best of both worlds.  Can you mostly revert the jsiter.c patch to the v11 form and test that?

/be

Comment 46

[Igor Bukanov]

(In reply to comment #45)
> Still, it ought to be possible to leave the iteration support un#if'ed and have
> best of both worlds.  Can you mostly revert the jsiter.c patch to the v11 form
> and test that?

There is no difference between v11 and v12 besides code movements: the code is under "if JS_HAS_GENERATORS" in both cases.

Do you mean you would like to have a patch that does not touch code unrelated to this bug and leaves the compilation problem with JS_HAS_GENERATORS==0 to a separated bug? 

Comment 47

[Brendan Eich [:brendan]]

Sorry, I misread the patch, and thought I had !JS_HAS_GENERATORS working at one point.  Sure, separate bug unless it's trivial to piggy-back (we don't QA all the configs that aren't the default, so it doesn't hurt regression testing to fix it here).

/be

Comment 48

[Igor Bukanov]

Attachment: Implementation v13

This a version of the previous patch where I removed all changes to fix compilation problem when JS_HAS_GENERATORS set to 0. This is a job for bug 347065.

Comment 49

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 231815 [details] [diff] [review]
Implementation v13

>+     * Temporarily set aside cx->fp here to prevent the close hooks from
>+     * running on the GC's interpreter stack.
>      */
>-    rt->gcClosePhase = JS_TRUE;
>     fp = cx->fp;
>     cx->fp = NULL;

Here we set aside cx->fp storing it in a local fp, and then call into arbitrary user code, which could potentially cause a GC. The problem is that the original cx->fp is no longer on the stack, and thus won't be marked, creating a GC hazard. We need to make sure that we push the original cx->fp onto cx->dormanFrameChain so that those frames are still marked.

Other than that, this patch looks good.

Comment 50

[Igor Bukanov]

(In reply to comment #49)
> (From update of attachment 231815 [details] [diff] [review] [edit])
> >+     * Temporarily set aside cx->fp here to prevent the close hooks from
> >+     * running on the GC's interpreter stack.
> >      */
> >-    rt->gcClosePhase = JS_TRUE;
> >     fp = cx->fp;
> >     cx->fp = NULL;
> 
> Here we set aside cx->fp storing it in a local fp, and then call into arbitrary
> user code, which could potentially cause a GC. The problem is that the original
> cx->fp is no longer on the stack, and thus won't be marked, creating a GC
> hazard. We need to make sure that we push the original cx->fp onto
> cx->dormanFrameChain so that those frames are still marked.

Yes, but I wonder why it is problematic to "running on the GC's interpreter stack"? After the patch it is no longer true I suspect.

Comment 51

[Igor Bukanov]

Attachment: Implementation v14

Compared with the previous version this patch just removes setting aside cx->fp.

Comment 52

[Brendan Eich [:brendan]]

(In reply to comment #51)
> Created an attachment (id=232289) [edit]
> Implementation v14
> 
> Compared with the previous version this patch just removes setting aside
> cx->fp.

We probably do not want close hooks using caller or other means to backtrace into whatever stack might be active under js_RunCloseHooks.  The GC-safe way to start a new frame chain on a cx is to set the old one aside using cx->dormantFrameChain and fp->dormantNext -- see js_Execute and ReportIsNotFunction at least.

/be

Comment 53

[Igor Bukanov]

Attachment: Implementation v15

The new version put the set-aside frame to the dormant list.

Comment 54

[Brendan Eich [:brendan]]

Comment on attachment 232367 [details] [diff] [review]
Implementation v15

With that fix for the good catch by mrbkap, I say r+.  Thanks,

/be

Comment 55

[Brendan Eich [:brendan]]

Comment on attachment 232367 [details] [diff] [review]
Implementation v15

mrbkap is in Vegas this weekend, but he and I talked about this patch yesterday, and I'm sure I can mark second-review+ for him, given the stack set-aside fix.

/be

Comment 56

[Igor Bukanov]

I committed to the trunk the patch from comment 53. 

Comment 57

[Igor Bukanov]

Attachment: Implementation v15 - 1.8.1 version

The only difference compared with the trunk version is that this patch does not add 
include "jsexn.c"
to the jsgc.h as it is already there. 

The story behind this deviation is the following. When I committed to the trunk the patch for bug 346450, I missed that include there. That caused an unnoticed warning about missed function declaration that lasted until the patch for this bug was committed removing the warning. But for the branch I do not want to commit problematic patches, hence a temporary divergence. Sorry for the mess.

Comment 58

[Mike Schroepfer]

Comment on attachment 232422 [details] [diff] [review]
Implementation v15 - 1.8.1 version

a=schrep for drivers.

Comment 59

[Igor Bukanov]

I committed the patch from comment 57 to MOZILLA_1_8_BRANCH

Comment 60

[Bob Clary [:bc] (inactive)]

Checking in regress-341821.js;
/cvsroot/mozilla/js/tests/js1_7/iterable/regress-341821.js,v  <--  regress-341821.js
initial revision: 1.1

Note in 2006-08-01 bon echo windows builds, this would crash on exit and immediately restart the browser. 

Comment 61

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 win/mac(ppc|tel)/linux 20060808 normal exits, no time outs.

not fixed 1.8 win/mac(ppc|tel)/linux 20060808 time outs in the qa farm tests. 

debug crash windows xp:

#ifdef DEBUG
        if (tracefp) {
            intN ndefs, n;
            jsval *siter;

=>            ndefs = js_CodeSpec[op].ndefs;


		op	52682769	int

>	js3250.dll!js_Interpret(JSContext * cx=0x02f7c7b8, unsigned char * pc=0x0325b6f0, long * result=0x0012dbb8)  Line 6158 + 0x6 bytes	C
 	js3250.dll!generator_send(JSContext * cx=0x02f7c7b8, JSObject * obj=0x0334db30, unsigned int argc=0, long * argv=0x032f1188, long * rval=0x0012dcdc)  Line 789 + 0x14 bytes	C
 	js3250.dll!generator_close(JSContext * cx=0x02f7c7b8, JSObject * obj=0x0334db30, unsigned int argc=0, long * argv=0x032f1188, long * rval=0x0012dcdc)  Line 840 + 0x17 bytes	C
 	js3250.dll!js_Invoke(JSContext * cx=0x02f7c7b8, unsigned int argc=0, unsigned int flags=2)  Line 1350 + 0x1a bytes	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x02f7c7b8, JSObject * obj=0x0334db30, long fval=53795656, unsigned int flags=0, unsigned int argc=0, long * argv=0x00000000, long * rval=0x0012de14)  Line 1448 + 0x14 bytes	C
 	js3250.dll!js_CloseGeneratorObject(JSContext * cx=0x02f7c7b8, JSGenerator * gen=0x0323e010)  Line 617 + 0x1b bytes	C
 	js3250.dll!js_RunCloseHooks(JSContext * cx=0x02f7c7b8)  Line 1057 + 0xd bytes	C
 	js3250.dll!JS_GC(JSContext * cx=0x02f7c7b8)  Line 1945 + 0x9 bytes	C
...

Comment 62

[Bob Clary [:bc] (inactive)]

(In reply to comment #61)

Also same issue with 1.8 on
<http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=js1_7/iterable/regress-340526-02.js;language=type;text/javascript;version=1.7>

Should I file a new bug on the regression?

Comment 63

[Bob Clary [:bc] (inactive)]

filed Bug 348027 

Comment 64

[Bob Clary [:bc] (inactive)]

ff2b2 verified fixed 1.8 windows/linux. note to self: modify test so that alerts do not fire before page load so dialog closer can do its thing.

Comment 65

[Bob Clary [:bc] (inactive)]

note to self: js1_7/iterable/regress-341821.js is crashing on mac in the test
automation but it appears to be a result of spider's dialog closer closing the
slow script warning dialog.

Comment 66

[Bob Clary [:bc] (inactive)]

Checking in regress-341821.js;
/cvsroot/mozilla/js/tests/js1_7/iterable/regress-341821.js,v  <--  regress-341821.js
new revision: 1.3; previous revision: 1.2