Last Comment Bug 643590 - nsJSON::DecodeFromStream should buffer up data, then parse the entire thing as JSON all at once
: nsJSON::DecodeFromStream should buffer up data, then parse the entire thing a...
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla5
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-21 15:01 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-04-14 08:53 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (3.84 KB, patch)
2011-03-21 15:01 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
jst: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-03-21 15:01:48 PDT
Created attachment 520772 [details] [diff] [review]
Patch

Callers don't generally pass in such a huge amount of data that the extra (temporal) memory hit is burdensome, and the new API being added in bug 643532 is a bit simpler to use.  Moreover, this simplification makes possible various JSON performance enhancements based on not needing to always have parser-saved state in case of buffer boundaries.

I'm way out of date on what the proper standard data structure is for an array of chars, but what I could find on MDN suggests nsTArray is the right container.

This depends on the API added in the patch in bug 643532.
Comment 1 Johnny Stenback (:jst, jst@mozilla.com) 2011-03-21 17:05:35 PDT
Comment on attachment 520772 [details] [diff] [review]
Patch

- In nsJSONListener::ConsumeConverted():

+  PRUnichar* endelems = mBufferedChars.AppendElements(unicharLength);
+  if (!endelems)
+    return NS_ERROR_OUT_OF_MEMORY;

I think you picked the right container for this data, but it is an infallible container, as in AppendElements will never fail, it'll abort() on oom. So no need to check here, or if you feel that's wrong, then we should change this to be a FallibleTArray<>. Seems to me that having this be an infallible container is ok though.

r=jst.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-13 14:59:59 PDT
I landed this in TM a bit ago, and it recently got merged to m-c:

http://hg.mozilla.org/mozilla-central/rev/7b977e10c721
Comment 3 Brendan Eich [:brendan] 2011-04-14 08:33:45 PDT
If unicharLength is content-determined, this should be a fallible allocation. Very large JSON exists and it can trigger OOM (at least on Windows) without the browser being fatally out of memory.

Please cite the followup bug here. Thanks.

/be
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-14 08:53:30 PDT
This is only tenuously determined by content.  decodeFromStream for this interface is only called from the search service, which appears to cache a JSON description of used search engines, and from the addon manager, which appears to cache a JSON description per extension.  Neither use seems interestingly content-controllable to me, nor is either likely to be very large unless someone's deliberately trying to mess with things here (in which case, they can knock themselves out).

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