348836 - Error reported in long line can hang JavaScript console

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[[e-mail]]

If a JavaScript error occurs in a line of more than 253 characters then only a trailing portion of the source text is reported. Furthermore if the error position occurs before that portion then the reported error column becomes negative which translates to a very large PRUint32. The error console cannot cope as it tries to construct a string with that length. Test case coming up.

Reproduced on trunk and 1.8.1; probably affects 1.8.0.7 too.

Comment 1

[[e-mail]]

Attachment: Test case

Comment 2

[Martijn Wargers (dead)]

This regressed between 2004-11-25 and 2004-12-05, although I get a slow script warning with the 2004-12-05. With current builds, I don't get a slow script warning, I hang indefinetely.

Comment 3

[Daniel Veditz [:dveditz]]

Not gonna block 1.5.0.7 on this, but if you come up with a patch (this week!) we'll take a look

Comment 4

[Jesse Ruderman]

Dup of bug 348627?

Comment 5

[[e-mail]]

(In reply to comment #4)
>Dup of bug 348627?
No, this bug is about illegal column numbers, not about taking a long time to construct legally long strings, but because of this bug I can't test legally long strings to resolve that bug, thus I have marked this as blocking that.

Comment 6

[Mike Schroepfer]

Putting on the 1.8.1/FF2 radar...

Comment 7

[Mats Palmgren (inactive)]

Attachment: Some debug data + stack

We hang here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/jsarray.c&rev=3.95&root=/cvsroot&mark=586#582

length is 4294967057 (0xFFFFFF11) which is 0xFFFFFFFF - 238 and
is one less than the length of the number of digits in the
unterminated string.

I don't know anything about the JS engine, but this looks like
an easy kill for someone who do ;-)

Comment 8

[Mike Schroepfer]

Since this isn't a regression from 1.8.0/FF15 we are not going to block FF2 for this.   If we get a patch please nom for 1.8.1.1

Comment 9

[Jesse Ruderman]

CCing the reporter of bug 351657, which I think is a dup.

Comment 10

[Brian Crowder]

Attachment: wallpaper

Comment 11

[Brian Crowder]

Accidentally submitted before explaining:  This is a wallpaper patch for this bug.  The wallpaper probably isn't a horrible thing to keep in place, but there is also probably something wrongish happening further upstream, since somehow we have an index pointing at data before linepos.  I don't know enough yet about how parsing works to trace it back easily, and since this seems like an important bug, I thought I'd throw a patch up as soon as I had something.

Comment 12

[Brian Crowder]

Attachment: Better wallpaper

Better version of this, breaks out of the while loop upon finding the bounds issue, and is hopefully more readable.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 238739 [details] [diff] [review]
Better wallpaper

Followup bug cited by a /* FIXME: bug nnnnnn */ comment (could be a major comment and say a bit more about finding error tokens in userbuf if !ts->file).

Looks good otherwise.

/be

Comment 14

[Brian Crowder]

Attachment: fix with reference to new bug

Added a major FIXME comment referencing bug #352970

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 238814 [details] [diff] [review]
fix with reference to new bug

Being picky while I can ;-).

>+                /*
>+                 * FIXME: Probably what should be happening here instead is to
>+                 * find error-tokens in userbuf (if !ts->file).

I'd lose the "Probably" and the parens around if !ts->file, since we should be certain about the FIXME, and the !ts->file condition is not an aside but a prerequisite.

>  That should
>+                 * allow us to deliver a more helpful error message, which
>+                 * includes all or part of the bad string or bad token.  This
>+                 * code will yield something that looks very truncated.

s/very //

Thanks, r=me with those comment edits.

/be

Comment 16

[Brian Crowder]

Attachment: Better comment

Comment 17

[Brian Crowder]

Attachment: Ugh, better spelling

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 238818 [details] [diff] [review]
Ugh, better spelling

Thanks!  I'll land this right away.

/be

Comment 19

[Brendan Eich [:brendan]]

Checking in jsscan.c;
/cvsroot/mozilla/js/src/jsscan.c,v  <--  jsscan.c
new revision: 3.112; previous revision: 3.111
done

/be

Comment 20

[Brian Crowder]

Comment on attachment 238818 [details] [diff] [review]
Ugh, better spelling

This is a pretty safe fix that should at least prevent the Fx error-reporter from trying to build a gigantic (0xffffffxx or thereabouts, as an unsigned int) array of spaces for error printing.

Comment 21

[[e-mail]]

Out of interest I compared the behaviour of Mozilla 1.7.x, which while also mangling the error line does limit the error position to between 0 and 255.

Comment 22

[Mike Schroepfer]

Comment on attachment 238818 [details] [diff] [review]
Ugh, better spelling

a=schrep - approved to land on 1.8.1 branch before rc1.

Comment 23

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 238818 [details] [diff] [review]
Ugh, better spelling

approved for 1.8.0 branch, a=dveditz for drivers

Comment 25

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

mozilla/js/src/jsscan.c 	3.81.2.7.2.8

Comment 26

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.8pre) Gecko/20061020 Firefox/1.5.0.8pre, no hang with testcase and console shows: Error: unterminated string literal
Source File: https://bugzilla.mozilla.org/attachment.cgi?id=233985&action=view
Line: 5