Closed Bug 643590 Opened 9 years ago Closed 9 years ago

nsJSON::DecodeFromStream should buffer up data, then parse the entire thing as JSON all at once


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)


(Whiteboard: fixed-in-tracemonkey)


(1 file)

Attached patch PatchSplinter Review
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.
Attachment #520772 - Flags: review?(jst)
Comment on attachment 520772 [details] [diff] [review]

- In nsJSONListener::ConsumeConverted():

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

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.

Attachment #520772 - Flags: review?(jst) → review+
I landed this in TM a bit ago, and it recently got merged to m-c:
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla5
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.

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).
You need to log in before you can comment on or make changes to this bug.