All users were logged out of Bugzilla on October 13th, 2018

Should clear DOM storage when Clear Private Data is used

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: enndeakin, Assigned: enndeakin)

Tracking

({fixed1.8.1})

unspecified
x86
Mac OS X
fixed1.8.1
Points:
---
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

12 years ago
 

Updated

12 years ago
Flags: blocking-firefox2+
I'm guessing this would clear along with cookies?
Given localization concerns tieing it to cookies might be our only choice at this late point, and easy to explain to users: DOM Storage is just "super cookies". But I think it's ultimately a bad choice that will discourage use of the feature and should eventually get it's own checkbox setting. (Maybe that has to wait for FF3)

If sites ever actually store useful data in there (like local writely documents)  users might well want to clear their cookies and not blow away the DOM Storage.

But it absolutely must be clearable one way or another -- if people think they've cleaned up their tracks and they've got persistent backup cookies in DOM Storage (a likely first use for the feature, as with Flash local storage) they will be quite upset.
We cannot touch l10n, so cookies it is (for now)

Neil, do you have an ETA?
(Assignee)

Comment 4

12 years ago
Created attachment 238567 [details] [diff] [review]
Clear DOM Storage when cookies are deleted

The only issue is that the Cookies option is disabled when there are no cookies.
Attachment #238567 - Flags: superreview?(dveditz)
Attachment #238567 - Flags: review?(jst)
(Assignee)

Comment 5

12 years ago
Created attachment 238642 [details] [diff] [review]
Always enable clear all cookies options in Clear Private Data and Cookies dialogs
Attachment #238642 - Flags: review?(mconnor)
Comment on attachment 238642 [details] [diff] [review]
Always enable clear all cookies options in Clear Private Data and Cookies dialogs

r=me, thanks Neil!
Attachment #238642 - Flags: review?(mconnor) → review+
Comment on attachment 238567 [details] [diff] [review]
Clear DOM Storage when cookies are deleted

- In nsLayoutStatics::Initialize():

   if (!txXSLTProcessor::init()) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
+  nsDOMStorageManager::Initialize();

This can return error codes, don't loose them, return early on error.

r=jst with that.
Attachment #238567 - Flags: review?(jst) → review+
Comment on attachment 238642 [details] [diff] [review]
Always enable clear all cookies options in Clear Private Data and Cookies dialogs

sr=dveditz
Attachment #238642 - Flags: superreview+
Comment on attachment 238567 [details] [diff] [review]
Clear DOM Storage when cookies are deleted

sr=dveditz
Attachment #238567 - Flags: superreview?(dveditz) → superreview+
I assume writing null to the key in sqlite deletes the actual data off the disk and doesn't just mark it deleted in some way. Is that right?

Comment 11

12 years ago
Comment on attachment 238567 [details] [diff] [review]
Clear DOM Storage when cookies are deleted

a=schrep - thanks Neil, JST, and DVeditz
Attachment #238567 - Flags: approval1.8.1+

Comment 12

12 years ago
Comment on attachment 238642 [details] [diff] [review]
Always enable clear all cookies options in Clear Private Data and Cookies dialogs

a=schrep - thanks Neil, JST, and DVeditz
Attachment #238642 - Flags: approval1.8.1+
(Assignee)

Comment 13

12 years ago
Checked into trunk

Comment 14

12 years ago
Please land on 1.8 branch...
Whiteboard: [checkin needed (1.8 branch)]
Created attachment 238864 [details] [diff] [review]
Proposed fix for building xulrunner minimal configuration
Attachment #238864 - Flags: superreview?(dveditz)
Attachment #238864 - Flags: review?

Updated

12 years ago
Attachment #238864 - Flags: review? → review?(mconnor)
Comment on attachment 238864 [details] [diff] [review]
Proposed fix for building xulrunner minimal configuration

I'm so far away from being the right reviewer here, but are there cases where we build DOMStorage without MOZ_STORAGE?
Attachment #238864 - Flags: review?(mconnor) → review?(jst)
Comment on attachment 238864 [details] [diff] [review]
Proposed fix for building xulrunner minimal configuration

Neil ought to review this, he's the owner of this code.
Attachment #238864 - Flags: review?(jst) → review?(enndeakin)
(Assignee)

Comment 19

12 years ago
Comment on attachment 238864 [details] [diff] [review]
Proposed fix for building xulrunner minimal configuration

You may have mistook the MOZ_STORAGE define to refer to DOMStorage (actually called Client-side persistent storage). The define instead refers to the sqllite component.

This patch appears to just remove the clearing ability for session-only storage when MOZ_STORAGE is not defined, and session-only storage doesn't even use the database.

Could you describe what the actual build error is?
Attachment #238864 - Flags: review?(enndeakin) → review-
Created attachment 238953 [details]
Build log with error

mozilla/dom/src/base/../storage/nsDOMStorage.h:69: error: ISO C++ forbids declaration of ‘nsDOMStorage’ with no type
mozilla/dom/src/base/../storage/nsDOMStorage.h:69: error: expected ‘;’ before ‘*’ token
mozilla/dom/src/base/../storage/nsDOMStorage.h:91: error: ‘nsDOMStorage’ has not been declared
mozilla/dom/src/base/../storage/nsDOMStorage.h:92: error: ‘nsDOMStorage’ has not been declared
Created attachment 238954 [details]
Brief html from tinderbox
Attachment #238953 - Attachment is obsolete: true

Comment 22

12 years ago
I think this has caused new crashes @  nsSubstring::IsDependentOn
For example
http://talkback-public.mozilla.org/search/start.jsp?search=2&type=iid&id=23398975

Comment 23

12 years ago
Created attachment 238984 [details] [diff] [review]
the problem was that nsDOMStorageDB forward declares nsDOMStorage
Attachment #238864 - Attachment is obsolete: true
Attachment #238864 - Flags: superreview?(dveditz)

Updated

12 years ago
Attachment #238984 - Flags: superreview?(dveditz)
Attachment #238984 - Flags: review?(enndeakin)

Comment 24

12 years ago
I think this has also caused crashes @ PL_DHashTableEnumerate.
So #24 and #31 in the TB trunk crashes. 
(Assignee)

Updated

12 years ago
Attachment #238984 - Flags: review?(enndeakin) → review+
Comment on attachment 238984 [details] [diff] [review]
the problem was that nsDOMStorageDB forward declares nsDOMStorage

this is trivial and shouldn't need SR, please just get patches landed
Attachment #238984 - Flags: superreview?(dveditz) → approval1.8.1+
(Assignee)

Comment 26

12 years ago
Checked the last patch in, and both to 1.8
Whiteboard: [checkin needed (1.8 branch)]
sr=me, although the request had been removed by time I got here from my mail.

(In reply to comment #22)
> I think this has caused new crashes

Please file regression bugs on the crashes and mark them "blocking" this bug (so they appear in this bug's "depends on" list), and preferably add a comment here saying "filed bugs x,y,z".  Might as well nominate them for blocking FF2 so they'll get looked at.

Could this be something simple like the clearing code not handling an already-empty DOMStorage? I can't imaging there's any real-world use of storage yet, certainly not enough to be registering top crashes.
Whiteboard: [checkin needed (1.8 branch)]
Whiteboard: [checkin needed (1.8 branch)]
Marking fixed, please spin off new bugs for crashes/regressions, as dveditz noted
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED

Updated

12 years ago
Depends on: 353227

Comment 29

12 years ago
Filed Bug 353227.
You need to log in before you can comment on or make changes to this bug.