Closed Bug 983582 Opened 6 years ago Closed 6 years ago

HTTP cache v2: special handles are not released until shutdown

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: michal, Assigned: michal)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch fix (obsolete) — Splinter Review
CacheFileIOManager::mSpecialHandles holds a strong reference to the handles, so they aren't released until shutdown.
Summary: special handles are not released until shutdown → HTTP cache v2: special handles are not released until shutdown
Comment on attachment 8391116 [details] [diff] [review]
fix

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

I think you need to change the removal condition in CacheFileIOManager::CloseHandleInternal.  You shouldn't touch special handles when shutting down as well we do for 'normal' handles.
Attachment #8391116 - Flags: review?(honzab.moz) → feedback+
Also, needs cache2=on and off try pushes with ASAN build.
Attached patch patch v2Splinter Review
try push:
cache2=off https://tbpl.mozilla.org/?tree=Try&rev=e57a0498ade9
cache2=on  https://tbpl.mozilla.org/?tree=Try&rev=3616c0e16f7d
Assignee: nobody → michal.novotny
Attachment #8391116 - Attachment is obsolete: true
Attachment #8395473 - Flags: review?(honzab.moz)
The patch looks good, but xpcshell tests are regularly failing.  Seems like it's not happening on gum tho.
Note that test_beaconFrame.html and browser_dbg_stack-02.js are known failures, both with fixes.
The same failures on m-c are also with cache2=on without this patch:

https://tbpl.mozilla.org/?tree=Try&rev=ddc6cc646997

I don't understand why gum is green...
I think I've see those xpcshell failures somewhere already...  I will remerge gum and see.
Comment on attachment 8395473 [details] [diff] [review]
patch v2

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

Thanks
Attachment #8395473 - Flags: review?(honzab.moz) → review+
They all should not be run with the new cache backend, but they call newCacheBackEndUsed() before do_get_profile() which constantly returns a wrong value on m-c. But it is not clear to me, why this works on gum.
(In reply to Michal Novotny (:michal) from comment #10)
> They all should not be run with the new cache backend, but they call
> newCacheBackEndUsed() before do_get_profile() which constantly returns a
> wrong value on m-c. But it is not clear to me, why this works on gum.

The patch that enables cache2 on gum also switches the default value const in CacheObserver.  That's why.  But I had fixed this in the past, so that only the pref change should cover everything correctly.  I'll look at that.
https://hg.mozilla.org/mozilla-central/rev/fe1fc50bad54
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.