407110 - sessionstore should use native json to serialize session data

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P1)

Description

[Dietrich Ayala (:dietrich)]

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

Comment 1

[Simon Bünzli]

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.

Comment 2

[Simon Bünzli]

Attachment: use native JSON instead of toSource

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 3

[Dietrich Ayala (:dietrich)]

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 4

[Simon Bünzli]

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.

Comment 5

[Simon Bünzli]

Attachment: use JSON.stringify (without blacklist)

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

Comment 6

[Dietrich Ayala (:dietrich)]

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?

Comment 7

[Simon Bünzli]

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 8

[Dietrich Ayala (:dietrich)]

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?

Comment 9

[Simon Bünzli]

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 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 345968 [details] [diff] [review]
use JSON.stringify (without blacklist)

r=me, thanks!

Comment 11

[Simon Bünzli]

Attachment: unbitrotted (for check-in)

Comment 12

[Simon Bünzli]

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 13

[Simon Bünzli]

Attachment: never include non-serializable objects in our data objects

Comment 14

[Dietrich Ayala (:dietrich)]

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?

Comment 15

[Simon Bünzli]

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 16

[Dietrich Ayala (:dietrich)]

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.

Comment 17

[Dietrich Ayala (:dietrich)]

marking blocking+ as it breaks the RCT menu.

Comment 18

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/mozilla-central/rev/10c21d116e5d

Comment 19

[(no longer active)]

Any special reason this is not RESOLVED FIXED now?

Comment 20

[Tony Chung [:tchung]]

Anyway i can verify this fix?

Comment 21

[Simon Bünzli]

(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.

Comment 22

[Tony Chung [:tchung]]

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.

Comment 23

[firefox on osx]

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