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.
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?
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)?
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 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 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. :) )
Thanks for the reviews.
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 on attachment 333803 [details] [diff] [review] split the string in JSON.jsm when it's too large (backed out) Approved for 22.214.171.124, a=dveditz for release-drivers
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
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!
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 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.
(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?
You're right. We don't want this and it doesn't block.
Simon, does it need a dev-doc update to inform extension developers?
Simon, can you please give an update about the state of this bug?
This bug is INVALID on Trunk (JSON.jsm was removed in bug 462774) and WONTFIX on the 1.9.0 branch (see comment #16).
(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.
(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.
The code module hasn't only been deprecated but removed. I don't think anything needs to be documented here.