983582 - HTTP cache v2: special handles are not released until shutdown

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Michal Novotny [:michal]]

Attachment: fix

CacheFileIOManager::mSpecialHandles holds a strong reference to the handles, so they aren't released until shutdown.

Comment 1

[Michal Novotny [:michal]]

Comment on attachment 8391116 [details] [diff] [review]
fix

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

Comment 2

[Honza Bambas (:mayhemer)]

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.

Comment 3

[Honza Bambas (:mayhemer)]

Also, needs cache2=on and off try pushes with ASAN build.

Comment 4

[Michal Novotny [:michal]]

Attachment: patch v2

try push:
cache2=off https://tbpl.mozilla.org/?tree=Try&rev=e57a0498ade9
cache2=on  https://tbpl.mozilla.org/?tree=Try&rev=3616c0e16f7d

Comment 5

[Honza Bambas (:mayhemer)]

The patch looks good, but xpcshell tests are regularly failing.  Seems like it's not happening on gum tho.

Comment 6

[Honza Bambas (:mayhemer)]

Note that test_beaconFrame.html and browser_dbg_stack-02.js are known failures, both with fixes.

Comment 7

[Michal Novotny [:michal]]

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...

Comment 8

[Honza Bambas (:mayhemer)]

I think I've see those xpcshell failures somewhere already...  I will remerge gum and see.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 8395473 [details] [diff] [review]
patch v2

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

Thanks

Comment 10

[Michal Novotny [:michal]]

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.

Comment 11

[Honza Bambas (:mayhemer)]

(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.

Comment 12

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe1fc50bad54

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fe1fc50bad54