Closed Bug 968101 Opened 11 years ago Closed 11 years ago

HTTP cache v2: remove directories with low priority on background

Categories

(Core :: Networking: Cache, defect)

Product:
Core
Component:
Networking: Cache
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla30

People

(Reporter: michal, Assigned: michal)

References

Details

Attachments

(2 files, 2 obsolete files)

patch v3
(deleted), patch
mayhemer
: review+
Details | Diff | Splinter Review
v2 -> v3 diff
(deleted), patch
Details | Diff | Splinter Review
Attached patch patch v1 (obsolete) (deleted) — — 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) (deleted) — — 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 v3 (deleted) — — Splinter 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 diff (deleted) — — Splinter 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: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30

Hi. I'm getting infinite loop (due to that: while true) and this is what repeats, these 2 lines, nothing else inbetween:

[Parent 12272: Cache2 I/O]: D/cache2 CacheFileIOManager::FindTrashDirToRemove() - Returning directory trash13638
[Parent 12272: Cache2 I/O]: D/cache2 CacheFileIOManager::FindTrashDirToRemove()

it's as if that directory and the files within it are not even attempted to be removed.

This happens inside sandboxie. If I manually remove(d) those trash* dirs, the high cpu usage (25% aka one full core, of a 4-core system) stops!
I tried to find the attempt to delete anything via Process Monitor, but I can't see any. I even tried filtering by Operation SetDispositionInformationFile but nothing. So either it is trying to delete the files/dirs in some sneaky way (one of the Operations seen here where it keeps repeating: https://paste.mozilla.org/MK8ZFw3Z/raw ) OR, it isn't even trying to delete anything for some reason.

I tried outside of sandboxie but couldn't reproduce, however I'm unsure if it's because those trash* dirs exist or not. Apparently there are some trash* dirs, but I just didn't get the high cpu usage.

Tried Firefox 78.0b3 (64-bit) on Windows 7. Started to find out about the issue here: https://matrix.to/#/!OjiTSQTpPWGpfDenKT:mozilla.org/$pCmunFBOzmnembqdrwpz0gL0tM-FgjzREV2kY89SjyQ?via=mozilla.org
(I'm not even sure that link works, but it's on the official "Firefox Desktop Community" channel)

I've just confirmed that outside of sandboxie, normal firefox run did remove the trash* dirs without any trouble.
So this is clearly only happening inside sandboxie! But why!??? is sandboxie broken? or something else

oh there's an issue already, didn't try to find it before, here it is: https://github.com/sandboxie-plus/Sandboxie/issues/32

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: