'Illegal character' error

VERIFIED FIXED in mozilla1.0.1

Status

()

Core
JavaScript Engine
P1
normal
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: leveille, Assigned: brendan)

Tracking

({js1.5})

Trunk
mozilla1.0.1
x86
All
js1.5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

16 years ago
A line as this one in the head not work due to the http://www.xxx.com

<script type="text/javascript" 
src="http://www.xxx.com/moreconf/zzz.js"></script>

the src argument must be an uri (url) as defined by the standard (see
http://www.ietf.org/rfc/rfc2396.txt
http://www.w3.org/TR/REC-html40/interact/scripts.html
)
Is there a site or testcase that shows the problem?  I have been unable to 
reproduce it....
Mail from reporter:

After more investigations it appears that I have not correctly qualified
the bug. In fact, the URL work fine but if the JavaScript file is
terminated by the character hexa 00 (as in the attachment), the last
line is understand as a JavaScript syntax error. It should be fine to
arrange this.

------------------------

Over to JS engine based on that description
Assignee: Matti → rogerl
Component: Browser-General → JavaScript Engine
QA Contact: imajes-qa → pschwartau

Comment 3

16 years ago
> (as in the attachment)

Boris, was there an attachment in the email from the reporter? 
I can't see an example here that we can check -
Assignee: rogerl → khanson
Created attachment 86250 [details]
The zipfile attached to the mail

I have no good way to unzip this and attach the individual files at the moment
(without munging the file contents).

Comment 5

16 years ago
Confirming report. When I unzip the file and load 'xx.htm' in Mozilla
trunk 2002053008 on WinNT, I get this error in the JavaScript Console:

                    Error: illegal character

And when I load 'yy.js' in the standalone JS shell on WinNT, I crash
with this stack:


NTDLL! 77f762e8()
GetChar(JSTokenStream * 0x002ff838) line 295 + 38 bytes
js_GetToken(JSContext * 0x00301d60, JSTokenStream * 0x002ff838) line 736 + 9 
bytes
js_MatchToken(JSContext * 0x00301d60, JSTokenStream * 0x002ff838, int 2) line 
1281 + 13 bytes
Statement(JSContext * 0x00301d60, JSTokenStream * 0x002ff838, JSTreeContext * 
0x0012edb4) line 1850 + 15 bytes
Statements(JSContext * 0x00301d60, JSTokenStream * 0x002ff838, JSTreeContext * 
0x0012edb4) line 886 + 17 bytes
js_CompileTokenStream(JSContext * 0x00301d60, JSObject * 0x002fb340, 
JSTokenStream * 0x002ff838, JSCodeGenerator * 0x0012edb4) line 392 + 17 bytes
CompileTokenStream(JSContext * 0x00301d60, JSObject * 0x002fb340, JSTokenStream 
* 0x002ff838, void * 0x00301de0, int * 0x00000000) line 2846 + 24 bytes
JS_CompileFileHandleForPrincipals(JSContext * 0x00301d60, JSObject * 0x002fb340, 
const char * 0x00300028, _iobuf * 0x7803ac88, JSPrincipals * 0x00000000) line 
3026 + 23 bytes
JS_CompileFileHandle(JSContext * 0x00301d60, JSObject * 0x002fb340, const char * 
0x00300028, _iobuf * 0x7803ac88) line 3003 + 23 bytes
Process(JSContext * 0x00301d60, JSObject * 0x002fb340, char * 0x00300028) line 
328 + 25 bytes
ProcessArgs(JSContext * 0x00301d60, JSObject * 0x002fb340, char * * 0x00300014, 
int 2) line 481 + 17 bytes
main(int 2, char * * 0x00300014) line 2129 + 21 bytes
JS! mainCRTStartup + 227 bytes
KERNEL32! 77f1b9ea()


I also see this assert in the JS shell:

          Assertion failure: len > 0, at jsscan.c:295


However, when I try to look at the file yy.js, I'm not sure what I see
is a valid file. cc'ing Brendan, Mike to see if this bug is valid -
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: text/javascript src not work with a full url → 'Illegal' character error

Updated

16 years ago
Keywords: crash
Summary: 'Illegal' character error → 'Illegal character' error
(Assignee)

Comment 6

16 years ago
This is not a crash bug, but the assertion in the debug build is telling us
there is a non-crash bug here -- the code in jsscan.c stumbled onto a bad
assumption, and is confused.  The assumption is that NUL characters won't occur
in the input buffer.  Patch coming up.

/be
Assignee: khanson → brendan
Keywords: crash → js1.5, mozilla1.0.1
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.0.1
(Assignee)

Comment 7

16 years ago
According to ECMA Chapter 7 (my reading), \u0000 is not legal in that position
in a valid program.  So the only bug here is the bogus assertion in the js
shell, and that's due to fgets's old-as-the-Unix-hills botched API, which does
not allow embedded NULs in strings.  I'll patch that, but this is so low priority.

/be
Priority: P2 → P4
(Assignee)

Comment 8

16 years ago
Created attachment 86529 [details] [diff] [review]
proposed fix

This may underperform compared to optimized stdio fgets implementations, but I
don't care.  Comments, reviews?  It tests with the same (now 18!?) failures
from the js/tests suite that I get without the patch.

/be

Comment 9

16 years ago
On Linux, I'm not getting 18 baseline failures, only 14.

One is my fault (need to update js1_5/Exceptions/errstack-001.js).

Plus: there are 5 new failures due to recent corrections in RegExp testcases.
These will disappear when the big RegExp patch goes in (bug 85721)

Plus: there are 2 failures due to recently-added tests:
      ecma_3/Function/15.3.4.3-1.js    (for bug 145779)
      js1_5/Object/regress-137000.js   (for bug 137000)



Here's my list of baseline failures on Linux:

# Retest List, smdebug, generated Wed Jun  5 19:05:59 2002.
# Original test base was: All tests.
# 1035 of 1035 test(s) were completed, 14 failures reported.

ecma_2/String/match-002.js          <--- new RegExp expectations
ecma_3/Function/15.3.4.3-1.js       <--- recently added test
ecma_3/RegExp/regress-123437.js
ecma_3/RegExp/regress-31316.js      <--- new RegExp expectations 
ecma_3/RegExp/regress-57572.js      <--- new RegExp expectations
ecma_3/RegExp/regress-85721.js
ecma_3/RegExp/regress-87231.js      <--- new RegExp expectations
js1_2/regexp/vertical_bar.js        <--- new RegExp expectations
js1_5/Array/regress-101964.js
js1_5/Exceptions/errstack-001.js    <--- my fault; will correct this one
js1_5/Object/regress-90596-001.js
js1_5/Object/regress-90596-002.js
js1_5/Object/regress-90596-003.js
js1_5/Object/regress-137000.js      <--- recently added test



The 6 remaining failures are long-standing issues -
(Assignee)

Comment 10

16 years ago
Ah, I was testing in a tree with another change that broke some tests.  Never
mind.  But I still want to fix the assertion, using my_fgets to work around the
ancient Unix stdio API botch that is fgets.  Roger or Kenton, can you r= this
bug's proposed fix patch?  Thanks,

/be
Comment on attachment 86529 [details] [diff] [review]
proposed fix

sr=shaver, thanks for the comments.
Attachment #86529 - Flags: superreview+

Comment 12

16 years ago
Comment on attachment 86529 [details] [diff] [review]
proposed fix

r=rogerl
Attachment #86529 - Flags: review+
(Assignee)

Comment 13

16 years ago
Waiting for 1.1beta trunk to open, then going for drivers approval for 1.0
branch checkin.

/be
Priority: P4 → P1
(Assignee)

Comment 14

16 years ago
Fix is in the trunk.

/be

Updated

16 years ago
Blocks: 149801

Comment 15

16 years ago
Phil, can you verify this fix on the trunk? brendan's_fgets(...) looks fine to
me, but, some 3rd party verification would be nice.

Comment 16

16 years ago
I can confirm that on the trunk, the fix is performing the way Brendan
outlined in Comment #7. That is, using current Mozilla trunk builds
on the "xx.htm" testcase above, a SyntaxError is reported per ECMA:


                  SyntaxError: illegal character


And - I no longer crash in the debug JS shell when I load "yy.js" 
as above, but receive this error instead. The optimized JS shell
also behaves this way (and always did, IIRC).

Comment 17

16 years ago
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+

Updated

16 years ago
Attachment #86529 - Flags: approval+
(Assignee)

Comment 18

16 years ago
Fixed in the 1.0 branch too.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Keywords: mozilla1.0.1+ → fixed1.0.1
Resolution: --- → FIXED

Comment 19

16 years ago
Patch verified on the 1.0 branch as well -
Status: RESOLVED → VERIFIED
Keywords: verified1.0.1
You need to log in before you can comment on or make changes to this bug.