HTTP cache v2: remove directories with low priority on background
Categories
(Core :: Networking: Cache, defect)
Tracking
()
People
(Reporter: michal, Assigned: michal)
References
Details
Attachments
(2 files, 2 obsolete files)
59.42 KB,
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
1.11 KB,
patch
|
Details | Diff | 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.
![]() |
||
Comment 1•7 years ago
|
||
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•7 years ago
|
||
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
Assignee | ||
Comment 3•7 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #2) This will be part of bug #968106
Assignee | ||
Comment 4•7 years ago
|
||
- addressed reviewer's comments - fixed brackets in if statements according to coding style
Assignee | ||
Comment 5•7 years ago
|
||
pushed to gum http://hg.mozilla.org/projects/gum/rev/26e111d9cb7c
Assignee | ||
Comment 6•7 years ago
|
||
(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•7 years ago
|
||
(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.)
Assignee | ||
Comment 8•7 years ago
|
||
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.
Assignee | ||
Comment 9•7 years ago
|
||
![]() |
||
Comment 10•7 years ago
|
||
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.
Assignee | ||
Comment 11•7 years ago
|
||
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•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=2275f8be60c9
![]() |
||
Comment 13•7 years ago
|
||
Ups, looks like this landed w/o a review from mak... Should we backout?
Comment 14•7 years ago
|
||
I can do a post-review, no worries.
Comment 15•7 years ago
|
||
just, please file a bug about the issue, and consider what landed rs=me
![]() |
||
Comment 16•7 years ago
|
||
Marco, it's bug 977053.
Updated•7 years ago
|
Assignee | ||
Comment 18•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c60c6fa67c9
Comment 19•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8c60c6fa67c9
Comment 20•11 months ago
|
||
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)
Comment 21•11 months ago
|
||
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
Comment 22•11 months ago
|
||
oh there's an issue already, didn't try to find it before, here it is: https://github.com/sandboxie-plus/Sandboxie/issues/32
Description
•