466957 - incorrect parsing of javascripts

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Matej Zagiba]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.3) Gecko/2008092814 Iceweasel/3.0.4 (Debian-3.0.4-1)
Build Identifier: 

Javascripts generated by certain Allied Telesys switches are incorectly parsed by javascript engine. As a result, js functions are not defined and not executed.

Two sample files are attached.
- js_bad.html is incorectly parsed
- js_good.html is parsed correctly

only diference between them is one additional newline inserted on line 697 in js_good.html. in fact, this newline can be inserted on any line before.

Problem is reproducible in linux (debian lenny) and windows, firefox 3.0.3, 3.0.4 are affected. firefox 2.0.0.18 is not affected.

Reproducible: Always

Steps to Reproduce:
1. open attached file js_bad.html

Actual Results:  
page is rendered incorectly, js functions are not executed, errors in error console:
1. unterminated string literal (line 701)
2. writeVlanTitle is not defined (line 1279)
3. writeVlan is not defined (line 1378)


Expected Results:  
page should be rendered as js_good.html is.

This is regresion and should be fixed ASAP.
It could affect web usability (JS, AJAX).

Comment 1

[Matej Zagiba]

Attachment: javascript which is badly parsed

Comment 2

[Matej Zagiba]

Attachment: slightly modified javascript parsed correctly

Comment 3

[Bob Clary [:bc] (inactive)]

Ok, I see the results described in comment 0 with granparadiso but not with minefield on mac os x. The source uses \ to escape newlines to prettify the source, e.g.

str = 'this is a \
string';

But looking in saved versions there are carriage returns and newlines.

mrbkap: sounds like a parser change to 1.9.0 that was reverted in 1.9.1. Thoughts?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Brian, is this related to the stuff you did with BOM-stripping?

Comment 5

[Brian Crowder]

The error that I get in my nightly for the "javascript which is badly parsed" attachment is "portIdToPort is not defined", which actually seems legit to me.

I don't see how the BOM lexing stuff could affect this, though, one way or the other.

Comment 6

[Brendan Eich [:brendan]]

That attached .html file does not stand alone. It references other files via script src= and the URLs don't resolve. Please fix so that the attachment refers to bugzilla attachment URLs -- you can attach the .js files first, then edit the .html file to use their full bugzilla URLs.

/be

Comment 7

[Brendan Eich [:brendan]]

This needs to be confirmed and, if real, investigated.

/be

Comment 8

[Matej Zagiba]

Attachment: js included in html files

Comment 9

[Matej Zagiba]

Attachment: badly parsed js (script src pointing to bugzilla)

as requested by comment #6 script src now points to bugzilla

Comment 10

[Brendan Eich [:brendan]]

Please test your attachment. It's still incomplete:

Error: unterminated string literal
Source File: https://bugzilla.mozilla.org/attachment.cgi?id=351136
Line: 701, Column: 22
Source Code:
 
Error: writeVlanTitle is not defined
Source File: https://bugzilla.mozilla.org/attachment.cgi?id=351136
Line: 1279

Error: writeVlan is not defined
Source File: https://bugzilla.mozilla.org/attachment.cgi?id=351136
Line: 1378

/be

Comment 11

[Brian Crowder]

Taking this, since the behavior is definitely different in 3.0.4 than it is on the trunk.

Comment 12

[WADA:World Anti-bad-Duping Agency]

> Error: unterminated string literal
> Source File: https://bugzilla.mozilla.org/attachment.cgi?id=351136
> Line: 701, Column: 22

This error occurs next part in HTML source ( [HT]=0x09, [CRLF]=0x0D0A ).
>(snip)
> 688  function writeOneVlan(vlanIndex) [CRLF]
> 689  {[CRLF]
> 690  [HT]var unit[CRLF]
> 691  [HT]var portId[CRLF]
> 692  [HT]memberFromString(vlanIndex)[CRLF]
> 693  [HT]var userPorNo[CRLF]
> 694  [CRLF]
> 695  [HT]document.writeln('<tr>')[CRLF]
> 696  [HT][HT][CRLF]
> 697  [HT]if (vlanIndex < gxVlanCount) [CRLF]
> 698  [HT][CRLF]
> 700  [HT][HT][HT][HT]document.writeln('\[CRLF]
> 701  [HT][HT][HT][HT][HT]<input\[CRLF]
>(snip)

Above part is split into 2 textnodes at line 700/701.
DOM Inspector displays textnode content as follows. (checked with Fx 3.0.4/Win)

(1) Last part of the_script_tag.childNodes[4] (5-th text node of the Script tag)
>(snip)
> function writeOneVlan(vlanIndex) 
> {
>   var unit
>   var portId
>   memberFromString(vlanIndex)
>   var userPorNo
>
>   document.writeln('<tr>')
>
>   if (vlanIndex < gxVlanCount) 
>   {
>       dcument.writeln('\
>                            <== Note: Null line exists here

(2) Top part of the_script_tag.childNodes[5] (6-th text node of the Script tag)
>                            <== Note: Null line exists here
>       <input\
>(snip)

It looks for me to be a result that document.writeln('\[CRLF] is split between [CR] and [LF].

Comment 13

[Matej Zagiba]

(In reply to comment #10)
> Please test your attachment. It's still incomplete:
> 
> Error: unterminated string literal
> Source File: https://bugzilla.mozilla.org/attachment.cgi?id=351136
> Line: 701, Column: 22
> Source Code:
> 
> Error: writeVlanTitle is not defined
> Source File: https://bugzilla.mozilla.org/attachment.cgi?id=351136
> Line: 1279
> 
> Error: writeVlan is not defined
> Source File: https://bugzilla.mozilla.org/attachment.cgi?id=351136
> Line: 1378
> 
> /be

well that is exactly the bug I reported (see comment #1 - actual results).
function writeVlanTitle is defined on line 818, 
function writeVlan is defined on line 1095

MZ

Comment 14

[Brendan Eich [:brendan]]

MZ: thanks, that is what crowder said too over IRC.

Pinging mrbkap since document.write is involved.

Brian, any news debugging or bisecting?

/be

Comment 15

[Brian Crowder]

brendan:  No, not a lot of progress, yet; working on other stuff, but it seems likely not to be a JS-engine bug as such.  More when I have it.

Comment 16

[WADA:World Anti-bad-Duping Agency]

Tested with locally saved version of "javascript which is badly parsed" attached to Comment #1(obsoleted), to avoid security error of locally saved version of "badly parsed js (script src pointing to bugzilla)".

(Original)
> 700  [HT][HT][HT][HT]document.writeln('\[CRLF]
(Case-1) One [HT] is removed
> 700  [HT][HT][HT]document.writeln('\[CRLF]
(Case-2) One space is added before document.write
> 700  [HT][HT][HT][HT] document.writeln('\[CRLF]

In both cases, original problem disappeared, and phenomenon was changed to next error only because portIdToPort is defined in external JS file.
> Error: portIdToPort is not defined
> Source File: file:///C:/wada/TEST/Bug-org/bug-466957/bad-00x.html
> Line: 721

I believe above test result explains following in Comment #0.  
> only diference between them is one additional newline inserted on line 697 in
js_good.html. 
> in fact, this newline can be inserted on any line before.
Case-1        : Split at just after [CRLF]
Original(bad) : Split between [CR] and [LF]
Case-2        : Split at just before [CRLF] == Split to writeln('\ and [CRLF]...
Original(good): Split to writeln(' and \[CRLF]...

Comment 17

[Brian Crowder]

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-08-19+03%3A44+%09+&enddate=2008-08-20+03%3A36 seems to reflect the range of time in which this bug was fixed.  Looking more closely now.

Comment 18

[Brian Crowder]

I suspect the patch for bug 449712 addresses this issue; I'll experiment with it on my 3.0.4 build.  More shortly.

Comment 19

[Brian Crowder]

Attachment: backport of patch from bug 449712

Comment 20

[Brian Crowder]

This bug does not exist on the trunk, anymore.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 351953 [details] [diff] [review]
backport of patch from bug 449712

Wow, I'd totally forgotten about this patch.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 351953 [details] [diff] [review]
backport of patch from bug 449712

sr=bzbarsky.  Good catch!

Did that bug have regression tests?  If so, can you land them on branch too?

Comment 23

[Brian Crowder]

No, that bug didn't really have a great regression test; I'll try to cook something up.

Comment 24

[Daniel Veditz [:dveditz]]

Doesn't meet the blocking criteria we'll look at a branch approval request. 1.9.0.5 is done, btw, so it's 1.9.0.6

Comment 25

[Brian Crowder]

approval ping?

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 351953 [details] [diff] [review]
backport of patch from bug 449712

Approved for 1.9.0.6, a=dveditz for release-drivers.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.6 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.6pre) Gecko/2009011304 GranParadiso/3.0.6pre using the working version of the attached test file.

Comment 28

[Brian Crowder]

Was fixed ages ago, sorry for not closing.