Is there a site or testcase that shows the problem? I have been unable to reproduce it....
Assignee: Matti → rogerl
QA Contact: imajes-qa → pschwartau
> (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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: 'Illegal' character error → 'Illegal character' error
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
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.0.1
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
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
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/22.214.171.124-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/126.96.36.199-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 -
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 on attachment 86529 [details] [diff] [review] proposed fix r=rogerl
Attachment #86529 - Flags: review+
Waiting for 1.1beta trunk to open, then going for drivers approval for 1.0 branch checkin. /be
Priority: P4 → P1
Fix is in the trunk. /be
Phil, can you verify this fix on the trunk? brendan's_fgets(...) looks fine to me, but, some 3rd party verification would be nice.
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).
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+
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
Patch verified on the 1.0 branch as well -
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.