312064 - E4X - XML Initializers with CDATA sections

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Bob Clary [:bc] (inactive)]

Branch and Trunk builds from this morning:

    Testcase e4x/Expressions/11.4.1-02.js failed Bug Number 257679
    [ Top of Page ]
    STATUS: 11.1.4 - XML Initializer
    STATUS: XML Initializer should accept single CDATA Section
    Failure messages were:
    FAILED!: Section 1 of test -
    FAILED!: Expected value:
    FAILED!: processing-instruction
    FAILED!: Actual value:
    FAILED!: text
    FAILED!:

    Testcase e4x/Expressions/11.4.1-05.js failed Bug Number 311157
    [ Top of Page ]
    Expected exit code 0, got 3
    Testcase terminated with signal 0
    Complete testcase output was:
    STATUS: Parsing - Comment hiding parsing/scanning
    BUGNUMBER: 311157
    STATUS: Comment-hiding compromise left E4X parsing/scanning inconsistent
    ./e4x/Expressions/11.4.1-05.js:48: SyntaxError: illegal XML character:
    ./e4x/Expressions/11.4.1-05.js:48: <parent xmlns=''><bye> <!CDATA[ duh ]]>
    ./e4x/Expressions/11.4.1-05.js:48: ..............................^

Comment 1

[Brendan Eich [:brendan]]

Looks like you're missing a [ before the C in that CDATA section.

/be

Comment 2

[Bob Clary [:bc] (inactive)]

look at the test source:

11.4.1-02: Ok, this is invalid. I forgot to change the expected value.


11.4.1-05: This is not invalid. Look at the test source.

var x = <bye> <![CDATA[ duh ]]>
    there </bye>;

Comment 3

[Brendan Eich [:brendan]]

Oops!  Sorry.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix

Argh, this is a stupid one-char patch to fix the bug.  Should go into 1.8
branch ASAP, it's trivial and obviously correct.

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 199263 [details] [diff] [review]
fix

mrbkap is looking over my shoulder and says "r=me" with a slight tone of
disgusted bemusement.

/be

Comment 6

[Brendan Eich [:brendan]]

Fixed in both places.  Sigh.

/be

Comment 7

[Bob Clary [:bc] (inactive)]

x = <bye> <![CDATA[ duh ]]>
    there </bye>

'[' + x.toString().replace(/ /g, '_') + ']' gives
[duhthere]

Is this correct?

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> x = <bye> <![CDATA[ duh ]]>
>     there </bye>
> 
> '[' + x.toString().replace(/ /g, '_') + ']' gives
> [duhthere]
> 
> Is this correct?

Yes.  Set XML.ignoreWhitespace = false if you want to preserve insignificant
whitespace.  Also, x.toXMLString() gives the complete serialization, while
x.toString() is more compressed for readability, or something.

/be

Comment 9

[Bob Clary [:bc] (inactive)]

(In reply to comment #8)

Ok, but the difference in behavior between 1) unadorned duh, 2) commented duh
and 3) cdata duh is bizarre. I'll fix the test and check it in later today.

Comment 10

[Brendan Eich [:brendan]]

People should double-check Rhino (the actual gold standard, generally) and
ECMA-357 (a buggy recoding of Java code for Rhino into spec-language).

/be

Comment 11

[Bob Clary [:bc] (inactive)]

(In reply to comment #10)
> People should double-check Rhino (the actual gold standard, generally) and

I will start doing that.

> ECMA-357 (a buggy recoding of Java code for Rhino into spec-language).

I did, and "10.1.1 ToString Applied to the XML Type" said nothing about
XML.ignoreWhitespace. Personally, I think ECMA 357 and E4X as a waste.

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> I did, and "10.1.1 ToString Applied to the XML Type" said nothing about
> XML.ignoreWhitespace.

> Personally, I think ECMA 357 and E4X as a waste.

Think *of* them as a waste?  We shall see what users think.

The basic ideas seem sound (XML literals, built-in query operators, etc.).

My name is listed on ECMA-357 because mozilla.org joined ECMA just before it was
finished, and I helped compile errata while implementing and debugging.  Turns
out Macromedia was going through the same exercise.  The bulk of the errata are
not fixed in ECMA-357, but we have tried to fix as many as we can for the ISO
version that will become ECMA-357 Edition 3.

The moral here is to avoid standardizing based only on one implementation.  Or
to say it another way, don't standardize a spec derived from one implementation
without any other implementations being developed and then tested for usability
and interoperation, feeding errata and design flaws back into the spec draft.

I won't participate in any such exercise; with ECMA-357, the horse was out of
the barn by the time I showed up.

/be

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (In reply to comment #11)
> > I did, and "10.1.1 ToString Applied to the XML Type" said nothing about
> > XML.ignoreWhitespace.

I forgot to reply to this part: XML.ignoreWhitespace affects input, not output,
so you shouldn't be looking at 10.1.1 or any To*String sub-spec.  Rather see
ECMA-357 10.3.2.1 MapInfoItemToXML, 2(c), and 13.4.3.4 XML.ignoreWhitespace.

/be

Comment 14

[Aiko]

There is still a small problem with CDATA sections. Anything but the sequence ]]> should be allowed but the engine generates a syntax error if it encounters the sequence ]]

For example, |var x = <x><![CDATA[ ]] ]]></x>| should be ok and produce the same result as |var x = <x> ]] </x>|.

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #14)
> There is still a small problem with CDATA sections. Anything but the sequence
> ]]> should be allowed but the engine generates a syntax error if it encounters
> the sequence ]]
> 
> For example, |var x = <x><![CDATA[ ]] ]]></x>| should be ok and produce the
> same result as |var x = <x> ]] </x>|.

Could you please file a new bug on this?  Thanks,

/be

Comment 16

[Aiko]

(In reply to comment #15)
> Could you please file a new bug on this?  Thanks,

Sure, please see bug 313929.