301692 - E4X: Function.prototype.toString quotes XML literals

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Aiko]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b3) Gecko/20050712 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b3) Gecko/20050712 Firefox/1.0+

Calling a function's toString or toSource method doesn't generate the right
result, XML literals are quoted.

Reproducible: Always

Steps to Reproduce:
1. Evaluate String(function() { <xml/> })

Actual Results:  
function () {
    "<xml/>";
}

Expected Results:  
function () {
    <xml/>;
}

Comment 1

[Brendan Eich [:brendan]]

I confess that I got sleepy at some point and didn't test the decompiler fully.
 Shaver, mrbkap: want to have some decompiler+E4X phun?

/be

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Shaver and I talked about this on IRC. I suspect he could fix this faster, but
he has other stuff to do, so I get this. The current plan is to generate source
notes for the decompiler to use.

Comment 3

[Brendan Eich [:brendan]]

Or not, where bytecode is unambiguous.  We're full up on srcnotes, so you'd have
to use an existing one that can unambiguous annotate an overloaded bytecode such
as JSOP_ADD.

/be

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> Or not, where bytecode is unambiguous.  We're full up on srcnotes, so you'd have
> to use an existing one that can unambiguousLY annotate an overloaded bytecode
> such as JSOP_ADD.

In the JSOP_ADD example, SRC_HIDDEN might do well.

Even better would be if the decompiler could tell from some local state that the
JSOP_ADD was for an XML literal.  Worth looking into.

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Without an extra opcode, I don't think that's possible, unfortunately. The first
opcode the decompiler sees is the 'string' opcode and there are no other hints
that we're in XML until we see the 'toxml'. Is there some other source of info
that I'm missing?

Comment 6

[Brendan Eich [:brendan]]

Sure, there's always a way.  In the decompiler's JSOP_STRING case, you could
peek ahead (if there is another bytecode after the string op and its immediate)
for the JSOP_TOXML op, or another XML-only op that might be adjacent (followed
at some point by JSOP_ADD).

/be

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

My concern with this is that the 'toxml' opcode might be very far down the tree
(consider (function f(k) { return <{k}>{k}</{k}>; })) and it feels fragile to
have to scan forward (e.g., figuring out (function f() { return "haha" + <xml/>;
}) seems like it might be an ambiguous case, though I'm not quite certain yet).

Comment 8

[Brendan Eich [:brendan]]

mrbkap and I talked more and poked around; new specialized no-ops are the best
way to fix this, and he's on the case.

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: partial patch

This isn't ready. I haven't touched jsinterp.c, and <k k={k}/> asserts. Feel
free to make comments, but I'll work on it more over the weekend.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: full patch, v1

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: testcases

Here are some testcases. Note that they show another bug (that I'll file in a
second) where <a b={c}> is output as <a b="{c}">, note the quotes.

Comment 12

[Mike Shaver (:shaver emeritus)]

Comment on attachment 190417 [details] [diff] [review]
full patch, v1

r=shaver

I don't think the ETAGO case can happen, but I'm not confident enough to ask
you to turn it into an assertion.  Nice work.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

...And for the record, I filed bug 302097 on the attribute quoting problem.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 190417 [details] [diff] [review]
full patch, v1

>+    JSOp op, nextop;
[snip]
>+        nextop = pn->pn_head->pn_type;

That's not a JSOp right-hand side, it's of JSTokenType.

Also, is it worth another frame slot for a temporary?  js_EmitTree is
recursive, and a bit portly -- let's put it on a diet.	The following:

>+        if (nextop != TOK_XMLPTAGC &&
>+            nextop != TOK_XMLSTAGO &&
>+            nextop != TOK_XMLETAGO && /* XXX can this happen? */

A switch on pn->pn_head->pn_type would be better.  The cases enumerated would
break (the ETAGO case should JS_ASSERT(0); and fall through into the others'
break;) with the default: case handling the JSOP_STARTXML code generation.

JS_ASSERT before the if.

>+        if (pn2->pn_type == TOK_LC && js_Emit1(cx, cg, JSOP_JSEXPR) < 0)
>+            return JS_FALSE;

Nice layout -- one if, && well-used, < 0 tested correctly in second clause.

>         for (pn2 = pn2->pn_next, i = 0; pn2; pn2 = pn2->pn_next, i++) {
>+            if (pn2->pn_type == TOK_LC) {
>+                if (!js_Emit1(cx, cg, JSOP_JSEXPR) < 0)
>+                    return JS_FALSE;
>+            }

Oops.  ! in front of js_Emit1 call, and if-if instead of if-&&.

Please fix the ! bug and the nits ASAP, r+a=me.

/be

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #15)
> >         for (pn2 = pn2->pn_next, i = 0; pn2; pn2 = pn2->pn_next, i++) {
> >+            if (pn2->pn_type == TOK_LC) {
> >+                if (!js_Emit1(cx, cg, JSOP_JSEXPR) < 0)
> >+                    return JS_FALSE;
> >+            }
> 
> Oops.  ! in front of js_Emit1 call, and if-if instead of if-&&.

Oh, and always brace then and else clauses if any of the if condition, then
clause, and else clause if present, are multiline (even if only due to a comment).

/be

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: followup patch (checked in)

Comment 18

[Bob Clary [:bc] (inactive)]

Checking in regress-301692.js;
/cvsroot/mozilla/js/tests/e4x/Regress/regress-301692.js,v  <--  regress-301692.js
initial revision: 1.1