incorrect parsing of javascripts

RESOLVED FIXED

Status

()

Core
HTML: Parser
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Matej Zagiba, Assigned: Brian Crowder)

Tracking

({verified1.9.0.6})

1.9.0 Branch
verified1.9.0.6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: 1.9.0-only)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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).
(Reporter)

Comment 1

10 years ago
Created attachment 350303 [details]
javascript which is badly parsed
(Reporter)

Comment 2

10 years ago
Created attachment 350304 [details]
slightly modified javascript parsed correctly

Comment 3

10 years ago
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?
Summary: incorect parsing of javascripts → incorrect parsing of javascripts
Brian, is this related to the stuff you did with BOM-stripping?
(Assignee)

Comment 5

10 years ago
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.
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
This needs to be confirmed and, if real, investigated.

/be
Flags: blocking1.9.1?
(Reporter)

Comment 8

10 years ago
Created attachment 351135 [details]
js included in html files
(Reporter)

Comment 9

10 years ago
Created attachment 351136 [details]
badly parsed js (script src pointing to bugzilla)

as requested by comment #6 script src now points to bugzilla
Attachment #350303 - Attachment is obsolete: true
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
(Assignee)

Comment 11

10 years ago
Taking this, since the behavior is definitely different in 3.0.4 than it is on the trunk.
Assignee: general → crowder
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
> 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].
(Reporter)

Comment 13

10 years ago
(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
MZ: thanks, that is what crowder said too over IRC.

Pinging mrbkap since document.write is involved.

Brian, any news debugging or bisecting?

/be
(Assignee)

Comment 15

10 years ago
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.
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]...
(Assignee)

Comment 17

10 years ago
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.
(Assignee)

Comment 18

10 years ago
I suspect the patch for bug 449712 addresses this issue; I'll experiment with it on my 3.0.4 build.  More shortly.
(Assignee)

Comment 19

10 years ago
Created attachment 351953 [details] [diff] [review]
backport of patch from bug 449712
Attachment #351953 - Flags: superreview?(bzbarsky)
Attachment #351953 - Flags: review?(mrbkap)
(Assignee)

Comment 20

10 years ago
This bug does not exist on the trunk, anymore.
Assignee: crowder → nobody
Severity: major → normal
Component: JavaScript Engine → HTML: Parser
Flags: blocking1.9.1? → blocking1.9.0.5?
QA Contact: general → parser
Hardware: PC → All
Version: unspecified → 1.9.0 Branch
Comment on attachment 351953 [details] [diff] [review]
backport of patch from bug 449712

Wow, I'd totally forgotten about this patch.
Attachment #351953 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

10 years ago
Assignee: nobody → crowder
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?
Attachment #351953 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 23

10 years ago
No, that bug didn't really have a great regression test; I'll try to cook something up.
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
Flags: blocking1.9.0.5?
Attachment #351953 - Flags: approval1.9.0.6?
(Assignee)

Comment 25

10 years ago
approval ping?
Depends on: 449712
Whiteboard: 1.9.0-only
Attachment #351953 - Flags: approval1.9.0.6? → approval1.9.0.6+
Comment on attachment 351953 [details] [diff] [review]
backport of patch from bug 449712

Approved for 1.9.0.6, a=dveditz for release-drivers.
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
(Assignee)

Updated

9 years ago
Keywords: checkin-needed → fixed1.9.0.6
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.
Keywords: fixed1.9.0.6 → verified1.9.0.6
(Assignee)

Comment 28

9 years ago
Was fixed ages ago, sorry for not closing.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.