416601 - property cache can be left disabled after exit from a generator or trap handler

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

Currently the code that re-enables the property cache in the interpreter after an exit from middle of an with block does that only when en exception or error occur. But this ignores early exits forced by a trap handler or triggered by closing a generator.

Comment 1

[Igor Bukanov]

Here is a test case demonstrating the problem with a debug build:

~/m/ff/mozilla $ cat ~/m/y.js
function f()
{
    with (Math) {
        yield 1;
    }
}

var iter = f();
iter.next();
iter.close();
~/m/ff/mozilla $ ~/m/ff/mozilla/js/src/Linux_All_DBG.OBJ/js ~/m/y.js
Assertion failure: JS_PROPERTY_CACHE(cx).disabled == fp->pcDisabledSave, at jsinterp.c:6741
trace/breakpoint trap

Comment 2

[Igor Bukanov]

Attachment: v1

Backup: the main idea here is to add try notes for with, let and xml filter blocks so a generic stack unwind code will properly restore the stack without PutBlockObjects etc.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 302361 [details] [diff] [review]
v1

Wondering whether there a more minimal fix for the short run? Also would like to avoid forcedReturn altogether (no additional overhead for a rare case). Could use a frame flag bit, but I wonder if you can't get rid of this by winding the logic tighter -- if it's set only by JSTRAP_RETURN cases, and tested only when setting catchable, perhaps the JSTRAP_RETURN cases could goto somewhere different.

/be

Comment 4

[Igor Bukanov]

Attachment: v2

Here is another backup of a more minimal patch that do addrese compared with v1 the property cache enable/disable after an exit from a generator.

For that the patch adds JSOP_AFTERYIELD that together with JSOP_YIELD do the necessary number of enable/disable calls.

The patch also changes EmitJumpFixupPrefix to emit the fixups even for JSOP_RETURN to simplify the logic in the interpreter and avoid extra checks on return when there is no pending blocks.

This is untested.

Comment 5

[Igor Bukanov]

Attachment: v3

Here is a smaller version that fixes the issue and does not move the code. The fix still alters the emitter to insert missing [leavewith]/[leaveblock] bytecodes before the return for simpler logic in the interpreter.

Comment 6

[Igor Bukanov]

Attachment: v4

Compared with v3 I renamed out2: to exit: given the other 2 labels, forced_return: and do_return: that the patch adds. Plus the patch turns (Alloc|Free)RawStack into static functions as they are not used outside jsinterp.c.

I will ask for a review after some testing.

Comment 8

[Igor Bukanov]

Comment on attachment 303048 [details] [diff] [review]
v4

In the patch I missed the fact that that AllocRawStack must set the mark after the potential time stamp header.

Comment 9

[Igor Bukanov]

Attachment: v5

The new version removes that overzealous optimization of AllocRawStack the previous patch attempted, avoids that do_return label and do inlineCallCount != 0 checks only once on all code paths.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 303178 [details] [diff] [review]
v5

>+         * EmitNonLocalJumpFixup may add fixup bytecode to close an open try
>+         * blocks having finally clauses or to exit let blocks. We can't

Lose "an" before "open try blocks" and rewrap.

>+static JSClass *
>+IsEnteredWithOrBlock(JSContext *cx, JSObject *obj, int stackDepth)

Suggest IsActiveWithOrBlock, Entered is a bit awkward.

>+/*
>+ * Unwind the scope chain to match the given depth. The function sets fp->sp
>+ * on return to stackDepth.
>+ */
>+static JSBool
>+UnwindScope(JSContext *cx, JSStackFrame *fp, jsint stackDepth,

Maybe UnwindScopeChain is the better name. Not a big deal but thought I would mention it. Actually, since it unwinds fp->blockChain too, keep the name but change the comment to read "Unwind block and scope chains ...".

>+    for (;;) {
>+        obj = fp->scopeChain;
>+
>+        clasp = IsEnteredWithOrBlock(cx, obj, stackDepth);
>+        if (!clasp)
>+            break;

Nit: blank line after obj = fp->scopeChain; seems unnecessary, but blank line after break; is nice. Or no blank lines, it's not too dense here.

>+        for (i = CountWithBlocks(cx, fp); i != 0; --i)
>+            js_DisablePropertyCache(cx);

Really just want JS_PROPERTY_CACHE(cx).disabled += CountWithBlocks(cx, fp), with assertion against overflow. Could macroize to abstract a bit, but why bother? We're in jsinterp.c, it's all in the family.

>+        /*
>+         * To support generator_throw and to catch ignored exceptions,
>+         * fail if cx->throwing is set.
>+         */
>+        if (cx->throwing) {

Great to have this in the generator only case, now that filtering is done without leaving the interpreter.

>+            for (i = CountWithBlocks(cx, fp); i != 0; --i)
>+                js_EnablePropertyCache(cx);
>+            goto exit;

Same drill here, disabled -= CountWithBlocks().

>-         * Call debugger throw hook if set (XXX thread safety?).
>-         */
>+        /* Call debugger throw hook if set (XXX thread safety?). */

Since you are touching this comment, lose the ancient XXX.

>@@ -355,20 +355,20 @@ js_GetPrimitiveThis(JSContext *cx, jsval
>  */
> extern JSBool
> js_ComputeThis(JSContext *cx, jsval *argv);
> 
> /*
>  * NB: js_Invoke requires that cx is currently running JS (i.e., that cx->fp
>  * is non-null), and that vp points to the callee, |this| parameter, and
>  * actual arguments of the call. [vp .. vp + 2 + argc) must belong to the last
>- * JS stack segment that js_AllocStack or js_AllocRawStack allocated. The
>- * function may use the space available after vp + 2 + argc in the stack
>- * segment for temporaries so the caller should not use that space for values
>- * that must be preserved across the call.
>+ * JS stack segment that js_AllocStack allocated. The function may use the
>+ * space available after vp + 2 + argc in the stack segment for temporaries so

Comma before "so".

>+ * the caller should not use that space for value that must be preserved

s/value/values/

>+            if (!js_DefineNativeProperty(cx, obj, sprop->id,
>+                                         fp->spbase[slot], NULL, NULL,
>+                                         JSPROP_ENUMERATE | JSPROP_PERMANENT,
>+                                         SPROP_HAS_SHORTID, sprop->shortid,
>+                                         NULL)) {
>+                /*
>+                 * Assume failed js_DefineNativeProperty means out-of-memory
>+                 * or other quit-asap conditions.

Could be an exception thrown by clasp->addProperty -- can't we cope?

>+                 */
>+                normalUnwind = JS_FALSE;
>+                cx->throwing = JS_FALSE;
>+                goto out;
>+            }
>         }
>     }
> 
>-    return JS_SetPrivate(cx, obj, NULL);
>+  out:
>+    /* We must clear the private slot even with errors. */
>+    if (!JS_SetPrivate(cx, obj, NULL))

JS_SetPrivate is infallible -- we should stop testing it as we patch, make a clean sweep in a separate bug if necessary.

/be

Comment 11

[Igor Bukanov]

Attachment: v6 (nit fixer)

In the new version I addressed just the nits from the comment 10, it does not yet address:

> >+            if (!js_DefineNativeProperty(cx, obj, sprop->id,
> >+                                         fp->spbase[slot], NULL, NULL,
> >+                                         JSPROP_ENUMERATE | JSPROP_PERMANENT,
> >+                                         SPROP_HAS_SHORTID, sprop->shortid,
> >+                                         NULL)) {
> >+                /*
> >+                 * Assume failed js_DefineNativeProperty means out-of-memory
> >+                 * or other quit-asap conditions.
> 
> Could be an exception thrown by clasp->addProperty -- can't we cope?

Comment 12

[Igor Bukanov]

Attachment: v5-v6 delta

Comment 13

[Igor Bukanov]

Attachment: v7

The new version fixes:

> >+                /*
> >+                 * Assume failed js_DefineNativeProperty means out-of-memory
> >+                 * or other quit-asap conditions.
> 
> Could be an exception thrown by clasp->addProperty -- can't we cope?

To properly detects that out-of-memory was thrown, not an exception that a script can catch, the patch clears cx->throwing in js_ReportOutOFMemeory and sets normalUnwind to false only if js_DefineNativeProperty failed with cx->throwing unset.

Here is a delta from v6:

--- .pc/.snap/js/src/jsobj.c	2008-02-15 00:50:12.000000000 +0100
+++ js/src/jsobj.c	2008-02-15 01:14:30.000000000 +0100
@@ -1942,18 +1942,19 @@ js_PutBlockObject(JSContext *cx, JSObjec
             if (!js_DefineNativeProperty(cx, obj, sprop->id,
                                          fp->spbase[slot], NULL, NULL,
                                          JSPROP_ENUMERATE | JSPROP_PERMANENT,
                                          SPROP_HAS_SHORTID, sprop->shortid,
                                          NULL)) {
                 /*
-                 * Assume failed js_DefineNativeProperty means out-of-memory
-                 * or other quit-asap conditions.
+                 * Stop adding properties if we failed due to out-of-memory or
+                 * other quit-asap errors.
                  */
-                normalUnwind = JS_FALSE;
-                cx->throwing = JS_FALSE;
-                goto out;
+                if (!cx->throwing) {
+                    normalUnwind = JS_FALSE;
+                    goto out;
+                }
             }
         }
     }
 
   out:
     /* We must clear the private slot even with errors. */
--- .pc/unwind.patch/js/src/jscntxt.c	2008-01-30 02:32:03.000000000 +0100
+++ js/src/jscntxt.c	2008-02-15 01:10:58.000000000 +0100
@@ -895,15 +895,18 @@ js_ReportOutOfMemory(JSContext *cx)
             report.lineno = js_PCToLineNumber(cx, fp->script, fp->pc);
             break;
         }
     }
 
     /*
-     * If debugErrorHook is present then we give it a chance to veto
-     * sending the error on to the regular ErrorReporter.
+     * If debugErrorHook is present then we give it a chance to veto sending
+     * the error on to the regular ErrorReporter. We also clear a pending
+     * exception if any now so the hooks can replace the out-of-memory error
+     * by a script-catchable exception.
      */
+    cx->throwing = JS_FALSE;
     if (onError) {
         JSDebugErrorHook hook = cx->debugHooks->debugErrorHook;
         if (hook &&
             !hook(cx, msg, &report, cx->debugHooks->debugErrorHookData)) {
             onError = NULL;
         }

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 303405 [details] [diff] [review]
v7

I rewrote this:

>+         * EmitNonLocalJumpFixup may add fixup bytecode to close open try
>+         * blocks having finally clauses or to exit let blocks. We can't
>+         * simply transfer control flow to our caller in that case, because we
>+         * must gosub to those clauses from inner to outer, with the correct
>+         * stack pointer (i.e., after popping any with, for/in, etc., slots
>+         * nested inside the finally's try). In this case we mutate
>+         * JSOP_RETURN into JSOP_SETRVAL and add an extra JSOP_RETRVAL after
>+         * the fixups.

to this:

        /*
         * EmitNonLocalJumpFixup may add fixup bytecode to close open try
         * blocks having finally clauses and to exit intermingled let blocks.
         * We can't simply transfer control flow to our caller in that case,
         * because we must gosub to those finally clauses from inner to outer,
         * with the correct stack pointer (i.e., after popping any with,
         * for/in, etc., slots nested inside the finally's try).
         *
         * In this case we mutate JSOP_RETURN into JSOP_SETRVAL and add an
         * extra JSOP_RETRVAL after the fixups.
         */

Better? Use if so.

r/a=me, thanks for fixing this bug.

/be

Comment 15

[Igor Bukanov]

Attachment: v7b

Here is a version to check in with the fixed comment in jsemit.c.

Comment 16

[Igor Bukanov]

I checked in the version from comment 15 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1203075360&maxdate=1203075481&who=igor%25mir2.org

Comment 17

[Robert Sayre]

This patch regressed sunspider:

http://graphs.mozilla.org/graph.html#spst=range&spstart=0&spend=1203076814&bpst=cursor&bpstart=0&bpend=1203076814&m1tid=151880&m1bl=0&m1avg=0

Comment 18

[Igor Bukanov]

(In reply to comment #17)
> This patch regressed sunspider:

Here is the detailed data for the spider's tinderbox log:

03:07:26  788.84
03:25:10  795.98
03:38:01  782.38

the commit for this bug

03:56:48  765.34
04:10:55  759.26
05:01:42  793.52
05:27:21  790.24
	
The data is just too noisy to make any conclusions yet.

Comment 19

[Igor Bukanov]

(In reply to comment #17)
> This patch regressed sunspider:
> 
> http://graphs.mozilla.org/graph.html#spst=range&spstart=0&spend=1203076814&bpst=cursor&bpstart=0&bpend=1203076814&m1tid=151880&m1bl=0&m1avg=0

The check-in happens at 2008-02-15 03:38, after whcih at least 2 points on the graph show no regression. The regression happens between 05:00 and 06:00, when, for example, the check in for bug 405128 happened.

As such I mark the bug fixed again.

Comment 20

[Robert Sayre]

The patch didn't regress all sunspider tests equally. For example, a regression in access-fannkuch is pretty clear. Here's three runs before and after the patch:

access-fannkuch.html; 1381.5; 1376.5;  1365; 1385; 1365; 1378; 1385; 1380; 1383

access-fannkuch.html; 1367.5; 1363.25; 1357; 1384; 1369; 1361; 1366; 1384; 1357

access-fannkuch.html; 1381  ; 1371.75; 1358; 1386; 1367; 1383; 1386; 1358; 1379

-- patch --

access-fannkuch.html; 1470.5; 1446.75; 1419; 1529; 1419; 1511; 1430; 1427; 1529

access-fannkuch.html; 1514.5; 1465.25; 1414; 1520; 1418; 1414; 1518; 1520; 1511

access-fannkuch.html; 1516;   1467;    1415; 1552; 1421; 1415; 1552; 1516; 1516

Comment 21

[Robert Sayre]

> 
> The check-in happens at 2008-02-15 03:38, after whcih at least 2 points on the
> graph show no regression. The regression happens between 05:00 and 06:00, 

I see the regression in a run that started at 03:54:00, on qm-pxp-fast01.

Comment 22

[Igor Bukanov]

(In reply to comment #20)
> The patch didn't regress all sunspider tests equally. For example, a regression
> in access-fannkuch is pretty clear. Here's three runs before and after the
> patch:

I do not see a regression on Linux. Could you also apply the patch from bug 416439 on top of it? That bug targets code savings and needs some of code movements that this patch has done.

Comment 23

[Igor Bukanov]

Attachment: attempt to fix the regression

To Robert Sayre: could you try this patch on top of the patch in this bug to see if it would make a difference for you?

Comment 24

[Robert Sayre]

Yeah, I'll check this on windows, but it will take me a few hours.

Comment 25

[Robert Sayre]

(In reply to comment #24)
> Yeah, I'll check this on windows, but it will take me a few hours.
> 

I just realized, I don't have 2005pro, so I can't build with jemalloc. That might not matter, so I'll check anyway. Might be worth trying the patch on tinderbox.

Comment 26

[Robert Sayre]

(In reply to comment #25)
> (In reply to comment #24)
> > Yeah, I'll check this on windows, but it will take me a few hours.
> > 
> 
> I just realized, I don't have 2005pro, so I can't build with jemalloc. That
> might not matter, so I'll check anyway. Might be worth trying the patch on
> tinderbox.
> 

I checked this out--no difference, but unfortunately with a configuration not identical to the tinderbox.

Comment 27

[Igor Bukanov]

(In reply to comment #26)
> I checked this out--no difference, but unfortunately with a configuration not
> identical to the tinderbox.

Still, could you try the patch from comment 23? And even if it would show no difference with MSVC, how would it affect the code size of jsinterp.o?

Comment 28

[Robert Sayre]

(In reply to comment #27)
> 
> Still, could you try the patch from comment 23? 

Tried the patch, but didn't see a difference.

> And even if it would show no
> difference with MSVC, how would it affect the code size of jsinterp.o?

jsinterp.obj: 235282 with patch
              235289 without patch

Comment 29

[Brendan Eich [:brendan]]

Comment on attachment 303418 [details] [diff] [review]
v7b

Forgot to suggest using

    RESTORE_SP(fp);

instead of the sevaral

    sp = fp->sp;

occurrences.

/be

Comment 30

[Igor Bukanov]

i filed bug 417821 to investigate the performance regression not to mix performance-related problems with fixing the original bug.

Comment 31

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_7/regress/regress-416601.js,v  <--  regress-416601.js
initial revision: 1.1

Comment 32

[Bob Clary [:bc] (inactive)]

v