559438 - Decompiler bug with CONCATN

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

js> function f() { return "" + 2 + (i + 1); }
js> f
function f() {return "" + 2 + i + 1;}

Note the subtle change in meaning there.

Comment 1

[Jason Orendorff [:jorendorff]]

hasenj, the contributor who originally reported this bug, spent days working with me to reduce it from a complex web application.

The initial bug was simply that the former code was mysteriously behaving like the latter. We suspected the interpreter, the tracer, and ultimately the parser before discovering in a marathon gdb session that the code being handed to the JS compiler was actually subtly different from the script on disk.

hasenj ultimately discovered that the application has module-loading code that takes functions, converts them to strings, and passes the strings back to Function by way of eval, or something.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 3

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

My bad, I bet. r=shaver

Comment 4

[Jeff Walden [:Waldo]]

There's either a decompilation or extensions directory the test should go in, no?

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Why do we care?  We always have to search for them that way (else memorize JS and ECMA version history), and people who want to re-use our test suite can filter out whatever they don't want.  This bug affects web content, so they might well want to include it.

Comment 6

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/d9e5e022384f

Waldo, r=me to move the test wherever you want, if shaver's argument in comment 5 doesn't satisfy you.

The test is a little different from most decompiler tests in that it's directly testing behavior.

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Asserting that we want to fix this compat bug in the next 3.6 update.

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/d9e5e022384f

Comment 9

[fcp]

(In reply to comment #8)
> http://hg.mozilla.org/mozilla-central/rev/d9e5e022384f

> +function f(x) { return 1 + "" + (x + 1); }
> +reportCompare("12", f(1), "");
> +var g = eval(String(f));
> +reportCompare("12", f(1), "");
Shouldn’t f(1) on the last line be g(1)?

Comment 10

[fcp]

(In reply to comment #9)
> Shouldn’t f(1) on the last line be g(1)?
Oops, never mind.  Now I see how the test works (the eval redefines the function f).

Comment 11

[Jason Orendorff [:jorendorff]]

fcp: You're right. peterv noticed this yesterday too, and I changed the test.

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/9a9694a03441

Comment 14

[Boris Zbarsky [:bzbarsky]]

Jason, we should probably fix this on 1.9.2...  This is breaking greasemonkey users there, at least, plus whatever web app you were looking at, right?

Comment 15

[LegNeato]

We'd like to take this for 1.9.2.14.

Comment 16

[Peter Wu]

Attachment: Demonstrates some failures

The bug still exists.

I can confirm this error on both 3.6.17 and 4.0.1 (tarball from www.firefox.com) running on Ubuntu 10.04.

Comment 17

[Peter Wu]

I must have kept open the wrong window, 4.0.1 is fine, only 3.6.17 is affected (and possibly previous versions).