352285 - Decompiler escapes line breaks and backslashes in E4X CDATA literals (e4x/decompilation/decompile-xml-escapes.js)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Like bug 349814, but for cdata literals instead of element literals.

js> function() { var x = <![CDATA[\\]]> }
function () {
    var x = <![CDATA[\\\\]]>;
}

js> function() { var x = <![CDATA[  
]]> }
function () {
    var x = <![CDATA[\n]]>;
}

Comment 1

[Brendan Eich [:brendan]]

Where's Waldo?  I'm still dealing with maybe-brace madness required for let declarations.

/be

Comment 2

[Jeff Walden [:Waldo]]

This applies to CDATA, comments, and PIs:

js> function(){ return <![CDATA[foo
bar
baz]]>; }
function () {
    return <![CDATA[foo\nbar\nbaz]]>;
}
js> function(){ return <!--f
b
c
-->;
}
function () {
    return <!--f\nb\nc\n-->;
}
js> function(){ return <?f
b
c?>; }
function () {
    return <?f b\nc?>;
}

I'll look at this, but the odds of it happening for 1.8.1 are slim due to my time constraints.

Comment 3

[Brendan Eich [:brendan]]

If this is like the other bug 349814, the fix may be as easy as ensuring that QUOTE_IN_XML is set when strings are decompiled using QuoteString.

/be

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Patch

(This also includes the kinda-working patch for bug 355674.)

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Testcase

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 242306 [details] [diff] [review]
Patch

Sorry I forgot about this one -- it's overdue. Nom for branches as you will.

/be

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Patch as checked in

This is what I'm checking in -- it's the same modulo unbitrotting and the removal of the (incorrect) fix for bug 355674.

Comment 8

[Jeff Walden [:Waldo]]

...and fixed.  I'll give it some time on trunk and then nominate for branches.

I would have checked in the testcase, but I'm unsure where a decompilation bug testcase should go (since semantics-preserving decompilation isn't required by the spec).

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> ...and fixed.  I'll give it some time on trunk and then nominate for branches.
> 
> I would have checked in the testcase, but I'm unsure where a decompilation bug
> testcase should go (since semantics-preserving decompilation isn't required by
> the spec).

ECMA-262 does require of Function.prototype.toString that:

"An implementation-dependent representation of the function is returned. This representation has the syntax of a FunctionDeclaration. Note in particular that the use and placement of white space, line terminators, and semicolons within the representation string is implementation-dependent."

In fact real-world interop (e.g., user-generated ebay.com content) requires that the function's string representation be compilable into an equivalent function. We will clarify this for ES4. So I think this bug's test belongs in the core ECMA part of the suite.

/be

Comment 10

[Bob Clary [:bc] (inactive)]

I need to check in a fix to correct an inappropriate use of netscape.security in non-gecko browsers, but we will still have issues with tests like js1_5/Regress/regress-349596.js where we decompile function() { L: if (0) return 5 } as function ( ) { L : { } } and IE7 decompiles it as function ( ) { L : if ( 0 ) return 5 } (modulo our compareSource function). Shouldn't these type tests still be in /extensions/ ?

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> I need to check in a fix to correct an inappropriate use of netscape.security
> in non-gecko browsers, but we will still have issues with tests like
> js1_5/Regress/regress-349596.js where we decompile function() { L: if (0)
> return 5 } as function ( ) { L : { } } and IE7 decompiles it as function ( ) {
> L : if ( 0 ) return 5 } (modulo our compareSource function).

Both are allowed by ECMA, since the functions are semantically equivalent.

> Shouldn't these
> type tests still be in /extensions/ ?

I don't think so, as noted above.

/be

Comment 12

[Jeff Walden [:Waldo]]

(In reply to comment #10)
> Shouldn't these type tests still be in /extensions/ ?

Depends how they're written.  This test, for example, should be decompilation-agnostic, because the character sequence that makes up the literal in the decompiled code must match *exactly*.  However, even this might fail for versions which decompiled XML literals to XML("..."), tho that seems a very unlikely decompilation, implementation-wise.

I think with some care you can make nearly all decompilation tests implementation-agnostic, but it's not particularly easy to do so.

Comment 13

[Bob Clary [:bc] (inactive)]

mid-air.

(In reply to comment #11)

> > Shouldn't these
> > type tests still be in /extensions/ ?
> 
> I don't think so, as noted above.

I don't test semantics of the function but a simplified serialization of the function. Leaving these in the non-extensions directories will cause other browsers |implementations to fail the current tests unless they have an equivalent serialization to what we produce. If you have a way to test two serializations as functionally equivalent I can adapt the tests to use that.

Comment 14

[Jeff Walden [:Waldo]]

(In reply to comment #13)
> If you have a way to test two serializations as functionally equivalent I can
> adapt the tests to use that.

I highly doubt there's a silver bullet here.  The only approach is really to work from base principles and be really careful, as in the following examples:

js/tests/e4x/Namespace/regress-350442.js
js/tests/e4x/Regress/regress-350629.js

Basically, you can do it with lots of highly specific regular expressions and a bunch of care, but the cost of doing so is fairly high.  Also, working with XML syntax helped there, because XML has more limitations on output format than raw JS does.

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (In reply to comment #10)
> > Shouldn't these type tests still be in /extensions/ ?
> 
> Depends how they're written.  This test, for example, should be
> decompilation-agnostic, because the character sequence that makes up the
> literal in the decompiled code must match *exactly*.  However, even this might
> fail for versions which decompiled XML literals to XML("..."), tho that seems a
> very unlikely decompilation, implementation-wise.

It wouldn't even be guaranteed to produce the same results, because XML the property in the global object (until ES4) is permanent (DontDelete) but read/write (!ReadOnly). So literals are the only way to go.

One idea for Function.prototype.toString testing: normalize using narcissus in an AST, then match against a canonical AST. Still may require fuzz for extra braces and parens (beware let, braces become meaningful where they weren't before), and of course constant folding and dead code elimination. Pain, or a fun project for an undergrad.

/be

Comment 16

[Bob Clary [:bc] (inactive)]

In the meantime, I vote for either /extensions/ or /decompilation/.

Comment 17

[Brendan Eich [:brendan]]

decompilation, my two cents.

/be

Comment 18

[Jeff Walden [:Waldo]]

One of the tinderboxen went orange after this was checked in, so I assumed it was meaningful, tried to reproduce, and backed out when I couldn't.  It seems, however, that Tp2 had *just* been turned on on that tinderbox, so the crash was bogus.  I'd recommit except that I'm about dead right now, so I'll recommit within the next 24 hours, probably first thing in the morning.

I'm slightly abusing the system by not reopening (avoid dependency bugspam), so if by some chance I don't comment again within that time saying I've landed, feel free to reopen.

Comment 19

[Jeff Walden [:Waldo]]

Relanded.

Comment 20

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/e4x/decompilation/browser.js,v  <--  browser.js

/cvsroot/mozilla/js/tests/e4x/decompilation/decompile-xml-escapes.js,v  <--  decompile-xml-escapes.js

/cvsroot/mozilla/js/tests/e4x/decompilation/shell.js,v  <--  shell.js

Comment 21

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 20070226 windows/mac*/linux

Comment 22

[Bob Clary [:bc] (inactive)]

bug 373678 regressed this. filed bug 379846 on the regression.