try { let([] = c) 1; } catch(e) { this.a.b; } Expected: TypeError: this.a is undefined Result in opt: TypeError: (void 0) is undefined Result in debug: Assertion failure: *pcstack[pcdepth - 1] == JSOP_ENTERBLOCK, at jsopcode.c:5150 This LOCAL_ASSERT is part of ReconstructPCStack, which was added in bug 420399.

> Assertion failure: *pcstack[pcdepth - 1] == JSOP_ENTERBLOCK, at This assertion is bogus. let([] = ...) generates let blocks with zero locals. Thus in this case the top-most pc on pcstack points to the pc that generated the let expression (JSOP_ONE).

After looking at this I have found another bad bug in the code: DecompileCode still access the stack of the last interpreted frame to find the pc of the missing operands.

The patch fixes the assert and changes the decompiler logic to use the modeled pcstack, not something below fp->spbase. Another fix is for the generator to allocate just depth slots for the stack, not depth * 2. While looking into this I was wondering why the decompiler has to recurs into DecompileValueGenerator in GetOff. AFAICS if the decompiler starts from the expression statement start, it should have all the necessary slots computed by the time it reaches pc, shouldn't it?

This should block. /be

(In reply to comment #3) > While looking into this I was wondering why the decompiler has to recurs into > DecompileValueGenerator in GetOff. AFAICS if the decompiler starts from the > expression statement start, it should have all the necessary slots computed by > the time it reaches pc, shouldn't it? See bug 346642. One example: with({x: (new (b = 1))}) (2).x /be

Comment on attachment 308334 [details] [diff] [review] v1 >+#define FAILED_EXPRESSION_DECOMPILER ((char *) 1) Safe enough if some caller fails to test for this, a nearly null crash. A static char FAILURE_COOKIE[] = "..." would be even safer and maybe prettier. >+/* >+ * Decompile a part of expression up to the given pc. The function returns >+ * NULL on out-of-memory, FAILED_EXPRESSION_DECOMPILER sentinel when the "or" after the comma above. >+ * decompiler fails due to a bug and/or unimplemented feature or the Doubled space before "or the" should be ", ". >+ * decompiled string. Suggest "on success" to finish this sentence in context. >+ name = DecompileExpression(cx, script, fp->fun, pc); >+ if (name == FAILED_EXPRESSION_DECOMPILER) >+ goto do_fallback; >+ return name; Could invert the FAIL... test and return name in the consequent, avoid a goto. Patch seems good, I tried some torture-tests from Jesse's comments in bug 346642. r=me if it tests well in the suite, which IIRC has regression tests for all fuzz-found decompiler bugs. Bob, please deny if I'm wrong ;-). In that event we need Jesse to re-fuzz. /be

Comment on attachment 308334 [details] [diff] [review] v1 "modelled" => "modeled" > The decompiler wants to see on pcstack [leaveblockexpr], not [leaveblockexpr] on the pcstack > [enterblock] or the pc that ended the a simulated let the and a are both articles, pick precisely one > expression when [enterblock] defines zero locals like in "like in" => "as in:"

(In reply to comment #5) > See bug 346642. One example: > > with({x: (new (b = 1))}) (2).x I have not find a particular explanation for this in this bug. Why decompiling {x: (new (b = 1))} inside with is problematic while decompiling {x: (new (b = 1))} is not? Or is the real problem is decompiling of new (b = 1) ?

(In reply to comment #6) > Safe enough if some caller fails to test for this, a nearly null crash. A > static char FAILURE_COOKIE[] = "..." would be even safer and maybe prettier. DecompileExpression returns a freshly-allocated string. As such returning FAILURE_COOKIE does not add robustness if a caller forget to check for the cookie. It will crash when trying to free(FAILURE_COOKIE). Or is the suggestion to simply return JS_strdup(cx, "/* DECOMPILER BUG */") on errrors?

(In reply to comment #9) > Or is the suggestion to simply return JS_strdup(cx, "/* DECOMPILER BUG */") on errrors? That will remove the need to check in callers for FAILED_EXPRESSION_DECOMPILER or FAILURE_COOKIE. On the other hand it will prevent a fallback based on uneval of a vlaue form the stack slot that caused the error for JSOP_BINDNAME: /* * JSOP_BINDNAME is special: it generates a value, the base object of a * reference. But if it is the generating op for a diagnostic produced by * js_DecompileValueGenerator, the name being bound is irrelevant. Just * fall back to the base object. */ if (op == JSOP_BINDNAME) return FAILED_EXPRESSION_DECOMPILER;

The new version just addresses the nits without touching FAILED_EXPRESSION_DECOMPILER: @@ -786,5 +786,5 @@ ReconstructPCStack(JSContext *cx, JSScri * Decompile a part of expression up to the given pc. The function returns - * NULL on out-of-memory, FAILED_EXPRESSION_DECOMPILER sentinel when the - * decompiler fails due to a bug and/or unimplemented feature or the - * decompiled string. + * NULL on out-of-memory, or FAILED_EXPRESSION_DECOMPILER sentinel when the + * decompiler fails due to a bug and/or unimplemented feature, or the + * decompiled string on success. */ @@ -4864,5 +4864,4 @@ js_DecompileValueGenerator(JSContext *cx name = DecompileExpression(cx, script, fp->fun, pc); - if (name == FAILED_EXPRESSION_DECOMPILER) - goto do_fallback; - return name; + if (name != FAILED_EXPRESSION_DECOMPILER) + return name; @@ -5164,5 +5163,5 @@ ReconstructPCStack(JSContext *cx, JSScri /* - * The decompiler wants to see on pcstack [leaveblockexpr], not - * [enterblock] or the pc that ended the a simulated let - * expression when [enterblock] defines zero locals like in + * The decompiler wants to see [leaveblockexpr] on pcstack, not + * [enterblock] or the pc that ended a simulated let expression + * when [enterblock] defines zero locals as in: *

Comment on attachment 308821 [details] [diff] [review] v1b >+ * Decompile a part of expression up to the given pc. The function returns >+ * NULL on out-of-memory, or FAILED_EXPRESSION_DECOMPILER sentinel when the One last nit: "the" before FAILED_EXPRESSION_DECOMPILER and rewrap (should look less ragged right to boot!). /be

Patch v1 holds up fine to 10 hours of jsfunfuzz. I even added something to jsfunfuzz in an attempt to find more "incorrect error message" DVG bugs like the one in comment 0, but only ended up finding bug 422501 and bug 422504.

The new version addresses the nit from the comment 12: #define FAILED_EXPRESSION_DECOMPILER ((char *) 1) /* * Decompile a part of expression up to the given pc. The function returns - * NULL on out-of-memory, or FAILED_EXPRESSION_DECOMPILER sentinel when the - * decompiler fails due to a bug and/or unimplemented feature, or the + * NULL on out-of-memory, or the FAILED_EXPRESSION_DECOMPILER sentinel when + * the decompiler fails due to a bug and/or unimplemented feature, or the * decompiled string on success. */

I checked in the patch from the comment 14 to the trunk: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1205362741&maxdate=1205363029&who=igor%25mir2.org

you ignored this: "modelled" => "modeled" :(

verified linux|mac|windows /cvsroot/mozilla/js/tests/js1_7/expressions/regress-421806.js,v <-- regress-421806.js initial revision: 1.1