Handle DOM Storage in Private Browsing Mode

RESOLVED FIXED in Firefox 3.6a1

Status

()

Firefox
Private Browsing
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

({fixed1.9.1})

Trunk
Firefox 3.6a1
fixed1.9.1
Points:
---
Bug Flags:
blocking-firefox3.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

9 years ago
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.  :-)
Flags: blocking-firefox3.1?
Blocking for decisions to be made. Seems like we should do the same thing as we do for cookies, in terms of policy.
Flags: blocking-firefox3.1? → blocking-firefox3.1+
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.

Comment 3

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

Comment 4

9 years ago
Mass moving of all Firefox::General private browsing bugs to Firefox::Private Browsing.
Component: General → Private Browsing
(Assignee)

Updated

9 years ago
QA Contact: general → private.browsing
Priority: -- → P2
(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?

Comment 6

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

Updated

9 years ago
Whiteboard: [PB-P1]
(Assignee)

Comment 7

9 years ago
Created attachment 362296 [details] [diff] [review]
WIP

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?
Whiteboard: [PB-P1] → [PB-P1][wip patch][needs comment dcamp]

Comment 8

9 years ago
This might be bug 426436, have you tried checking against an empty string rather than undefined?
(Assignee)

Comment 9

9 years ago
(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.
Whiteboard: [PB-P1][wip patch][needs comment dcamp] → [PB-P1]
(Assignee)

Comment 10

9 years ago
Created attachment 362887 [details] [diff] [review]
Patch (v1)

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.
Attachment #362296 - Attachment is obsolete: true
Attachment #362887 - Flags: superreview?(dcamp)
Attachment #362887 - Flags: review?(dcamp)
(Assignee)

Updated

9 years ago
Whiteboard: [PB-P1]
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs review campd]

Comment 11

9 years ago
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.
Attachment #362887 - Flags: superreview?(jst)
Attachment #362887 - Flags: superreview?(dcamp)
Attachment #362887 - Flags: review?(dcamp)
Attachment #362887 - Flags: review+
(Assignee)

Comment 12

9 years ago
Created attachment 362947 [details] [diff] [review]
Patch (v1.1)

Made mInPrivateBrowsing an instance variable.
Attachment #362887 - Attachment is obsolete: true
Attachment #362947 - Flags: superreview?(jst)
Attachment #362887 - Flags: superreview?(jst)
(Assignee)

Comment 13

9 years ago
Created attachment 362949 [details] [diff] [review]
Patch (v1.1)

Ah, forgot to qrefresh...  :/
Attachment #362947 - Attachment is obsolete: true
Attachment #362949 - Flags: superreview?(jst)
Attachment #362947 - Flags: superreview?(jst)
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs review campd] → [has patch][needs super-review jst]

Updated

9 years ago
Attachment #362949 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 14

9 years ago
http://hg.mozilla.org/mozilla-central/rev/9059416d26cf
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch][needs super-review jst]
Target Milestone: Firefox 3.1 → Firefox 3.2a1
(Assignee)

Comment 15

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/014b94780259
Keywords: fixed1.9.1
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 17

9 years ago
Re-landed on trunk: <http://hg.mozilla.org/mozilla-central/rev/b442420b4e84>
(Assignee)

Updated

9 years ago
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.