373678 - Missing quotes around string in decompilation, with for..in and do..while

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> function() { do {for(a.b in []) { } } while("c\\d"); } 
function () {
    do {
        for (a.b in []) {
        }
    } while (c\d);
}

Comment 1

[Jeff Walden [:Waldo]]

More qnamepart fun not turning off inXML.

Comment 2

[Igor Bukanov]

(In reply to comment #1)
> More qnamepart fun not turning off inXML.
> 

I suspect that this is caused by the abuse of JSOP_QNAMEPART in EmitElemOp from jsemit.c. This XML-only bytecode is used just to allow to decompile JSOP_STRING as name. I think we should just add a new source note for JSOP_STRING and replace JSOP_QNAMEPART/JSOP_TOATTRNAME pair by a single JSOP_ATTRNAME.

Comment 3

[Jesse Ruderman]

What part of this testcase is interpreted as being E4X-related?

Comment 4

[Jeff Walden [:Waldo]]

js> dis(function() { do {for(a.b in []) { } } while("c\\d"); })
flags: LAMBDA INTERPRETED
main:
00000:  nop
00001:  callname "Array"
00004:  newinit
00005:  endinit
00006:  forin
00007:  forelem
00008:  ifeq 21 (13)
00011:  name "a"
00014:  qnamepart "b"
00017:  enumelem
00018:  goto 7 (-11)
00021:  enditer
00022:  string "c\\\\d"
00025:  ifne 1 (-24)
00028:  stop

inXML gets turned on for the qnamepart bytecode, and nothing turns it off.

Comment 5

[Igor Bukanov]

(In reply to comment #3)
> What part of this testcase is interpreted as being E4X-related?

The emitter uses JSOP_QNAMEPART (defined only for JS_HAS_XML_SUPPORT) in the code that has nothing to with E4X. This is done to take advantage of the fact that compared with JSOP_STRING, JSOP_QNAMEPART when decompiling treats its atom as a name and does not put "" around it. This use of xml-only bytecode is wrong even ignoring the fact that JSOP_QNAMEPART incorrectly sets inXML. 

Comment 6

[Brendan Eich [:brendan]]

I have a patch to rename JSOP_QNAMEPART to JSOP_LOCALNAME. Remember that namespaces are coming in ES4/JS2. I don't think the emitter is abusing this op; rather, inXML should not be set for this op. (As usual, the decompiler is the hack to clean up.) Would that fix things?

/be

Comment 7

[Igor Bukanov]

Attachment: Minimal fix v1

The fix just removes inXML = JS_TRUE from JSOP_QNAMEPART. Fixing proliferation of JSOP_STRING synonymous is a subject of bug 373772.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 258425 [details] [diff] [review]
Minimal fix v1

r=me, and I will link bug to js1.7src. Easy branch patch if possible.

/be

Comment 9

[Igor Bukanov]

I committed the patch from comment 7 to the trunk:

Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.217; previous revision: 3.216
done

Comment 10

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/decompilation/regress-373678.js,v  <--  regress-373678.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/decompilation/browser.js,v  <--  browser.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/decompilation/shell.js,v  <--  shell.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/e4x/decompilation/regress-373678.js,v  <--  regress-373678.js
initial revision: 1.1

note 1.8.1 passes this already while 1.8.0 will fail due to decompilation of a.b as a["b"].

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 20070320 win/mac*/linux

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 258425 [details] [diff] [review]
Minimal fix v1

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers

Comment 13

[Igor Bukanov]

The patch fixes trunk-only regression caused by bug 352285. Since that bug is not nominated for branches, the approval flags from the patch should be dropped. 

Comment 14

[Igor Bukanov]

Comment on attachment 258425 [details] [diff] [review]
Minimal fix v1

The patch is not applicable to branches.

Comment 15

[Bob Clary [:bc] (inactive)]

This regressed the test <?f b\n\nc\nc?> in 
e4x/decompilation/decompile-xml-escapes.js:

FAILED!: Actual value:
FAILED!: 'function anonymous() {
FAILED!:     return <?f b\n\nc\nc?>;
FAILED!: }' does not contain '<?f b
FAILED!: 
FAILED!: c
FAILED!: c?>'!

Comment 16

[Bob Clary [:bc] (inactive)]

filed Bug 379846 on the regression