Rhino's new JSON.parse breaks on trailing whitespace

RESOLVED FIXED

Status

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: mozilla, Unassigned)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.13) Gecko/2009080315 Ubuntu/9.04 (jaunty) Firefox/3.0.13 FirePHP/0.3
Build Identifier: Rhino 1.7 release 3 PRERELEASE 2009 08 05

The JSON.parse in CVS breaks when a JSON string has trailing whitespace.

Reproducible: Always

Steps to Reproduce:
1. Open up the javascript shell
2. Type in `JSON.parse("{} ");
3. Type in `JSON.parse("{}\n");
Actual Results:  
Rhino throws a "SyntaxError: Expected end of stream at char 2" indicating JSON.parse considers the whitespace after the last } to be invalid JSON.

Expected Results:  
Both JSON.parse calls should return a new empty object.

SpiderMonkey's JSON.parse handles trailing whitespace fine. Also I don't believe trailing whitespace is supposed to be considered illegal. After all whitespace doesn't break things anywhere else in a JSON string, why should trailing whitespace be any different?

This causes issues when reading JSON from files. On POSIX systems files normally end in a newline and most reading methods end up reading in a string with a trailing newline, thus if you try to read a file then JSON.parse it using Rhino's new JSON system you'll get an unexpected (and hard to understand) error.

Comment 1

9 years ago
Thanks for the report, JSON source is now trimmed before parsing.

Checking in src/org/mozilla/javascript/json/JsonParser.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/json/JsonParser.java,v  <--  JsonParser.java
new revision: 1.4; previous revision: 1.3
Status: UNCONFIRMED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Reporter)

Comment 2

9 years ago
Great, I'll update rhino in MonkeyScript and pull out my workaround (trimming it with .trim() first) once the commit makes it's way into the git-mirror.
(Reporter)

Comment 3

9 years ago
This bug looks like it has cropped up again and json ending with a \n is throwing errors again.
Status: RESOLVED → UNCONFIRMED
Resolution: FIXED → ---

Comment 4

9 years ago
Norris converted it back with the following commit message:

Fix regression in unit test.
Patch from Raphael Speyer. 

I don't think it makes much sense, but I trust Raphael on ES5 issues (and haven't actually made the effort to read the spec).

Comment 5

9 years ago
Created attachment 415249 [details] [diff] [review]
Ignore trailing whitespace when parsing JSON

Sorry, only just saw this bug now. As far as I can tell, whitespace at the beginning and end should be ignored. Paragraph 2 of 5.1.2 seems to imply that, and that's what other implementations do. 

I didn't trim so that the character offset of errors could still be reported correctly.

Comment 6

9 years ago
Created attachment 415299 [details] [diff] [review]
made consumption of whitespace a little more robust
Attachment #415249 - Attachment is obsolete: true

Comment 7

9 years ago
Thanks!

Checking in testsrc/org/mozilla/javascript/tests/json/JsonParserTest.java;
/cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/json/JsonParserTest.java,v  <--  JsonParserTest.java
new revision: 1.3; previous revision: 1.2
done
Checking in src/org/mozilla/javascript/json/JsonParser.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/json/JsonParser.java,v  <--  JsonParser.java
new revision: 1.6; previous revision: 1.5
done
Status: UNCONFIRMED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Comment 8

9 years ago
Thanks for setting this straight, Raphael and Norris! After you undid my simple trim() patch, I wrongly assumed that ES5 would not allow leading/trailing whitespace. I never actually checked with the spec, although I definitely should have.
You need to log in before you can comment on or make changes to this bug.