Closed Bug 600982 Opened 9 years ago Closed 9 years ago

Clear DOM storage entries for a domain when using the Forget about this site feature

Categories

(Toolkit :: Data Sanitization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: ehsan, Assigned: ehsan)

Details

(Keywords: privacy)

Attachments

(1 file, 3 obsolete files)

DOM storage listens to cookie-changed "cleared" notifications to clear itself when all cookies are cleared (what happends in the CRH dialog for example) but it doesn't handle clearing itself when clearing data for a specific domain.  We should fix that.

Note to myself: listen for "browser:purge-domain-data" in nsDOMStorage.
blocking2.0: --- → ?
blocking2.0: ? → final+
So it seems like the docshell is what stores a list of storage objects based on domain names.  Is that the best place for the browser:purge-domain-data to live?
Honza might know.
No, the code you need to modify is here:
http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/nsDOMStorage.cpp#347

You will add (schematically):

if (!strcmp(aTopic, "browser:purge-domain-data"))
{
  // clears cache of each storage
  mStorages.EnumerateEntries(ClearStorage, nsnull); 

  nsDOMStorage::InitDB();

  // the second arg means to also delete all subdomains, what I passume you want to
  nsDOMStorage::gStorageDB->RemoveOwner(aData, true); 
}
Clearing the cache that way will clear it for all domains, right?  Is that fine?
I don't understand what you mean.
(In reply to comment #3)
In this code:

> if (!strcmp(aTopic, "browser:purge-domain-data"))
> {
>   // clears cache of each storage
>   mStorages.EnumerateEntries(ClearStorage, nsnull); 

mStorages is not domain specific, right?  So when I clear it, do we clear the cache for all domains?  Is this OK?
Hmm... you are right.  This code is a mess...

You have to clear cache of storages for those StringBeginsWith(ReverseString(domainBeingCleared), storage->GetScopeDBKey()) is true.
OS: Mac OS X → All
Hardware: x86 → All
Attached patch Patch (v1) (obsolete) — Splinter Review
Attachment #496419 - Flags: review?(honzab.moz)
Comment on attachment 496419 [details] [diff] [review]
Patch (v1)

ClearStorageIfDomainMatches is unfortunately wrong.  Also, please never use NS_LossyConvertUTF16toASCII on domains!  Sorry, I had to me more clear on how the code have to look like.  To explain:

GetScopeDBKey() is derived this way:
localStorage:
  - REVERSE(principal.asciiHost) + "." + "http" [ + ":portnumber" ]
  - principal.asciiHost is in ACE
globalStorage[domain]:
  - REVERSE(IDNService.convertUTF8toACE(UTF16toUTF8(domain))) + "."

There are existing APIs used to derive the key: nsDOMStorageDBWrapper::CreateDomainScopeDBKey and nsDOMStorageDBWrapper::CreateOriginScopeDBKey.  The one you are interested in is CreateDomainScopeDBKey.  It creates the correct key for you, simple reverse is not enough.  It expects the domain in ACE already, so you have to do the ACE conversion before calling on it.  I don't expect that it is not done before [1] where from the purge API is called from.

I can see this bug on more places using RemoveOwner and RemoveOwners (similar methods, but has different APIs - one nsCString and other nsString, what a mess!), general check would be good, let's fix them and create proper tests in a different bug (feel free to file and work on it ;) ).
Attachment #496419 - Flags: review?(honzab.moz)
And BTW: you are using StringBeginsWith wrong - the arguments are expected in reverse order, the first is a string that has to begin with the second argument string.  You will always get false for your call.  

Your test is passing because accessing 'length' property of the storage reloads the cache.  You have to change it to check for presence of the keys, not for length to have a really good check.  Only after that just for sure check the length property as well.

Again, sorry, this code is a mess!
Attached patch Patch (v2) (obsolete) — Splinter Review
Well, my previous patch was a fiasco!  I was foolish enough to think that everything is fine because the test was passing.  Hope this one is better!
Attachment #496419 - Attachment is obsolete: true
Attachment #496684 - Flags: review?(honzab.moz)
(In reply to comment #9)
> I can see this bug on more places using RemoveOwner and RemoveOwners (similar
> methods, but has different APIs - one nsCString and other nsString, what a
> mess!), general check would be good, let's fix them and create proper tests in
> a different bug (feel free to file and work on it ;) ).

I'm not quite sure what kind of bug you want me to file here...
Whiteboard: [has patch][needs review honza]
Comment on attachment 496684 [details] [diff] [review]
Patch (v2)

>+++ b/browser/components/privatebrowsing/test/unit/do_test_removeDataFromDomain.js

>+  pb.removeDataFromDomain("mozilla.org");
>+
>+  do_check_throws(function() s[0].key(0));
>+  do_check_throws(function() s[0].key(0));
>+  do_check_eq(s[2].key(0), "test");
>+  do_check_eq(s[2].getItem("test"), "value2");

Even better check is to, in this order:
1. check presence of "test" key with getItem first on each storage
2. check length on each storage

This will check that mItems has been properly updated and then that the database is also updated correctly as length re-caches the items.

>+++ b/dom/src/storage/nsDOMStorage.cpp

>+static PLDHashOperator
>+ClearStorageIfDomainMatches(nsDOMStorageEntry* aEntry, void* userArg)
>+{
>+  nsACString* aKey = static_cast<nsACString*> (userArg);

Rather cast to nsCAutoString* here?

>@@ -425,16 +436,38 @@ nsDOMStorageManager::Observe(nsISupports
>+    nsCOMPtr<nsIIDNService> converter = do_GetService(NS_IDNSERVICE_CONTRACTID);
>+    NS_ENSURE_TRUE(converter, NS_ERROR_FAILURE);

This must not be fatal.  See [1] for details.

>+    DOMStorageImpl::gStorageDB->RemoveOwner(NS_LossyConvertUTF16toASCII(aData), PR_TRUE);

Why not to use the ace conversion here?  That is the bug I refer to, wrongly formatted domain is passed to RemoveOwner(s) also on other places.


[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsStandardURL.cpp#1070
Attachment #496684 - Flags: review?(honzab.moz)
Whiteboard: [has patch][needs review honza]
Attached patch Patch (v3) (obsolete) — Splinter Review
So I made the test changes, but the test fails currently.  Here's something very strange: if I do:

var x = s[0].getItem("test");
do_check_eq(x, null);

The test would fail (x would be "value0").  If I do:

var x = s[0].getItem("test");
dump(x);
do_check_eq(x, null);

The test passes!!!

Do you have any ideas on what might be going on here?
Attachment #496684 - Attachment is obsolete: true
Attachment #497440 - Flags: review?(honzab.moz)
Comment on attachment 497440 [details] [diff] [review]
Patch (v3)

>+++ b/browser/components/privatebrowsing/test/unit/do_test_removeDataFromDomain.js
>+  do_check_eq(s[0].getItem("test"), null);
>+  do_check_eq(s[0].length, 0);
>+  do_check_eq(s[1].getItem("test"), null);
>+  do_check_eq(s[1].length, 0);
>+  do_check_eq(s[2].key(0), "test");
>+  do_check_eq(s[2].getItem("test"), "value2");

Same check for s[2] as for s[0] and s[1] please.  Check for key(0) == "test" is not bad, but getItem should always be checked and as the first.


>+++ b/dom/src/storage/nsDOMStorage.cpp
>@@ -425,16 +436,40 @@ nsDOMStorageManager::Observe(nsISupports
>+    // Convert the domain name to the ACE format
>+    nsCOMPtr<nsIIDNService> converter = do_GetService(NS_IDNSERVICE_CONTRACTID);
>+    if (!converter) {
>+      return NS_OK;
>+    }
>+
>+    nsCAutoString aceDomain;
>+    nsresult rv = converter->ConvertUTF8toACE(NS_ConvertUTF16toUTF8(aData), aceDomain);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+

I had to be, again, clearer on what I wanted to suggest:

Please use the following code, copied from nsStandardURL class, it is also used for converting domain argument for globalStorage:

    if (gIDN) {
        nsresult rv;
        rv = gIDN->ConvertUTF8toACE(Host(), result);
        if (NS_SUCCEEDED(rv)) {
            mHostA = ToNewCString(result);
            return NS_OK;
        }
        NS_WARNING("nsIDNService::ConvertUTF8toACE failed");
    }

    // something went wrong... guess all we can do is URL escape :-/
    NS_EscapeURL(Host(), esc_OnlyNonASCII | esc_AlwaysCopy, result);
    return NS_OK;


>+    DOMStorageImpl::gStorageDB->RemoveOwner(key, PR_TRUE);

'key' is the reversed scope key.  RemoveOwner wants the ACE representation of the domain, it creates the reversed key from it internally.  Please, use aceDomain or whatever result of URL conversion you have got after the previous review bit addressed.  This is causing your test failure because the items are not deleted from the database.  I locally verified that.
Attachment #497440 - Flags: review?(honzab.moz) → review-
Attached patch Patch (v4)Splinter Review
This one should finally be good enough!  Thanks for your patience!  :-)
Attachment #497440 - Attachment is obsolete: true
Attachment #497734 - Flags: review?(honzab.moz)
Whiteboard: [has patch][needs review honza]
Comment on attachment 497734 [details] [diff] [review]
Patch (v4)

Yep, this is it.

r=honzab

It comes to my mind that also have a test for an international name would be good, but let's do it in a different bug (and probably also Gecko version).
Attachment #497734 - Flags: review?(honzab.moz) → review+
(In reply to comment #17)
> It comes to my mind that also have a test for an international name would be
> good, but let's do it in a different bug (and probably also Gecko version).

Sure, filed bug 619441 for investigation.
Whiteboard: [has patch][needs review honza] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/a9928aa3aa68
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → Firefox 4.0b9
Component: Private Browsing → Forget About Site
Product: Firefox → Toolkit
Target Milestone: Firefox 4.0b9 → ---
You need to log in before you can comment on or make changes to this bug.