Closed Bug 968101 Opened 6 years ago Closed 6 years ago

HTTP cache v2: remove directories with low priority on background

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: michal, Assigned: michal)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #8370634 - Flags: review?(honzab.moz)
Blocks: 968106
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.
Attachment #8370634 - Flags: review?(honzab.moz) → review+
Blocks: 965089
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
Flags: needinfo?(michal.novotny)
No longer blocks: 965089
(In reply to Honza Bambas (:mayhemer) from comment #2)

This will be part of bug #968106
Flags: needinfo?(michal.novotny)
Attached patch patch v2 (obsolete) — Splinter Review
- addressed reviewer's comments
- fixed brackets in if statements according to coding style
Assignee: nobody → michal.novotny
Attachment #8370634 - Attachment is obsolete: true
Attachment #8378211 - Flags: review+
(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.
(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.)
Attached patch patch v3Splinter Review
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.
Attachment #8378211 - Attachment is obsolete: true
Attachment #8382133 - Flags: review?(honzab.moz)
Attached patch v2 -> v3 diffSplinter Review
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.
Attachment #8382133 - Flags: review?(honzab.moz) → review+
Comment on attachment 8382133 [details] [diff] [review]
patch v3

Please, review just the change in toolkit directory, see comment #8 and bug #977053.
Attachment #8382133 - Flags: review?(mak77)
Ups, looks like this landed w/o a review from mak...  Should we backout?
I can do a post-review, no worries.
just, please file a bug about the issue, and consider what landed rs=me
Marco, it's bug 977053.
Intermediate functionality bug only.
Flags: in-testsuite-
Attachment #8382133 - Flags: review?(mak77)
https://hg.mozilla.org/mozilla-central/rev/8c60c6fa67c9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.