Last Comment Bug 551225 - Make pushState use structured clone
: Make pushState use structured clone
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Justin Lebar (not reading bugmail)
:
:
Mentors:
Depends on: 550275 656354 656991
Blocks: 500328 675162
  Show dependency treegraph
 
Reported: 2010-03-09 11:21 PST by Justin Lebar (not reading bugmail)
Modified: 2011-07-30 05:32 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (48.41 KB, patch)
2011-04-10 22:27 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v2 (48.30 KB, patch)
2011-04-21 11:56 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v2.1 (48.98 KB, patch)
2011-04-21 12:22 PDT, Justin Lebar (not reading bugmail)
paul: review+
Details | Diff | Splinter Review
Patch v2.2 (51.08 KB, patch)
2011-05-05 12:03 PDT, Justin Lebar (not reading bugmail)
jonas: review+
Details | Diff | Splinter Review
Patch v2.3 (51.68 KB, patch)
2011-05-06 07:47 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2010-03-09 11:21:57 PST
Right now, pushState and replaceState serialize their state objects to JSON.  We should use structured clone to be compliant with the spec.
Comment 1 Justin Lebar (not reading bugmail) 2010-03-09 11:25:53 PST
On the other hand, JSON serialization is convenient because it lets us easily serialize state objects for session restore.
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-03-09 11:39:34 PST
Right, what we need here is not the structured clone code developed over in bug 550275. What we need is code to serialize and deserialize such that

obj1 --serialize--> ser --deserialize--> obj2

obj2 is a structured clone of obj1. We'll need this code both for pushState and for localStorage.
Comment 3 Justin Lebar (not reading bugmail) 2010-07-27 18:27:23 PDT
Did a quick experiment and it appears that WebKit is doing something similar to us: If you create a state object which contains a function, the function doesn't exist when you get the state object back on popstate.

I like the fact that we can serialize state objects to disk, so I'm not sure we even want to do this.  But if we don't, maybe we should get the spec changed.
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-07-30 08:45:00 PDT
We should definitely serialize. Everything supported by the structured clone algorithm is serializable. The user-visible effect of fixing this bug is that you can pushState objects that contain things like ImageData, dates and regexps (the latter not being terribly important obviously).
Comment 5 Justin Lebar (not reading bugmail) 2011-04-01 08:17:37 PDT
For my reference, it looks like the JSAPI functions are JS_WriteStructuredClone and JS_ReadStructuredClone.  When we write out to storage, we can Base64-encode the value.  And when we read back in, we'll have to convert any JSON-encoded state objects to the new format.
Comment 6 Justin Lebar (not reading bugmail) 2011-04-10 22:27:33 PDT
Created attachment 525039 [details] [diff] [review]
Patch v1

I'll ask for review here once I push to try and verify that this works.
Comment 7 Glenn Maynard 2011-04-11 10:39:27 PDT
(In reply to comment #4)
> We should definitely serialize. Everything supported by the structured clone
> algorithm is serializable. The user-visible effect of fixing this bug is that
> you can pushState objects that contain things like ImageData, dates and regexps

Most importantly, File.  Once structured clones support them, it should be possible to store Files that the user has opened to the history, and to restore them from a serialized session, so users don't have to reopen files each time they restart their browser.
Comment 8 Justin Lebar (not reading bugmail) 2011-04-21 11:56:41 PDT
Created attachment 527605 [details] [diff] [review]
Patch v2
Comment 9 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-04-21 12:01:59 PDT
Is there a reason why nsIStructuredCloneContainer need to live under docshell/ ?
It looks like a very useful interface/class for other use too.

Maybe it could be under dom/intefaces/base ?
Comment 10 Justin Lebar (not reading bugmail) 2011-04-21 12:05:39 PDT
Patch is ready for review.  I fixed the one bug from my last try push, so things *should* be green, but I'll push again to be sure.

I'd originally used JS_AutoStructuredClone inside nsIStructuredCloneContainer, but I was getting strange memory management errors.  (I know you don't have to JS_free with the same cx you JS_malloc'ed with, so I'm not exactly sure what I was doing wrong.)  In any case, I think malloc'ing and free'ing manually is sufficiently simple not to worry about this.
Comment 11 Justin Lebar (not reading bugmail) 2011-04-21 12:06:03 PDT
(In reply to comment #9)
> Is there a reason why nsIStructuredCloneContainer need to live under docshell/
>
> Maybe it could be under dom/intefaces/base ?

That's fine with me!
Comment 12 Justin Lebar (not reading bugmail) 2011-04-21 12:22:14 PDT
Created attachment 527617 [details] [diff] [review]
Patch v2.1

Moved nsIContentUtils, per Smaug's suggestion.
Comment 13 Justin Lebar (not reading bugmail) 2011-04-21 12:37:20 PDT
er...I moved nsIStructuredCloneContainer (and kin), not nsIContentUtils (which hopefully will cease to exist soon).
Comment 14 Justin Lebar (not reading bugmail) 2011-04-21 15:39:32 PDT
(In reply to comment #10)
> Patch is ready for review.  I fixed the one bug from my last try push, so
> things *should* be green, but I'll push again to be sure.

Looks good.

Paul, can you have a look at the sessionstore changes?
Comment 15 Justin Lebar (not reading bugmail) 2011-04-21 15:44:33 PDT
I should mention that this change will cause a one-time loss of state objects upon session restore; I make no attempt to read the old JSON state objects and convert them to the new format.  I think this isn't a big deal, since persisting state across session restore didn't even work properly until we fixed bug 647028 yesterday.
Comment 16 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-04-25 10:21:45 PDT
Comment on attachment 527617 [details] [diff] [review]
Patch v2.1

Can I assume that since there are no tests here, this is covered under some of the other tests you've written? r=me assuming that, but I'll take it back if you tell me I'm wrong.

I'm ok with the one-time data loss. pushState isn't that widely used yet that most people wouldn't even notice.
Comment 17 Justin Lebar (not reading bugmail) 2011-04-25 10:29:59 PDT
Although I think we get decent coverage with the other tests I wrote, it would be trivial to add a test specifically for session restore to browser_500328.js.  I'll definitely add this.
Comment 18 Justin Lebar (not reading bugmail) 2011-05-05 12:03:29 PDT
Created attachment 530390 [details] [diff] [review]
Patch v2.2

Added session restore test.
Comment 19 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-06 04:07:35 PDT
Comment on attachment 530390 [details] [diff] [review]
Patch v2.2

Wow, this looks great
Comment 20 Justin Lebar (not reading bugmail) 2011-05-06 07:47:39 PDT
Created attachment 530622 [details] [diff] [review]
Patch v2.3

I forgot to update nsIDocument's CID and check the return value of JSAutoEnterCompartment::Enter in nsStructuredCloneContainer::InitFromVariant.
Comment 21 Justin Lebar (not reading bugmail) 2011-05-09 13:07:33 PDT
http://hg.mozilla.org/mozilla-central/rev/0221eb8660f4

Note You need to log in before you can comment on or make changes to this bug.