To throw or not throw a security error on DOM Storage access in private browsing mode; to use or not to use a memory-based DB implementation for DOM Storage inside the private browsing mode. These are the questions. :-)

Blocking for decisions to be made. Seems like we should do the same thing as we do for cookies, in terms of policy.

dcamp, is there a way to force everything to session-only? dveditz seemed to think so, otherwise I'm inclined to just fail here for 3.1.

It should work to always return false in nsDOMStorage::UseDB() when in private mode. That's what we do when the cookie pref is session-only..

Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.

(In reply to comment #3) > It should work to always return false in nsDOMStorage::UseDB() when in private > mode. That's what we do when the cookie pref is session-only.. As I understand it, this would that within a PB session, sites could set and retrieve DOMStorage info, but that it wouldn't persist after PB exit. What would it do to DOMStorage set up before entering PB? Am I right to think that this would be a pretty easy patch?

After entering PB mode, you'd get an empty, memory-only storage. It wouldn't overwrite the real on-disk data, but it wouldn't be able to access it either. When you came out of PB mode you'd be back with the original db-backed data. It would mostly be an easy patch - you just need to start returning false from UseDB(), and upon entering and leaving PB mode you'd need to clear mItems/mItemsCached.

In this patch, I think I have implemented all the things mentioned in comment 6, however the test fails because Pair-A exists in the private mode. Can you think of something that I'm doing wrong here?

This might be bug 426436, have you tried checking against an empty string rather than undefined?

(In reply to comment #8) > This might be bug 426436, have you tried checking against an empty string > rather than undefined? Yes, it looks like it. I'll check to make sure and will update the patch if necessary. With this issue addressed I think the patch is ready for review.

Bug 426436 was indeed the problem here. I updated the unit test to account for that (and a few other minor enhancements), but code wise this is the same as the WIP patch. And it's ready for review.

Comment on attachment 362887 [details] [diff] [review] Patch (v1) > nsDOMStorageManager* nsDOMStorageManager::gStorageManager; >+PRBool nsDOMStorageManager::mInPrivateBrowsing = PR_FALSE; nsDOMStorageManager is a singleton, this can be an instance member. Looks good other than that. Moved sr over to jst, I'm not a super-reviewer.

Made mInPrivateBrowsing an instance variable.

Ah, forgot to qrefresh... :/

I backed this out on suspicion of causing semi-random mochichrome orange (which did indeed clear up). Please reland on its own at some point, or at least not with the other two patches it landed with.

Re-landed on trunk: <http://hg.mozilla.org/mozilla-central/rev/b442420b4e84>