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 ?

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

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

The errors are unrelated to the repro case.

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.

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.

could this be related to the recently closed tabs feature being broken to history>recently closed tabs is blank

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

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.

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

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

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

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

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

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

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 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?

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

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

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

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.

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

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

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.

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

(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 :/

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.

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

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

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

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

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

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

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.

(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

> 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)?

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.

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

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

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

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

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

Verified fixed on Firefox 4.0b3