Last Comment Bug 669938 - nsIJSON::decodeFromStream fails to parse files <4 bytes
: nsIJSON::decodeFromStream fails to parse files <4 bytes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-07 10:31 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2011-07-10 13:44 PDT (History)
4 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.38 KB, patch)
2011-07-07 11:43 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
jwalden+bmo: review+
Details | Diff | Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-07 10:31:54 PDT
This came up while trying to load a file containing just "{}".

nsJSONListener::ProcessBytes attempts to sniff a charset using the first 4 bytes, but fails to do that if there isn't 4 bytes to check. This means that it fails to find a charset, CheckCharset() fails, and the parse is aborted.

I imagine it can just fall back to UTF-8 for files <4 bytes, since you can't have valid UTF-16 JSON in less than 4 bytes. It looks like there's already special handling for streams that don't even fill up mSniffBuffer, so I think I can just add a check.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-07 11:43:17 PDT
Created attachment 544564 [details] [diff] [review]
patch

The invariant that's worth noting is that ProcessBytes is never called unless:
- mSniffBuffer has been filled with 4 bytes from the stream (due to checks in OnDataAvailable)
*OR*
- onStopRequest has fired (i.e. we finished reading a stream whose length is <4 bytes)

In the first case, this patch has no effect - we'll be able to sniff a charset in ProcessBytes as we did before, and won't fall into the fallback I'm adding since mSniffBuffer.Length() will be >= 4.

For the second case, we'll now assume UTF-8 (can't have valid UTF-16 JSON in <4 bytes!), and then only consume the bytes from mSniffBuffer. It makes no sense to pass mSniffBuffer as aBuffer, since ProcessBytes will unconditionally consume the mSniffBuffer bytes when called the first time (assuming mNeedsConverter, which is the only time mSniffBuffer is filled up).

This is a bit hard to follow, suggestions on ways to make this clearer are welcome!
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-08 14:56:23 PDT
(In reply to comment #0)
> since you can't have valid UTF-16 JSON in less than 4 bytes

True, if we're requiring any incoming UTF-16 to have leading bytes indicating endianness, and not just treating the absence as network byte order or something.  I guess we're doing that already, via the CheckCharset call.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-08 15:01:24 PDT
Comment on attachment 544564 [details] [diff] [review]
patch

Review of attachment 544564 [details] [diff] [review]:
-----------------------------------------------------------------

This is really pretty ugly.  But my interest in cleaning it up, when the contagion seems unlikely to spread beyond this code, is minimal.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-09 09:37:30 PDT
https://hg.mozilla.org/integration/fx-team/rev/0d6b31b198f0
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-10 13:44:21 PDT
http://hg.mozilla.org/mozilla-central/rev/0d6b31b198f0

Note You need to log in before you can comment on or make changes to this bug.