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

RESOLVED WONTFIX

Status

()

Core
XPConnect
RESOLVED WONTFIX
9 years ago
8 years ago

People

(Reporter: morac, Unassigned)

Tracking

1.9.0 Branch
mozilla1.9.1b1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.0.4 -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
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

Updated

9 years ago
Assignee: nobody → general
Status: UNCONFIRMED → NEW
Component: Session Restore → JavaScript Engine
Ever confirmed: true
Product: Firefox → Core
QA Contact: session.restore → general
Version: unspecified → 1.9.0 Branch

Comment 1

9 years ago
Created attachment 333803 [details] [diff] [review]
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.
Attachment #333803 - Flags: review?(brendan)
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

9 years ago
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)?

Updated

9 years ago
Flags: blocking1.9.1?
Flags: blocking1.9.0.3?
OS: Windows XP → All
Hardware: PC → All
Assignee: general → nobody
Component: JavaScript Engine → XPConnect
QA Contact: general → xpconnect
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
Assignee: nobody → zeniko

Comment 5

9 years ago
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.
Attachment #333803 - Flags: review?(brendan) → review?(shaver)
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. :) )
Attachment #333803 - Flags: review?(shaver) → review?(sayrer)

Updated

9 years ago
Attachment #333803 - Flags: review?(sayrer) → review+

Updated

9 years ago
Attachment #333803 - Flags: superreview?(brendan)
Attachment #333803 - Flags: superreview?(brendan) → superreview+

Comment 7

9 years ago
Thanks for the reviews.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/38f03dcc5e22
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1

Updated

9 years ago
Attachment #333803 - Flags: approval1.9.0.3?

Comment 9

9 years ago
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.
Flags: blocking1.9.1?
Flags: blocking1.9.0.4?
Flags: blocking1.9.0.4+
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
Attachment #333803 - Flags: approval1.9.0.4? → approval1.9.0.4+

Updated

9 years ago
Keywords: checkin-needed
Attachment #333803 - Attachment description: split the string in JSON.jsm when it's too large → split the string in JSON.jsm when it's too large [Checkin: Comment 8]
Whiteboard: [c-n: 1.9.0 branch]
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
Keywords: checkin-needed → fixed1.9.0.4
Whiteboard: [c-n: 1.9.0 branch]

Updated

9 years ago
Depends on: 461048

Comment 12

9 years ago
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!
No longer depends on: 461048
Keywords: checkin-needed
Whiteboard: [needs back-out on branch and trunk]

Updated

9 years ago
Depends on: 461048
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
Status: RESOLVED → REOPENED
Keywords: checkin-needed, fixed1.9.0.4
Resolution: FIXED → ---
Whiteboard: [needs back-out on branch and trunk]
Flags: in-testsuite+ → in-testsuite?
Attachment #333803 - Attachment description: split the string in JSON.jsm when it's too large [Checkin: Comment 8] → split the string in JSON.jsm when it's too large (backed out)
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.
Attachment #333803 - Flags: approval1.9.0.4+ → approval1.9.0.4-

Comment 15

9 years ago
(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?
Assignee: zeniko → nobody
Status: REOPENED → NEW
Flags: blocking1.9.0.4+ → blocking1.9.0.4?
You're right. We don't want this and it doesn't block.
Flags: blocking1.9.0.4? → blocking1.9.0.4-
Simon, does it need a dev-doc update to inform extension developers?

Updated

9 years ago
Keywords: dev-doc-needed
Whiteboard: [dev-doc: use native JSON for large objects, JSON.jsm going away for Fx3.1]

Updated

9 years ago
Depends on: 462774

Updated

9 years ago
Duplicate of this bug: 463399
Simon, can you please give an update about the state of this bug?

Comment 20

9 years ago
This bug is INVALID on Trunk (JSON.jsm was removed in bug 462774) and WONTFIX on the 1.9.0 branch (see comment #16).
Whiteboard: [dev-doc: use native JSON for large objects, JSON.jsm going away for Fx3.1] → [dev-doc: use native JSON for large objects]
(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.
Status: NEW → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → WONTFIX
Updated:

https://developer.mozilla.org/En/Using_native_JSON
Keywords: dev-doc-needed → dev-doc-complete
(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.
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.
Keywords: dev-doc-complete → dev-doc-needed
The code module hasn't only been deprecated but removed. I don't think anything needs to be documented here.
Keywords: dev-doc-needed
Whiteboard: [dev-doc: use native JSON for large objects]
You need to log in before you can comment on or make changes to this bug.