We can get better performance at startup and likely Tp by using the new native json functionality, once it's landed.

This should make hardly any performance difference since we use JSON only for the exposed API which is mostly needed to display the list of recently closed tabs. Once we've got this, we can reconsider bug 387859, though. BTW: AFAICT Robert hasn't implemented an easy way to blacklist individual key/value pairs yet. We'd lose quite some perf improvement if we have to prune those manually in JS first (if there's any measurable improvement left at all, that is). NB: I'd be fine with being able to pass in a RegExp which keys have to match.

Besides speeding up our extension facing API, this should mainly "fix" all the Firebug related bugs that are caused by toSource. Note that we're still accepting toSource'd objects through our API and that for backwards compatibility sessionstore.js isn't pure JSON, either (the difference being a pair of parentheses around the whole object).

Comment on attachment 335046 [details] [diff] [review] use native JSON instead of toSource nsIJSON.encode no longer takes a whitelist param. also, where is _tabStillLoading getting set? i don't see that anywhere in the patch...

Comment on attachment 335046 [details] [diff] [review] use native JSON instead of toSource (In reply to comment #3) > nsIJSON.encode no longer takes a whitelist param. s/no longer/not yet (still)/ Now that we've already got more than half of proper native JSON, I prefer to wait for bug 442059 being fixed before coming back to this one - as that will be make this so much simpler to fix.

Turns out that we don't really need bug 442059 as native JSON just silently drops unencodable objects.

Comment on attachment 345968 [details] [diff] [review] use JSON.stringify (without blacklist) > _toJSONString: function sss_toJSONString(aJSObject) { >- let str = JSONModule.toString(aJSObject, ["_tab", "_hosts", "_formDataSaved"] /* keys to drop */); >- >- // sanity check - so that API consumers can just eval this string >- if (!JSONModule.isMostlyHarmless(str)) >- throw new Error("JSON conversion failed unexpectedly!"); >- >- return str; >+ // XXXzeniko drop the following keys: _tab, _hosts, _formDataSaved >+ return JSON.stringify(aJSObject); > }, how/why are those unencodable?

The only unencodable object is _tab which is a reference to a <xul:tab>. (OTOH _hosts and _formDataSaved are for internal use only and thus should ideally not be unnecessarily exposed.)

Comment on attachment 345968 [details] [diff] [review] use JSON.stringify (without blacklist) is there anything in that data that might be privacy sensitive, that couldn't already be discovered via on disk files?

No, _formDataSaved is a boolean indicating whether the new formdata attribute is up to date - and _hosts is the complete list of hosts for all the URLs we return anyways. Both of these have already been written to sessionstore.js as long as they existed, we'd now just (temporarily) also return them through our API.

Comment on attachment 345968 [details] [diff] [review] use JSON.stringify (without blacklist) r=me, thanks!

This bug was supposed to fix issues like bug 366509 and the more recent regression 463015. Unfortunately, JSON.stringify suddenly starts throwing while using JSON.jsm with a blacklist works just fine (and stringifying an object with _tab removed works fine as well). I guess, we still need the more extensive fix - and we need it ASAP. Updated patch coming...

Comment on attachment 346349 [details] [diff] [review] never include non-serializable objects in our data objects the json change looks ok. the tabs change looks internally consistent, however: how does it address the problem in those other bugs? actually, what is the cause of that problem? what does it have to do w/ this bug?

I don't really know what the exact problem is, it just seems quite related to toSourcing a <xul:tab> (see the dependencies of bug 429414). Using JSON will get rid of that - just that our JSON serialization seems to choke on those <xul:tab>s as well, so we need the rest of the changes (unless somebody more familiar with the core actually sits down and figures out the actual issue at hand).

Comment on attachment 346349 [details] [diff] [review] never include non-serializable objects in our data objects thanks for the explanation, makes sense now. r=me.

marking blocking+ as it breaks the RCT menu.

Any special reason this is not RESOLVED FIXED now?

Anyway i can verify this fix?

(In reply to comment #20) sessionstore.js has to be JSON without the module JSON.jsm being present in Firefox's modules folder (i.e. with bug 462774 being verified) and bugs like bug 366509 shouldn't be happening anymore.

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090123 Shiretoko/3.1b3pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090123 Minefield/3.2a1pre JSON.jsm is no longer present.

I still do have the issue on Mac OS X 10.5.8 using firefox 6.0.2