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

Status: RESOLVED FIXED

(Toolkit :: Data Sanitization, defect)

Description

[(no longer active)]

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.

Comment 1

[(no longer active)]

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?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Honza might know.

Comment 3

[Honza Bambas (:mayhemer)]

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); 
}

Comment 4

[(no longer active)]

Clearing the cache that way will clear it for all domains, right?  Is that fine?

Comment 5

[Honza Bambas (:mayhemer)]

I don't understand what you mean.

Comment 6

[(no longer active)]

(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?

Comment 7

[Honza Bambas (:mayhemer)]

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.

Comment 8

[(no longer active)]

Attachment: Patch (v1)

Comment 9

[Honza Bambas (:mayhemer)]

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 ;) ).

Comment 10

[Honza Bambas (:mayhemer)]

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!

Comment 11

[(no longer active)]

Attachment: Patch (v2)

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!

Comment 12

[(no longer active)]

(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...

Comment 13

[Honza Bambas (:mayhemer)]

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

Comment 14

[(no longer active)]

Attachment: Patch (v3)

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?

Comment 15

[Honza Bambas (:mayhemer)]

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.

Comment 16

[(no longer active)]

Attachment: Patch (v4)

This one should finally be good enough!  Thanks for your patience!  :-)

Comment 17

[Honza Bambas (:mayhemer)]

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).

Comment 18

[(no longer active)]

(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.

Comment 19

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/a9928aa3aa68