450633 - "script stack space quota is exhausted" exception in JSON.jsm when calling SessionStore API for sessions with a large amount of data

Status: RESOLVED WONTFIX

(Core :: XPConnect, defect)

Description

[Michael Kraft [:morac]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

If the current session state contains a very large amount of data, then any call to one of the SessionStore API functions that calls the _toJSONString() will result in a "script stack space quota is exhausted" exception being thrown on line 176 in JSON.jsm.

The exception occurs because the javascript "test" function will fail in Firefox 3.0.x when run on strings greater than approximate 1.2 million characters in length.  See bug 420869 for more details about RegExp functions and the stack space quota.

Reproducible: Always

Steps to Reproduce:
The easiest way to generate a large amount of session data is as follows:
1. Set browser.sessionstore.postdata to -1 
2. Upload a 1 MB or greater file using a HTML form (briefcase.yahoo.com works well)
3. In the error console type:  Components.classes["@mozilla.org/browser/sessionstore;1"].getService(Components.interfaces.nsISessionStore).getBrowserState()
Actual Results:  
Error: script stack space quota is exhausted
Source File: file:///C:/Program%20Files/Mozilla%20Firefox/modules/JSON.jsm
Line: 176

Error: uncaught exception: [Exception... "'[JavaScript Error: "script stack space quota is exhausted" {file: "file:///C:/Program%20Files/Mozilla%20Firefox/modules/JSON.jsm" line: 176}]' when calling method: [nsISessionStore::getBrowserState]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: javascript:%20Components.classes["@mozilla.org/browser/sessionstore;1"].getService(Components.interfaces.nsISessionStore).getBrowserState() :: <TOP_LEVEL> :: line 1"  data: yes]

Expected Results:  
No exception should be thrown.

The best method I can think of to fix this would be to break up large strings into multiple smaller ones (less 1,000,000 characters should work), then do the replace and test functions on each of the smaller strings.

The other choice is to just add a try catch around the the test() and replace() function calls in JSON.jsm line 176.  This could result in unsafe JSON strings though.

See also http://forums.mozillazine.org/viewtopic.php?p=4168125#p4168125

Comment 1

[Simon Bünzli]

Attachment: split the string in JSON.jsm when it's too large (backed out)

Not sure if this is the right thing to do. I'd like to get this patch on the 1.9.0 branch as well, though, so as not to unnecessarily cripple unaware extensions.

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

If the script stack quota is exhausted, Spidermonkey has been _told_ by the application (Gecko here) to not permit more stack allocation.  This isn't a Spidermonkey bug, though it might be either a session-store bug (don't try to process the world at once) or a Gecko bug (allow more stack use, if we can know it's safe to do so).

You seem to agree, since you're not patching the JS engine?

Comment 3

[Simon Bünzli]

Allowing more stack use would IMHO just postpone the issue as there could always be a bigger JSON string to parse (by SessionStore or any other caller) - so patching JSON.jsm looks like the right thing to do.

If this is the wrong component, where should this bug live, then (as JSON.jsm resides under /js)?

Comment 4

[Brendan Eich [:brendan]]

Simon, I'm giving you this bug since you are kindly patching it. It's an xpconnect bug, for better or worse, since xpconnect is (mis-)located under js/src but not part of the core JS engine. Thanks,

/be

Comment 5

[Simon Bünzli]

Comment on attachment 333803 [details] [diff] [review]
split the string in JSON.jsm when it's too large (backed out)

Thanks for the info, Brendan. Over to Mike, then.

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 333803 [details] [diff] [review]
split the string in JSON.jsm when it's too large (backed out)

I'm not sure why this is in XPConnect as well, but my usual plan of deflecting to the original author of the code won't work in this case, so I'm going to try our other JSON expert here.

(If it were xpconnect code proper, I'd not have expected sspitzer's review to have carried it into the tree the first time. :) )

Comment 7

[Simon Bünzli]

Thanks for the reviews.

Comment 8

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/38f03dcc5e22

Comment 9

[Simon Bünzli]

Comment on attachment 333803 [details] [diff] [review]
split the string in JSON.jsm when it's too large (backed out)

Drivers: This bug breaks session managing extensions when they're most needed: when the session is big and really does need managing.

This patch is of low overall impact, only affecting consumers of JSON.jsm, and thus of low risk. Tests are included and it's been baking on Trunk since 9/02.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 333803 [details] [diff] [review]
split the string in JSON.jsm when it's too large (backed out)

Approved for 1.9.0.4, a=dveditz for release-drivers

Comment 11

[Reed Loden [:reed]]

CVS HEAD:

Checking in js/src/xpconnect/loader/JSON.jsm;
/cvsroot/mozilla/js/src/xpconnect/loader/JSON.jsm,v  <--  JSON.jsm
new revision: 1.4; previous revision: 1.3
done
Checking in js/src/xpconnect/tests/unit/test_json.js;
/cvsroot/mozilla/js/src/xpconnect/tests/unit/test_json.js,v  <--  test_json.js
new revision: 1.4; previous revision: 1.3
done

Comment 12

[Simon Bünzli]

To my embarrassment, this patch can lead to a significant performance issue and even broken behavior (as reported in bug 461048) due to the fact that it might split strings in the middle of an escaped character (e.g. the JSON representation of |["\""]| might get split in halves, leading to |["\| and |""]| being checked which will lead to the rejection of a perfectly valid JSON encoded string).

I'd have a patch for fixing this in attachment #344750 [details] [diff] [review], but instead of risking further (even more subtle) breakage, I'd rather back this patch out, leave the bug on the 1.9.0 branch and fix bug 407110 and remove JSON.jsm from Trunk to get rid of this issue.

Extension authors being hit by this bug can always fix the issue for themselves by importing JSON.jsm and then setting JSON.isMostlyHarmless to the function from attachment #344750 [details] [diff] [review] before using the SessionStore API.

Sorry for the hassle!

Comment 13

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/cda625bb27d4

mozilla/js/src/xpconnect/loader/JSON.jsm 	1.5
mozilla/js/src/xpconnect/tests/unit/test_json.js 	1.5

Comment 14

[Samuel Sidler (old account; do not CC)]

Comment on attachment 333803 [details] [diff] [review]
split the string in JSON.jsm when it's too large (backed out)

Minusing for now. We'll want a rolled up patch when this is fixed.

Comment 15

[Simon Bünzli]

(In reply to comment #14)
> Minusing for now. We'll want a rolled up patch when this is fixed.

Do we really want a patch for this at all, considering the stricter branch rules and the fact that a work-around exists?

Comment 16

[Samuel Sidler (old account; do not CC)]

You're right. We don't want this and it doesn't block.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Simon, does it need a dev-doc update to inform extension developers?

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Simon, can you please give an update about the state of this bug?

Comment 20

[Simon Bünzli]

This bug is INVALID on Trunk (JSON.jsm was removed in bug 462774) and WONTFIX on the 1.9.0 branch (see comment #16).

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #20)
> This bug is INVALID on Trunk (JSON.jsm was removed in bug 462774) and WONTFIX
> on the 1.9.0 branch (see comment #16).

The version field is set to 1.9.0 branch. So lets mark it as WONTFIX.

Comment 22

[Eric Shepherd [:sheppy]]

Updated:

https://developer.mozilla.org/En/Using_native_JSON

Comment 23

[Dão Gottwald [:dao]]

(In reply to comment #22)
> Updated:
> 
> https://developer.mozilla.org/En/Using_native_JSON

That article doesn't seem to be about the JSON module.

Comment 24

[Eric Shepherd [:sheppy]]

OK, we don't have docs for this code module, which has been deprecated anyway. I've yanked the stuff out of that other article and am debating whether or not this justifies actually being written about now.

Comment 25

[Dão Gottwald [:dao]]

The code module hasn't only been deprecated but removed. I don't think anything needs to be documented here.