387859 - sessionstore should use native json to parse session data

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071115 Minefield/3.0a7pre ID:2007071115

repro:
startup
e.g. open a page with a bookmark

result
no display

Warning: reference to undefined property this[arguments[0]]
Source file: XPCSafeJSObjectWrapper.cpp
Line: 445

Error: uncaught exception: Permission denied to get property ChromeWindow.Array

regressionwindow
works in 20070711_1431_firefox-3.0a7pre.en-US.win32
fails in 20070711_1606_firefox-3.0a7pre.en-US.win32

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1184189460&maxdate=1184195159

-> bug 386635 ?

Comment 1

[aja+bugzilla]

I'm also seeing the ChromeWindow.Array permission error message...but at startup,
with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071123 Minefield/3.0a7pre ID:2007071123

Other symptoms I'm seeing are reported here:
http://forums.mozillazine.org/viewtopic.php?p=2962840#2962840

Comment 2

[Peter van der Woude [:Peter6]]

Yeah, the message is at startup.
The bookmark thing is just to show that pages aren't shown anymore

Comment 3

[Peter van der Woude [:Peter6]]

The errors are unrelated to the repro case.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

It's hard to see what the actual symptoms being reported here are. I can reproduce the session store bustage, but I can load pages just fine. I'm going to assume that this is about the sessionstore bustage, since it's sessionstore code that's causing the error.

It looks like the patch for bug 386635 broke the workaround added in bug 350558.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Note to nightly testers that want to recover their sessions: the sessionstore.js file is still intact, so if you copy it now before it gets overwritten you'll be able to restore it once this bug is fixed.

Comment 7

[gabe]

could this be related to the recently closed tabs feature being broken to


history>recently closed tabs is blank

Comment 8

[Michael Kraft [:morac]]

(In reply to comment #7)

Yes, since that feature calls the getClosedTabData function, which calls the toJSONString which is the function that is throwing the exception.

Comment 9

[Simon Bünzli]

AFAICT this is no SessionStore specific issue. We just depend on Arrays from the sandbox to behave the same as any other Array - that's no longer the case:

var d = document.body.appendChild(document.createElement("span"));
d.setAttribute("foo", "bar");
var s = new Components.utils.Sandbox("about:blank");
var a = Components.utils.evalInSandbox("({})", s);
a._d = d;
alert(a._d.getAttribute("foo"));

evaluated in the Error Console results in

Error: uncaught exception: Permission denied to call method HTMLSpanElement.getAttribute

whereas alert(d.getAttribute("foo")) - i.e. without the round-trip over the object from the sandbox - does the expected thing.

Furthermore calling .forEach on an array from the sandbox leads to

Error: uncaught exception: Permission denied to get property Function.__parent__

both of which - in case this is expected behavior - will make it pretty much impossible to work with arrays unless they're in some way "de-sandboxed".

The "de-sandboxing" work-around for SessionStore would consist in

(1) replacing |instanceof EVAL_SANDBOX.Array| with a duck-type test:

if (aObj instanceof Array || typeof aObj == "object" && "length" in aObj &&
    (aObj.length === 0 || aObj[aObj.length - 1] !== undefined))

(2) and converting everything returned from the sandbox to a JSON string first and then eval'ing that string normally (bonus points for checking whether we already got a JSON string and then skipping the sandbox completely).

I really hope that won't be necessary, though.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #9)
> var a = Components.utils.evalInSandbox("({})", s);
> a._d = d;

The problem is that this is unsafe (and the corresponding alert is also unsafe). Doing just about anything with an object returned from evalInSandbox breaks the security that sandbox initially provided. Getters and setters can run and do evil stuff to your script objects.

So that's the reason for bug 386635 -- to prevent you from shooting yourself (and the rest of the system) in the foot. I'll try to figure out a way to support this use-case, but it's going to be ... interesting.

Comment 11

[Simon Bünzli]

(In reply to comment #10)
> Doing just about anything with an object returned from evalInSandbox
> breaks the security that sandbox initially provided.

Wasn't the idea of bug 386635 to prevent objects from the sandbox to overrule the default setters/getters (similar to what XPCNativeWrappers do for DOM objects)? Would it alternatively be possible to somehow strip all executable oddities from a sandboxed object? We really just need stock Object, Array, Number, Boolean and null.

Should there be no solution for this, we'll probably want to use JSON strings wherever possible and use a sandbox just to remain backwards compatible (as outlined in comment #9).

Comment 12

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071213 Minefield/3.0a7pre ID:2007071213

Session Restore and Google Suggestions work again after the bug 386635 backout

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #11)
> Wasn't the idea of bug 386635 to prevent objects from the sandbox to overrule
> the default setters/getters (similar to what XPCNativeWrappers do for DOM
> objects)?

Sadly, it's impossible to do this for regular objects. The question is, "what is the 'base' value for a property?" For XPCNativeWrapper, we use the info in the .idl files.

> Would it alternatively be possible to somehow strip all executable
> oddities from a sandboxed object? We really just need stock Object, Array,
> Number, Boolean and null.

From talking with Gavin on IRC, it seems that using evalInSandbox here is really a belt-and-braces thing. So perhaps it's OK to simply use the return value as-is in your case (given the limited control content has over the input here). What I'll probably end up doing is to add a parameter to the Sandbox constructor that allows scripts to opt in to using safe JSObject wrappers.

Comment 14

[Simon Bünzli]

Attachment: convert SessionStore to JSON (mostly)

> What I'll probably end up doing is to add a parameter to the Sandbox
> constructor that allows scripts to opt in to using safe JSObject wrappers.

Either that, or we (mostly) ditch the sandbox for pure JSON - which is really all we need and which will get sandbox-less JavaScript support in bug 340987 anyway.

So, if Gavin agrees, you should be able to go ahead with what you had originally planned (Gavin: for the search suggestions, you can also replace the sandbox with a simple eval without risking run-away code - the RegExp will catch anything looking like a function).

Comment 15

[Simon Bünzli]

Attachment: convert SessionStore to JSON (mostly)

Comment 16

[:Gavin Sharp [email: [e-mail]]]

It looks to me that this is now more about code cleanup/simplification than it is about making session store work again, since the patch for bug 386635 was backed out (and won't be relanded).

Comment 17

[Simon Bünzli]

Comment on attachment 272604 [details] [diff] [review]
convert SessionStore to JSON (mostly)

If this doesn't have to happen ASAP, I'd rather wait until bug 340987 has been fixed.

Comment 18

[Simon Bünzli]

I'll fix this bug over in bug 407110. Note that sessionstore.js won't be a pure JSON file for backwards compatibility with Firefox 2 and 3.0 (the only difference being a pair of parentheses around the file's content). And I don't see a reason why it should be.

Comment 19

[[e-mail]]

Comment on attachment 272604 [details] [diff] [review]
convert SessionStore to JSON (mostly)

>+    if (!this._canSafelyEval(aStr)) {
>+      // for backwards compatibility: eval in a sandbox, convert
>+      // to a JSON string and then eval without risk.
>+      var s = new Components.utils.Sandbox("about:blank");
>+      var obj = Components.utils.evalInSandbox("(" + aStr + ")", s);
>+      aStr = this._toJSONString(obj);
>+    }
>+    
>+    return eval("(" + aStr + ")");
I don't get this. Why can't you use JSON.parse? If you can't, why do you convert obj to a string and eval it?

Comment 20

[Michael Kraft [:morac]]

Isn't a JSON.parse faster than evalInSandbox?

If the only difference would be a pair of parentheses, then you could check aStr for beginning and ending parentheses and simply remove them and do a JSON.parse.

If the JSON.parse throws an exception, then you could fall back to doing an evalInSandbox.

Comment 21

[Simon Bünzli]

Comment on attachment 272604 [details] [diff] [review]
convert SessionStore to JSON (mostly)

(In reply to comment #19)
This code predates JSON.parse over a year and has become obsolete with bug 407110 fixed.

Comment 22

[Simon Bünzli]

Attachment: WIP

Comment 23

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

Gets rid of all eval(InSandbox)/toSource uses. This came up again with bug 558540 - Blake said we should really stop using evalInSandbox.

This passes all current tests (some had to be modified to get/pass JSON). I haven't actually tested that this loading a current session, but it should work.

I think the only thing this actually breaks is passing in invalid JSON. For example,
> ss.setTabState(tab, "{ entries: [] }")
used to work, but now it doesn't and it must be a valid JSON string...
> ss.setTabState(tab, JSON.stringify({ entries: [] })
> ss.setTabState(tab, '{ "entries": [] }')

Also, it seems like \u2028 and \u2029 work now with JSON.parse/stringify - at least it passes the test for it.

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 443938 [details] [diff] [review]
Patch v0.1

>   /**
>    * @param aWindow      is the browser window to set the value for.
>    * @param aKey         is the value's name.
>-   * @param aStringValue is the value itself (use toSource/eval before setting JS objects).
>-   */
>+   * @param aStringValue is the value itself (use JSON.stringify/parse before setting JS objects).  */

looks like you accidentally removed a line break

Comment 25

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #24)
> looks like you accidentally removed a line break

Indeed. Fixed locally.

Comment 26

[Simon Bünzli]

Comment on attachment 443938 [details] [diff] [review]
Patch v0.1

This looks generally fine. Just a few things to consider:

>-        this._stateBackup = this._safeEval(this._getCurrentState(true).toSource());
>+        this._stateBackup = JSON.parse(this._getCurrentState(true));

Note: _getCurrentState doesn't return a string but an object which has to be stringified first. Please add a test for ensuring this functionality, in case there isn't any.

>     // parentheses are for backwards compatibility with Firefox 2.0 and 3.0
>-    stateString.data = "(" + this._toJSONString(aStateObj) + ")";
>+    stateString.data = JSON.stringify(aStateObj);

No more parentheses here means the both the comment is wrong and Firefox 3.0 won't be able to read these files any more (which I guess is fine, considering that Firefox 3.0 has reached EOL).

>-  _toJSONString: function sss_toJSONString(aJSObject) {
>-    // XXXzeniko drop the following keys used only for internal bookkeeping:
>-    //           _tabStillLoading, _hosts, _formDataSaved

I'd still be in favor of dropping these artefacts... JSON.stringify should allow to do so quite reasonably.

Comment 27

[Misak Khachatryan]

> I'd still be in favor of dropping these artefacts... JSON.stringify should
> allow to do so quite reasonably.

SeaMonkey uses this code, should apply to FF too:

http://mxr.mozilla.org/comm-central/source/suite/common/src/nsSessionStore.js#2691

Also, per Simon suggestion, we use "sessionstore.json" as session file name in user profile.

Comment 28

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #26)
> (From update of attachment 443938 [details] [diff] [review])
> This looks generally fine. Just a few things to consider:
> 
> >-        this._stateBackup = this._safeEval(this._getCurrentState(true).toSource());
> >+        this._stateBackup = JSON.parse(this._getCurrentState(true));
> 
> Note: _getCurrentState doesn't return a string but an object which has to be
> stringified first. Please add a test for ensuring this functionality, in case
> there isn't any.

Must have missed this while replacing. It's a bit of tough thing to test, but I'll look into it. t might have to be a litmus test - the only time _stateBackup makes it out of here is if we quit during private browsing.

You'd think there would at least have been an error since entering PB is tested, but JSON.parse mostly just fails silently (which probably shouldn't be the case).

> >     // parentheses are for backwards compatibility with Firefox 2.0 and 3.0
> >-    stateString.data = "(" + this._toJSONString(aStateObj) + ")";
> >+    stateString.data = JSON.stringify(aStateObj);
> 
> No more parentheses here means the both the comment is wrong and Firefox 3.0
> won't be able to read these files any more (which I guess is fine, considering
> that Firefox 3.0 has reached EOL).

True. We also phased out the other support for Firefox 3.0 sessions (bug 551285), so I think it's ok.

> >-  _toJSONString: function sss_toJSONString(aJSObject) {
> >-    // XXXzeniko drop the following keys used only for internal bookkeeping:
> >-    //           _tabStillLoading, _hosts, _formDataSaved
> 
> I'd still be in favor of dropping these artefacts... JSON.stringify should
> allow to do so quite reasonably.

I forgot about dropping those.

(In reply to comment #27)
> SeaMonkey uses this code, should apply to FF too:
> 
> http://mxr.mozilla.org/comm-central/source/suite/common/src/nsSessionStore.js#2691

Thanks for that Misak, I forgot that JSON.stringify has that functionality.

> Also, per Simon suggestion, we use "sessionstore.json" as session file name in
> user profile.

Meh. I don't see a very compelling reason to do that. I understand that sure, it's now actually JSON, but we'd have to support loading of both files. I'll file a bug for it, but it'll pretty low priority and likely won't happen unless somebody else does it.

Comment 29

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

So the only reason I don't have this done is because for some ^%(&@!# reason, this patch is causing session restore to save the private browsing session at quit. _stateBackup is getting corrupted at some point, at least when it's written as 
> this._stateBackup = this._getCurrentState(true);
When I write it as
> this._stateBackup = JSON.parse(JSON.stringify(this._getCurrentState(true)));
it works. I guess something from _getCurrentState being held on to outside the function and then is changing.

To be clear, only part of _stateBackup was getting corrupted - windows. cookies were correct

I'm asking ehsan why he did
> this._stateBackup = this._safeEval(this._getCurrentState(true).toSource());
in the first place? That may have just been masking an underlying problem.

Comment 30

[(no longer active)]

I can confirm that I originally wrote that code to make sure that I'm not holding any references to live objects.

Comment 31

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #29)
> To be clear, only part of _stateBackup was getting corrupted - windows. cookies
> were correct

To be more clear, it wasn't the whole windows array. cookies is a prop on each window, so only part of the window was getting corrupted.

> I'm asking ehsan why he did
> > this._stateBackup = this._safeEval(this._getCurrentState(true).toSource());
> in the first place? That may have just been masking an underlying problem.

Apparently for the same reason. _getCurrentState returns an object that changes, I guess just refs to objects in this._windows, which are probably getting modified at quit-application-requested, when we "get a current snapshot of all windows"

So it looks like I'll need to stringify, then parse it :/

Comment 32

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.2

Fixes the mentioned issues above.

No test for _backupState because like I said, it only ever makes it out when quitting while in PB. I'll look to see if there's an existing litmus test for it. My guess is that there is - it's a pretty obvious potential privacy hole.

Comment 33

[(no longer active)]

(In reply to comment #32)
> No test for _backupState because like I said, it only ever makes it out when
> quitting while in PB. I'll look to see if there's an existing litmus test for
> it. My guess is that there is - it's a pretty obvious potential privacy hole.

I _think_ that there is a Litmus test for it.  If not, there should be!  :-)  Or better yet, a MozMill test.

Comment 34

[(no longer active)]

Comment on attachment 449159 [details] [diff] [review]
Patch v0.2

>     case "private-browsing-change-granted":
>       if (aData == "enter") {
>         this.saveState(true);
>-        this._stateBackup = this._safeEval(this._getCurrentState(true).toSource());
>+        this._stateBackup = JSON.parse(this._toJSONString(this._getCurrentState(true)));

Perhaps add a comment to explain why we're doing this?

Comment 35

[Michael Kraft [:morac]]

I'm fairly certain the this._stateBackup is the same issue that I reported in bug 497975.

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #33)
> (In reply to comment #32)
> > No test for _backupState because like I said, it only ever makes it out when
> > quitting while in PB. I'll look to see if there's an existing litmus test for
> > it. My guess is that there is - it's a pretty obvious potential privacy hole.
> 
> I _think_ that there is a Litmus test for it.  If not, there should be!  :-) 
> Or better yet, a MozMill test.

Anthony, is managing this subgroup on Litmus. Lets add him so he can have a look if there is already a test for that.

Comment 37

[Simon Bünzli]

Comment on attachment 449159 [details] [diff] [review]
Patch v0.2

(In reply to comment #28)
> True. We also phased out the other support for Firefox 3.0 sessions (bug
> 551285), so I think it's ok.

That's not quite the same. Even with bug 551285, Firefox.next can read most of a Firefox 3.0 session. With this bug, Firefox 3.0 OTOH won't be able to read a Firefox.next session at all (not even getting a single tab loaded). This could be a minor issue for people sharing a profile at work and at home, where they're stuck with an outdated Firefox version in one place. That's however my only argument for delaying that change until bug 564520 is fixed.

(In reply to comment #34)
> Perhaps add a comment to explain why we're doing this?

Please do so before checking in. Thanks.

Comment 38

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #37)
> (From update of attachment 449159 [details] [diff] [review])
> (In reply to comment #28)
> > True. We also phased out the other support for Firefox 3.0 sessions (bug
> > 551285), so I think it's ok.
> 
> That's not quite the same. Even with bug 551285, Firefox.next can read most of
> a Firefox 3.0 session. With this bug, Firefox 3.0 OTOH won't be able to read a
> Firefox.next session at all (not even getting a single tab loaded). This could
> be a minor issue for people sharing a profile at work and at home, where
> they're stuck with an outdated Firefox version in one place. That's however my
> only argument for delaying that change until bug 564520 is fixed.

That's true, unless we decide to add (at least for 3.5/3.6) the ability to read the JSON (which shouldn't be too bad, just add the "(" ")" around it before trying to eval). We can talk about it more in bug 564520 if you want.

I'll hold off landing this for now.

> (In reply to comment #34)
> > Perhaps add a comment to explain why we're doing this?
> 
> Please do so before checking in. Thanks.

Will do.

Comment 39

[Simon Bünzli]

(In reply to comment #38)
> That's true, unless we decide to add (at least for 3.5/3.6) the ability to read
> the JSON (which shouldn't be too bad, just add the "(" ")" around it before
> trying to eval).

Firefox 3.5 and 3.6 should actually already be able to read a proper JSON-formatted sessionstore.js. Aren't they?

Comment 40

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #39)
> (In reply to comment #38)
> > That's true, unless we decide to add (at least for 3.5/3.6) the ability to read
> > the JSON (which shouldn't be too bad, just add the "(" ")" around it before
> > trying to eval).
> 
> Firefox 3.5 and 3.6 should actually already be able to read a proper
> JSON-formatted sessionstore.js. Aren't they?

You're right. Sessions between currently supported versions works fine.

Comment 41

[u279076]

So what exactly are the steps to reproduce, the result, and the expected result?  The original comment is not completely clear about any and the comments following do not clearly explain what is happening, what should be happening, and the steps to reproduce this failure.

Can someone please clarify this for me?  I'm trying to understand this bug for the purposes of the in-litmus? flag.  Thanks.

Comment 42

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #41)
> So what exactly are the steps to reproduce, the result, and the expected
> result?  The original comment is not completely clear about any and the
> comments following do not clearly explain what is happening, what should be
> happening, and the steps to reproduce this failure.
> 
> Can someone please clarify this for me?  I'm trying to understand this bug for
> the purposes of the in-litmus? flag.  Thanks.

The in-litmus? flag was more a question than anything. I had uncovered a case while working on in progress patch that was leaking the private browsing session. It doesn't currently happen and doesn't happen with the final patch. But it's not something that can be covered by our testing framework as is. It's not directly related to this bug even.

STR: Browse normally, open a couple tabs; Enter PB mode; Quit; Start Firefox again
Expected: Session before entering PB is restored
Bad result: Windows/tabs from PB session restored

I seems like something that would be in litums already, I just haven't checked yet

Comment 43

[u279076]

> STR: Browse normally, open a couple tabs; Enter PB mode; Quit; Start Firefox
> again
> Expected: Session before entering PB is restored
> Bad result: Windows/tabs from PB session restored
> 
> I seems like something that would be in litums already, I just haven't checked
> yet

I checked over the Session Store and Private Browsing tests.  It does not appear this scenario has been covered.  When this is fixed, which branches will it be applied to (trunk, 192, 191, etc)?

Comment 44

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

To be clear: *this is not currently broken*, and it's only tangentially related to this bug. Behavior should be correct in all branches with PB.

It was something I managed to hit with an incomplete patch and something I might have missed had I not been looking at another bug while doing this work. I just want to make sure we do have that case covered _somewhere_ since we can't automate it.

Comment 45

[u279076]

(In reply to comment #44)
> To be clear: *this is not currently broken*, and it's only tangentially related
> to this bug. Behavior should be correct in all branches with PB.
> 
> It was something I managed to hit with an incomplete patch and something I
> might have missed had I not been looking at another bug while doing this work.
> I just want to make sure we do have that case covered _somewhere_ since we
> can't automate it.

So the Litmus test case is not really a regression test for this bug.  In my opinion, moving to the JSON parser is largely a backend change which should be picked up by the entire Session Store test run.  In my opinion, I see the Litmus test as testing something we should be checking that PB sessions are not restored (restoring the pre-PB session instead).

Comment 46

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #45)
> So the Litmus test case is not really a regression test for this bug.  In my
> opinion, moving to the JSON parser is largely a backend change which should be
> picked up by the entire Session Store test run.

Correct, and agreed.

> In my opinion, I see the
> Litmus test as testing something we should be checking that PB sessions are not
> restored (restoring the pre-PB session instead).

Exactly.

Comment 47

[u279076]

Thanks Paul for helping me clarify this in the context of Litmus.  I'll go ahead and add that test shortly.

Comment 48

[u279076]

3.7: https://litmus.mozilla.org/show_test.cgi?id=11883
3.6: https://litmus.mozilla.org/show_test.cgi?id=11884
3.5: https://litmus.mozilla.org/show_test.cgi?id=11885

Comment 49

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/e3372af81a76

Comment 50

[u279076]

Verified fixed on Firefox 4.0b3