sessionstore should use native json to serialize session data

VERIFIED FIXED in Firefox 3.1b2

Status

()

defect
P1
major
VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: dietrich, Assigned: zeniko)

Tracking

({verified1.9.1})

Trunk
Firefox 3.1b2
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Comment 1

12 years ago
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.
Blocks: 387859
Flags: blocking1.8.0.14?
Flags: blocking1.8.0.14?
(Assignee)

Updated

11 years ago
Depends on: 442059
(Assignee)

Comment 2

11 years ago
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).
Attachment #335046 - Flags: review?(dietrich)
(Assignee)

Updated

11 years ago
Assignee: nobody → zeniko
(Reporter)

Updated

11 years ago
Attachment #335046 - Flags: review?(dietrich) → review-
(Reporter)

Comment 3

11 years ago
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...
(Assignee)

Comment 4

11 years ago
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.
Attachment #335046 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Blocks: 461048
(Assignee)

Comment 5

11 years ago
Turns out that we don't really need bug 442059 as native JSON just silently drops unencodable objects.
Attachment #345968 - Flags: review?(dietrich)
(Assignee)

Updated

11 years ago
Blocks: 462774
(Reporter)

Comment 6

11 years ago
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?
(Assignee)

Comment 7

11 years ago
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.)
(Reporter)

Comment 8

11 years ago
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?
(Assignee)

Comment 9

11 years ago
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.
(Reporter)

Comment 10

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

r=me, thanks!
Attachment #345968 - Flags: review?(dietrich) → review+
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
Target Milestone: --- → Firefox 3.1b2
(Assignee)

Comment 11

11 years ago
Posted patch unbitrotted (for check-in) (obsolete) — Splinter Review
Attachment #345968 - Attachment is obsolete: true
(Assignee)

Comment 12

11 years ago
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...
Flags: blocking-firefox3.1?
Keywords: checkin-needed
(Assignee)

Comment 13

11 years ago
Attachment #346227 - Attachment is obsolete: true
Attachment #346349 - Flags: review?(dietrich)
(Reporter)

Comment 14

11 years ago
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?
(Assignee)

Comment 15

11 years ago
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).
(Reporter)

Comment 16

11 years ago
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.
Attachment #346349 - Flags: review?(dietrich) → review+
(Assignee)

Updated

11 years ago
Keywords: checkin-needed
(Reporter)

Comment 17

11 years ago
marking blocking+ as it breaks the RCT menu.
Flags: blocking-firefox3.1? → blocking-firefox3.1+
(Reporter)

Comment 18

11 years ago
http://hg.mozilla.org/mozilla-central/rev/10c21d116e5d
Severity: normal → major
Keywords: checkin-needed
Priority: -- → P1
Blocks: 463015
Any special reason this is not RESOLVED FIXED now?
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
No longer depends on: 442059
Anyway i can verify this fix?
(Assignee)

Comment 21

11 years ago
(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.
Status: RESOLVED → VERIFIED
(Reporter)

Updated

10 years ago
Summary: sessionstore should use native json → sessionstore should use native json to serialize session data

Comment 23

8 years ago
I still do have the issue on Mac OS X 10.5.8 using firefox 6.0.2
You need to log in before you can comment on or make changes to this bug.