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

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
12 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: mrbkap)

Tracking

(Blocks: 1 bug, {crash, testcase, verified1.8.1})

Trunk
mozilla1.8.1
crash, testcase, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

12 years ago
javascript:with({}) let y;

Assertion failure: pn2->pn_slot >= 0 || varOrConst,
at jsemit.c:3478 [@ EmitVariables]
(Assignee)

Comment 1

12 years ago
Created attachment 236092 [details] [diff] [review]
Fix

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

Updated

12 years ago
Flags: blocking1.8.1?
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
Target Milestone: --- → mozilla1.8.1

Updated

12 years ago
Blocks: 336378
(Assignee)

Comment 4

12 years ago
Created attachment 236184 [details] [diff] [review]
Better fix

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-
(Assignee)

Comment 6

12 years ago
Created attachment 236354 [details] [diff] [review]
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

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
(Assignee)

Comment 10

12 years ago
Created attachment 237062 [details] [diff] [review]
Even better fix
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
(Assignee)

Comment 12

12 years ago
Created attachment 237068 [details] [diff] [review]
Even better-er fix
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+
(Assignee)

Comment 14

12 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 15

12 years ago
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?

Comment 17

12 years ago
verified fixed 1.9 20060909 windows/mac*/linux
Status: RESOLVED → VERIFIED

Comment 18

12 years ago
Comment on attachment 237068 [details] [diff] [review]
Even better-er fix

a=schrep
Attachment #237068 - Flags: approval1.8.1? → approval1.8.1+

Updated

12 years ago
Whiteboard: [checkin needed (1.8 branch)]
Fixed on the 1.8 branch.

/be
Keywords: fixed1.8.1

Comment 20

12 years ago
verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux
Keywords: fixed1.8.1 → verified1.8.1
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.