352704 - Should clear DOM storage when Clear Private Data is used

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Neil Deakin]

Comment 1

[Mike Connor [:mconnor]]

I'm guessing this would clear along with cookies?

Comment 2

[Daniel Veditz [:dveditz]]

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.

Comment 3

[Mike Connor [:mconnor]]

We cannot touch l10n, so cookies it is (for now)

Neil, do you have an ETA?

Comment 4

[Neil Deakin]

Attachment: Clear DOM Storage when cookies are deleted

The only issue is that the Cookies option is disabled when there are no cookies.

Comment 5

[Neil Deakin]

Attachment: Always enable clear all cookies options in Clear Private Data and Cookies dialogs

Comment 6

[Mike Connor [: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!

Comment 7

[Johnny Stenback (:jst)]

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.

Comment 8

[Daniel Veditz [:dveditz]]

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

sr=dveditz

Comment 9

[Daniel Veditz [:dveditz]]

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

sr=dveditz

Comment 10

[Daniel Veditz [:dveditz]]

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

[Mike Schroepfer]

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

a=schrep - thanks Neil, JST, and DVeditz

Comment 12

[Mike Schroepfer]

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

Comment 13

[Neil Deakin]

Checked into trunk

Comment 14

[Mike Schroepfer]

Please land on 1.8 branch...

Comment 15

[Oleg Romashin (:romaxa)]

This checkin:
http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=1158355510&maxdate=1158355800&who=enndeakin%25sympatico.ca

has broken xulrunner embedding-profile=minimal configuration, where MOZ_STORAGE is not defined....

Comment 16

[Oleg Romashin (:romaxa)]

Attachment: Proposed fix for building xulrunner minimal configuration

Comment 17

[Mike Connor [: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?

Comment 18

[Daniel Veditz [:dveditz]]

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.

Comment 19

[Neil Deakin]

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?

Comment 20

[Oleg Romashin (:romaxa)]

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

Comment 21

[Oleg Romashin (:romaxa)]

Attachment: Brief html from tinderbox

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

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

[timeless]

Attachment: the problem was that nsDOMStorageDB forward declares nsDOMStorage

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

I think this has also caused crashes @ PL_DHashTableEnumerate.
So #24 and #31 in the TB trunk crashes. 

Comment 25

[Mike Connor [:mconnor]]

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

Comment 26

[Neil Deakin]

Checked the last patch in, and both to 1.8

Comment 27

[Daniel Veditz [:dveditz]]

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.

Comment 28

[Mike Connor [:mconnor]]

Marking fixed, please spin off new bugs for crashes/regressions, as dveditz noted

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Filed Bug 353227.