596502 - mozIJSSubScriptLoader fails to load JS file if it contains CDATA section

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alice0775 White]

Build Identifier: http://hg.mozilla.org/mozilla-central/rev/1cc1070b27ff
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100914 Firefox/4.0b7pre ID:20100914201916

After landing changesets 8f82aa15328c TM Merge,
mozIJSSubScriptLoader fails to load JS file, if it contains CDATA section.

I am not sure this is regression or by design.

Reproducible: Always

1. JavaScript contains  CDATA section as follows:
var text = <![CDATA[
aaaaa 
bbbb 
]]>.toString();

Actual Result:
Error : syntax error


Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/8f4cd741f348
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100914 Firefox/4.0b7pre ID:20100914195811
Fails:
http://hg.mozilla.org/mozilla-central/rev/1cc1070b27ff
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100914 Firefox/4.0b7pre ID:20100914201916
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8f4cd741f348&tochange=1cc1070b27ff

Comment 2

[Alice0775 White]

Attachment: test xpi

[STR]
1. Start Minefield with new profile and  -jsconsole
2. Install test xpi and restart to complete instalattion.
3. Open Error Console
4. Tool > alert popup bug596502

[Actual]
In Error Console:

Error: syntax error
Source file: chrome://bug596502/content/script.js
Line: 2, Column: 23
Source code:
   var bug596502_aaa = <![CDATA[ 

[Expected]
An alert box popup. and it contains as follows:

   bug596502
   test

Comment 3

[Alice0775 White]

Please reopen this If nobody can reproduce bug 596580

Comment 4

[ithinc]

This bug needs to be reopened.

Comment 6

[Brendan Eich [:brendan]]

This bug was dup'ed to an UNCONFIRMED bug, which I've confirmed. Should confirm when dup'ing, bugzilla won't do it for you.

This does seem to a dup, so I'm not sure why ithinc wrote comment 4. Whatever is broken with default version including XML or 'let' support, we can solve it in bug 596580 -- we just need a testcase that reproduces. In this light the summary of both bugs should be merged or generalized.

/be

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Oh, sorry, I didn't see comment 6. I reopened because the symptom is different (people looking for dupes won't find that other bug as summarized), and testcases/ability to reproduce were different, so "depends on" seemed more appropriate.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Note that bug 596915 has clearer str, and in particular talks about this being broken even if the version is set explicitly, I think.

Comment 9

[ithinc]

(In reply to comment #6)
> This bug was dup'ed to an UNCONFIRMED bug, which I've confirmed. Should confirm
> when dup'ing, bugzilla won't do it for you.
> 
> This does seem to a dup, so I'm not sure why ithinc wrote comment 4. Whatever
> is broken with default version including XML or 'let' support, we can solve it
> in bug 596580 -- we just need a testcase that reproduces. In this light the
> summary of both bugs should be merged or generalized.
> 
> /be

This bug is not related to "let", and unnecessarily connected to mozIJSSubScriptLoader. Removing these, it is not dup'ed with 596980 at all.

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Fix XML scanning.

Botched the JSOPTION_HAS_XML(cx) macro transition in http://hg.mozilla.org/tracemonkey/rev/33bf77bcf1a0

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 476476 [details] [diff] [review]
Fix XML scanning.

ithinc was right, different bug -- I was hoping for one bug, two symptoms :-/.

Didn't we used to test only the XML bit sync'ed to version from options? Is that no longer done? (I didn't have time to review the version changes you made just recently.)

/be

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #11)
> Didn't we used to test only the XML bit sync'ed to version from options?

Unfortunately no, that was where I tripped up! See http://hg.mozilla.org/tracemonkey/rev/33bf77bcf1a0#l17.36

http://hg.mozilla.org/tracemonkey/rev/0a3530e79cb7

Comment 13

[Kris Maglione [:kmag]]

Is the above fix going to be committed soon? If this isn't fixed before the 4.0 release, a lot of code is going to have to be changed, and stay changed for years to come.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Kris, this is fixed in the tracemonkey repository, and you can get builds with the fix now to test if you want.  It'll be fixed on mozilla-central the next time tracemonkey merges over, which will be significantly before the Firefox 4 release.

Comment 15

[Brendan Eich [:brendan]]

Have we so broken devs that people are braced for a bug like this shipping? It's not gonna go unfixed over my live body.

/be

Comment 16

[Kris Maglione [:kmag]]

Thanks, I'll try the tracemonkey repo. Sorry for the noise.Brendan, I've seen releases ship with some pretty annoying bugs, but I was mostly just harried about the last beta shipping without the above fix, and I've read bug reports for affected extensions by users who expect 4.0 to ship with this broken.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Have we so broken devs that people are braced for a bug like this shipping?

No, people just like to assume the worst.

Comment 18

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0a3530e79cb7