Closed Bug 1273875 Opened 4 years ago Closed 4 years ago

During shutdown leak as much as we can from HTTP cache

Categories

(Core :: Networking: Cache, defect)

46 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

Details

(Whiteboard: [necko-active])

Attachments

(2 files)

The plan is to leave the list of active entries and the pools unfreed similarly to https://bugzilla.mozilla.org/attachment.cgi?id=8751204&action=diff#a/netwerk/cache2/CacheFileUtils.cpp_sec2
Attachment #8753832 - Flags: review?(michal.novotny)
Comment on attachment 8753832 [details] [diff] [review]
v1 (leak hashtables on !NS_FREE_PERMANENT_DATA)

With this change we won't release CacheEntry and the associated CacheFile. So if the entry is complete but the metadata has not been written via CacheFileIOManager::ScheduleMetadataWrite then we'll leave invalid file on the disk, right?
Flags: needinfo?(honzab.moz)
Flags: needinfo?(honzab.moz)
Whiteboard: [necko-active]
(In reply to Michal Novotny (:michal) from comment #2)
> Comment on attachment 8753832 [details] [diff] [review]
> v1 (leak hashtables on !NS_FREE_PERMANENT_DATA)
> 
> With this change we won't release CacheEntry and the associated CacheFile.
> So if the entry is complete but the metadata has not been written via
> CacheFileIOManager::ScheduleMetadataWrite then we'll leave invalid file on
> the disk, right?

Ups, didn't want to drop the ni flag.

Yes, and we already do that after bug 913822 and its followups.  As I understand, this is a correct thing to do.
Attachment #8753832 - Flags: review?(michal.novotny) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/61d37541894f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8753832 [details] [diff] [review]
v1 (leak hashtables on !NS_FREE_PERMANENT_DATA)

Approval Request Comment
[Feature/regressing bug #]: http cache v2
[User impact if declined]: shutdown hang/crash
[Describe test coverage new/current, TreeHerder]: on m-c for a week, heavily used code
[Risks and why]: extremely low, this just leaks memory (when we don't do leak checking) during shutdown
[String/UUID change made/needed]: none
Attachment #8753832 - Flags: approval-mozilla-aurora?
Attachment #8753832 - Flags: approval-mozilla-aurora?
Approval Request Comment, see comment 6
Attachment #8759606 - Flags: approval-mozilla-aurora?
mayhemer, will this still apply to beta 48? This missed uplift before the last merge.
Flags: needinfo?(honzab.moz)
Comment on attachment 8759606 [details] [diff] [review]
[m-a version] v1 (leak hashtables on !NS_FREE_PERMANENT_DATA)

Switching approval flag to 48 beta.
Attachment #8759606 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #8)
> mayhemer, will this still apply to beta 48? This missed uplift before the
> last merge.

It's very likely it will still apply.  I cannot verify by applying the patch, I don't have beta checked out, but looking at the changes and beta state tells me it's OK to go.
Flags: needinfo?(honzab.moz)
Comment on attachment 8759606 [details] [diff] [review]
[m-a version] v1 (leak hashtables on !NS_FREE_PERMANENT_DATA)

May help with shutdown hangs, let's uplift it.
Attachment #8759606 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.