968101 - HTTP cache v2: remove directories with low priority on background

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Michal Novotny [:michal]]

Attachment: patch v1

This patch implements functionality similar to nsDeleteDir in the old cache. It is used in this patch to remove files left in doomed directory after the crash. It will be also used when evicting the whole cache.

Comment 1

[Honza Bambas (:mayhemer)]

Comment on attachment 8370634 [details] [diff] [review]
patch v1

Review of attachment 8370634 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the nits fixed.

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +2317,5 @@
>  
>  nsresult
> +CacheFileIOManager::TrashDirectory(nsIFile *aFile)
> +{
> +#ifdef PR_LOGGING

PR_LOG I think

@@ +2378,5 @@
> +  LOG(("CacheIndex::TrashDirectory() - Renaming directory [leaf=%s]",
> +       leaf.get()));
> +
> +  rv = dir->MoveToNative(nullptr, leaf);
> +  NS_ENSURE_SUCCESS(rv, rv);

you may need to retry with different name?  not sure rand() is seeded here properly, maybe that'd be enough.

@@ +2458,5 @@
> +
> +nsresult
> +CacheFileIOManager::RemoveTrashInternal()
> +{
> +  LOG(("CacheIndex::RemoveTrashInternal()"));

LOG(("CacheIndex::

should be CacheFileIOManager::

(on more places)

@@ +2468,5 @@
> +  if (mShuttingDown)
> +    return NS_ERROR_NOT_INITIALIZED;
> +
> +  MOZ_ASSERT(!mTrashTimer);
> +  MOZ_ASSERT(mRemovingTrashDirs);

this may fail when we get here before it gets set on the invoking thread.
set the flag before dispatch, drop it when dispatch fails.

@@ +2472,5 @@
> +  MOZ_ASSERT(mRemovingTrashDirs);
> +
> +  if (!mTreeCreated) {
> +    rv = CreateCacheTree();
> +    if (NS_FAILED(rv)) return rv;

return on a new line

@@ +2475,5 @@
> +    rv = CreateCacheTree();
> +    if (NS_FAILED(rv)) return rv;
> +  }
> +
> +  mRemovingTrashDirs = false;

I don't understand why you drop the flag here ; when we get here, isn't it ensured that we loop over all the dirs already?  why should another invocation of this loop be allowed after this point?

@@ +2481,5 @@
> +  TimeStamp start;
> +
> +  while (true) {
> +    if (start.IsNull()) {
> +      start = TimeStamp::Now();

lores

@@ +2483,5 @@
> +  while (true) {
> +    if (start.IsNull()) {
> +      start = TimeStamp::Now();
> +    } else {
> +      TimeDuration elapsed = TimeStamp::Now() - start;

lores

@@ +2484,5 @@
> +    if (start.IsNull()) {
> +      start = TimeStamp::Now();
> +    } else {
> +      TimeDuration elapsed = TimeStamp::Now() - start;
> +      if (elapsed.ToMilliseconds() >= kRemoveTrashLoopLimit) {

static const

@@ +2512,5 @@
> +
> +      continue; // check elapsed time
> +    }
> +
> +    // Remove the trash directory if we don't have enumerator

why? (add a comment)

@@ +2517,5 @@
> +    if (!mTrashDirEnumerator) {
> +      rv = mTrashDir->Remove(false);
> +      if (NS_FAILED(rv)) {
> +        // This shouldn't normally happen, but if it does, we should try to
> +        // remove all other trash directories.

why? (add a comment)

@@ +2541,5 @@
> +      file->IsDirectory(&isDir);
> +      if (isDir) {
> +        NS_WARNING("Found a directory in a trash directory! It will be removed "
> +                   "recursively, but this can block IO thread for a while!");
> +#ifdef PR_LOGGING

PR_LOG as well here.. or am I wrong?  not sure..

@@ +2565,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +CacheFileIOManager::FindTrashDir()

Maybe FindNextTrashDir() ?

@@ +2602,5 @@
> +
> +    if (leafName.Length() < strlen(kTrashDir))
> +      continue;
> +
> +    if (PL_strncmp(leafName.get(), kTrashDir, strlen(kTrashDir)))

leafName.BeginsWith(NS_LITERAL_CSTRING(kTrashDir))

@@ +2615,5 @@
> +    mTrashDir = file;
> +    return NS_OK;
> +  }
> +
> +  mFailedTrashDirs.Clear();

just add a comment that here all trashes are deleted (the enumerator didn't find any more items)

@@ +2886,2 @@
>  {
>    nsresult rv;

assert !main thread or !!io thread

::: netwerk/cache2/CacheFileIOManager.h
@@ +343,5 @@
> +  bool                                 mRemovingTrashDirs;
> +  nsCOMPtr<nsITimer>                   mTrashTimer;
> +  nsCOMPtr<nsIFile>                    mTrashDir;
> +  nsCOMPtr<nsIDirectoryEnumerator>     mTrashDirEnumerator;
> +  nsTArray<nsCString>                  mFailedTrashDirs;

comment what this is would be good (for all members here, but not in this bug..)
I would a little bit more prefer a hashset (nsTHashtable<nsCStringHashKey> mFailed...;  It has bool Contains() method.

Comment 2

[Honza Bambas (:mayhemer)]

Michal: 
- is this code going to be used from nsICacheStorageService.clear() to wipe all the cache data?  (in this or a new bug?)
- if yes, we must rename the entries dir before we attempt to open any entry (so on the OPEN_PRIORITY or generic XPCOM dispatch level, which is run first) to make sure the entries are not found (with the cache API) after nsICacheStorageService.clear() exits

Comment 3

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #2)

This will be part of bug #968106

Comment 4

[Michal Novotny [:michal]]

Attachment: patch v2

- addressed reviewer's comments
- fixed brackets in if statements according to coding style

Comment 5

[Michal Novotny [:michal]]

pushed to gum http://hg.mozilla.org/projects/gum/rev/26e111d9cb7c

Comment 6

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #1)
> @@ +2378,5 @@
> > +  LOG(("CacheIndex::TrashDirectory() - Renaming directory [leaf=%s]",
> > +       leaf.get()));
> > +
> > +  rv = dir->MoveToNative(nullptr, leaf);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> 
> you may need to retry with different name?  not sure rand() is seeded here
> properly, maybe that'd be enough.

Why? Above this code, we loop until we find a trash name that doesn't exist.


> @@ +2468,5 @@
> > +  if (mShuttingDown)
> > +    return NS_ERROR_NOT_INITIALIZED;
> > +
> > +  MOZ_ASSERT(!mTrashTimer);
> > +  MOZ_ASSERT(mRemovingTrashDirs);
> 
> this may fail when we get here before it gets set on the invoking thread.
> set the flag before dispatch, drop it when dispatch fails.

No, it cannot. Both, StartRemovingTrash() and RemoveTrashInternal(), are run on IO thread. The reason why we dispatch another event is that StartRemovingTrash() is run on unpredictible priority level but definitely not on EVICT level and we want to remove the files on EVICT level.


> @@ +2475,5 @@
> > +    rv = CreateCacheTree();
> > +    if (NS_FAILED(rv)) return rv;
> > +  }
> > +
> > +  mRemovingTrashDirs = false;
> 
> I don't understand why you drop the flag here ; when we get here, isn't it
> ensured that we loop over all the dirs already?  why should another
> invocation of this loop be allowed after this point?

I added a comment.

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #6)
> (In reply to Honza Bambas (:mayhemer) from comment #1)
> > @@ +2378,5 @@
> > > +  LOG(("CacheIndex::TrashDirectory() - Renaming directory [leaf=%s]",
> > > +       leaf.get()));
> > > +
> > > +  rv = dir->MoveToNative(nullptr, leaf);
> > > +  NS_ENSURE_SUCCESS(rv, rv);
> > 
> > you may need to retry with different name?  not sure rand() is seeded here
> > properly, maybe that'd be enough.
> 
> Why? Above this code, we loop until we find a trash name that doesn't exist.

Ah, then it's OK.  I somehow missed that.

> 
> 
> > @@ +2468,5 @@
> > > +  if (mShuttingDown)
> > > +    return NS_ERROR_NOT_INITIALIZED;
> > > +
> > > +  MOZ_ASSERT(!mTrashTimer);
> > > +  MOZ_ASSERT(mRemovingTrashDirs);
> > 
> > this may fail when we get here before it gets set on the invoking thread.
> > set the flag before dispatch, drop it when dispatch fails.
> 
> No, it cannot. Both, StartRemovingTrash() and RemoveTrashInternal(), are run
> on IO thread. The reason why we dispatch another event is that
> StartRemovingTrash() is run on unpredictible priority level but definitely
> not on EVICT level and we want to remove the files on EVICT level.

I completely overlooked this.  Should be better documented in the code.

> 
> 
> > @@ +2475,5 @@
> > > +    rv = CreateCacheTree();
> > > +    if (NS_FAILED(rv)) return rv;
> > > +  }
> > > +
> > > +  mRemovingTrashDirs = false;
> > 
> > I don't understand why you drop the flag here ; when we get here, isn't it
> > ensured that we loop over all the dirs already?  why should another
> > invocation of this loop be allowed after this point?
> 
> I added a comment.

Thanks.  I was looking at it, now it's clear (probably also enough for the previous case, it's the same issue I think.)

Comment 8

[Michal Novotny [:michal]]

Attachment: patch v3

This patch fixes few bugs present in patch v2:

- fixes comparing file name with kTrashDir prefix
- starts removing trash at startup, so we continue to remove trash when it was interrupted on FF shutdown
- disables test_pref_interval.js because it registers JS implementation of timer which causes crash when some other code uses the timer off the main thread. Asking for a new review mainly because of this change.

Comment 9

[Michal Novotny [:michal]]

Attachment: v2 -> v3 diff

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8382133 [details] [diff] [review]
patch v3

Review of attachment 8382133 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab only for the networking parts.

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +3076,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    mTreeCreated = true;
> +
> +  StartRemovingTrash();

do you think this is really the best place?  what about OnProfile?  We are protected with the 30 secs delay if called too soon after startup.  I've seen CreateCacheTree() being called not only after startup.

::: toolkit/components/places/tests/expiration/xpcshell.ini
@@ +16,5 @@
>  [test_notifications_onDeleteVisits.js]
>  [test_outdated_analyze.js]
>  [test_pref_interval.js]
> +# Crashes when timer is used on non-main thread due to JS implemetation in this test
> +skip-if = "JS implementation of nsITimer"

Nit: the preferred style is (against my personal liking..) as:

# comment why disabled
skip-if = true


You need a review from a module peer.  I cannot review this change.

Comment 11

[Michal Novotny [:michal]]

Comment on attachment 8382133 [details] [diff] [review]
patch v3

Please, review just the change in toolkit directory, see comment #8 and bug #977053.

Comment 12

[Honza Bambas (:mayhemer)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=2275f8be60c9

Comment 13

[Honza Bambas (:mayhemer)]

Ups, looks like this landed w/o a review from mak...  Should we backout?

Comment 14

[Marco Bonardo [::mak]]

I can do a post-review, no worries.

Comment 15

[Marco Bonardo [::mak]]

just, please file a bug about the issue, and consider what landed rs=me

Comment 16

[Honza Bambas (:mayhemer)]

Marco, it's bug 977053.

Comment 17

[Honza Bambas (:mayhemer)]

Intermediate functionality bug only.

Comment 18

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c60c6fa67c9

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8c60c6fa67c9