Closed Bug 350730 Opened 18 years ago Closed 18 years ago

let in with causes "Assertion failure: pn2->pn_slot >= 0 || varOrConst" [@ EmitVariables]

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.8.1

People

(Reporter: jruderman, Assigned: mrbkap)

References

Details

(Keywords: crash, testcase, verified1.8.1)

Crash Data

Attachments

(1 file, 4 obsolete files)

javascript:with({}) let y;

Assertion failure: pn2->pn_slot >= 0 || varOrConst,
at jsemit.c:3478 [@ EmitVariables]
Attached patch Fix (obsolete) — Splinter Review
A with statement will never be a scope statement -- if it is, then it will have a child STMT_BLOCK that will be converted into a scope.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #236092 - Flags: review?(brendan)
Comment on attachment 236092 [details] [diff] [review]
Fix

It's odd that for a statement type t, STMT_TYPE_IS_SCOPE(t) does not imply STMT_TYPE_MAYBE_SCOPE(t).  Would STMT_TYPE_HAS_SCOPE (STMT_HAS_SCOPE in parallel) be a better name for the former?

r=me for the fix.

/be
Attachment #236092 - Flags: review?(brendan) → review+
Or since STMT_IS_SCOPE is used only in js_PushStatement and js_PopStatement (and STMT_TYPE_IS_SCOPE is used only in STMT_IS_SCOPE for parallel factoring to STMT_TYPE_MAYBE_SCOPE/STMT_MAYBE_SCOPE), would STMT_(TYPE_|)LINKS_SCOPE be even better?

/be
Flags: blocking1.8.1?
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
Target Milestone: --- → mozilla1.8.1
Blocks: js1.7let
Attached patch Better fix (obsolete) — Splinter Review
This brute-force patch fixes the problems with the STMT_TYPE macros. I didn't change the names to make this a smaller patch.
Attachment #236092 - Attachment is obsolete: true
Attachment #236184 - Flags: review?(brendan)
Comment on attachment 236184 [details] [diff] [review]
Better fix

>Index: jsemit.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsemit.c,v
>retrieving revision 3.190
>diff -p -U8 -r3.190 jsemit.c
>--- jsemit.c	29 Aug 2006 23:15:21 -0000	3.190
>+++ jsemit.c	31 Aug 2006 03:49:45 -0000
>@@ -236,18 +236,18 @@ const char js_with_statement_str[] = "wi
> const char js_finally_block_str[]  = "finally block";
> const char js_script_str[]         = "script";
> 
> static const char *statementName[] = {
>     "label statement",       /* LABEL */
>     "if statement",          /* IF */
>     "else statement",        /* ELSE */
>     "switch statement",      /* SWITCH */
>-    "block",                 /* BLOCK */
>     js_with_statement_str,   /* WITH */

You reverted the enum reordering, but not this table change -- that can't be right.

>+    "block",                 /* BLOCK */
> #define STMT_TYPE_MAYBE_SCOPE(type)                                           \
>-    STMT_TYPE_IN_RANGE(type, STMT_BLOCK, STMT_SUBROUTINE)
>+    (type != STMT_WITH &&                                                     \
>+     STMT_TYPE_IN_RANGE(type, STMT_BLOCK, STMT_SUBROUTINE))

Given the unchanged enum with block before with, this part seems ok -- but why not reorder and use the right ranges for MAYBE and IS?

/be
Attachment #236184 - Flags: review?(brendan) → review-
Attached patch Better still (obsolete) — Splinter Review
This leaves an assertion with the testcase in a function:
js> function f() { with ({}) let k; }
Assertion failure: pn2->pn_slot >= 0 || !let, at jsemit.c:3524

Which I'll file a new bug on, because it's a symptom of a larger misbehavior:
js> function f() { with({k : ''}) var k = 3; print(k); }
js> f()
undefined
Attachment #236184 - Attachment is obsolete: true
Attachment #236354 - Flags: review?(brendan)
(In reply to comment #6)
> Which I'll file a new bug on, because it's a symptom of a larger misbehavior:
> js> function f() { with({k : ''}) var k = 3; print(k); }
> js> f()
> undefined

That's correct behavior.  See ECMA 10.1.3 and 12.2.

/be
(In reply to comment #6)
> Created an attachment (id=236354) [edit]
> Better still
> 
> This leaves an assertion with the testcase in a function:
> js> function f() { with ({}) let k; }
> Assertion failure: pn2->pn_slot >= 0 || !let, at jsemit.c:3524

We need BindNameToSlot when called for a name being declared to use the precise scope in which the declaration should bind.  That would be the body of f, here, and the assertion would be satisfied.

Your flag idea is looking good at this point ;-).

/be
Flags: blocking1.8.1? → blocking1.8.1+
Need action here this week, sooner the better -- or else.

/be
Attached patch Even better fix (obsolete) — Splinter Review
Attachment #236354 - Attachment is obsolete: true
Attachment #237062 - Flags: review?(brendan)
Attachment #236354 - Flags: review?(brendan)
Comment on attachment 237062 [details] [diff] [review]
Even better fix


>+js_LexicalLookup(JSTreeContext *tc, JSAtom *atom, jsint *slotp, JSBool letdecl)
> {
>     JSStmtInfo *stmt;
>     JSObject *obj;
>     JSScope *scope;
>     JSScopeProperty *sprop;
>     jsval v;
> 
>     for (stmt = tc->topScopeStmt; stmt; stmt = stmt->downScope) {
>-        if (stmt->type == STMT_WITH)
>-            break;
>+        if (stmt->type == STMT_WITH) {
>+            /* Ignore with statements in let declarations. */
>+            if (!letdecl)
>+                break;
>+            continue;
>+        }

It would be more natural, and match the comment, to write if (letdecl) continue; break instead.

Also, how about let or letting?  Just a thought.

>Index: jsinterp.c

Stray file in patch.

/be
Attachment #237062 - Attachment is obsolete: true
Attachment #237068 - Flags: review?(brendan)
Attachment #237062 - Flags: review?(brendan)
Comment on attachment 237068 [details] [diff] [review]
Even better-er fix


> JSStmtInfo *
>-js_LexicalLookup(JSTreeContext *tc, JSAtom *atom, jsint *slotp)
>+js_LexicalLookup(JSTreeContext *tc, JSAtom *atom, jsint *slotp, JSBool letdecl)
> {
>     JSStmtInfo *stmt;
>     JSObject *obj;
>     JSScope *scope;
>     JSScopeProperty *sprop;
>     jsval v;
> 
>     for (stmt = tc->topScopeStmt; stmt; stmt = stmt->downScope) {
>-        if (stmt->type == STMT_WITH)
>+        if (stmt->type == STMT_WITH) {
>+            /* Ignore with statements in let declarations. */
>+            if (letdecl)
>+                continue;
>             break;
>+        }

Nit: "Ignore with statements enclosing a single let declaration."

r=me with that.

/be
Attachment #237068 - Flags: review?(brendan) → review+
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Checking in regress-350730.js;
/cvsroot/mozilla/js/tests/js1_7/block/regress-350730.js,v  <--  regress-350730.js
initial revision: 1.1
Flags: in-testsuite+
Comment on attachment 237068 [details] [diff] [review]
Even better-er fix

This needs to land ASAP.

/be
Attachment #237068 - Flags: approval1.8.1?
verified fixed 1.9 20060909 windows/mac*/linux
Status: RESOLVED → VERIFIED
Comment on attachment 237068 [details] [diff] [review]
Even better-er fix

a=schrep
Attachment #237068 - Flags: approval1.8.1? → approval1.8.1+
Whiteboard: [checkin needed (1.8 branch)]
Fixed on the 1.8 branch.

/be
Keywords: fixed1.8.1
verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux
Whiteboard: [checkin needed (1.8 branch)]
Crash Signature: [@ EmitVariables]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: