Any existing call to nsCacheEntryDescriptor::Doom does not need the entry to be doomed synchronously. So we can make this call asynchronous without changing the API.

(In reply to Michal Novotny (:michal) from comment #0) > Any existing call to nsCacheEntryDescriptor::Doom does not need the entry to > be doomed synchronously. So we can make this call asynchronous without > changing the API. Changing the method from synchronous to asynchronous is a big change and IMO such a big change should result in a new name for the method and a new UUIDs for the interface and sub-interfaces.

(In reply to Brian Smith (:bsmith) from comment #1) > Changing the method from synchronous to asynchronous is a big change and IMO > such a big change should result in a new name for the method and a new UUIDs > for the interface and sub-interfaces. I think new UUIDs might be a good idea, but I think changing the name (when our intent is to make all uses of this function asynchronous anyway) just sounds like asking for a patch bigger and more likely to miss something than is strictly necessary. I would prefer to see us keep this name for the asynchronous version, and if we find a place where we require a synchronous version, make a new function called SynchDoom (but ONLY if we know we require that synchronous version, no use exposing it if we don't need it)

Some addons require the Doom method to be synchronous; if we make it asynchronous, then we will introduce race conditions in them. LiveHTTPHeaders is one such addon; it dooms an entry and then loads the URL corresponding to that entry, with the expectation that the entry is doomed before the load [1]. These addons need to change, but we should give them some time to update their code, by supporting the old synchronous doom in addition to a new asynchronous doom. In the LiveHTTPHeaders case, it seems like the addon is probably technically racy already. However, I bet most of the time it works fine. If we change Doom to be asynchronous, it will almost always fail to do what it intends to do. [1] https://mxr.mozilla.org/addons/source/3829/chrome/content/LiveHTTPHeaders.js#581

We may not need to do this, and/or the priority may become much lower, once bug 722034 and similar bugs (yet to be filed) are fixed, because most cache accesses (including calls to Doom) will happen on the cache service thread.

See Comment 3. We cannot make Doom() async by default for existing addons. I clarified the summary.

Tryserver looks good https://tbpl.mozilla.org/?tree=Try&rev=a520740b0f4e

Comment on attachment 650927 [details] [diff] [review] fix Review of attachment 650927 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the one question below addressed ::: netwerk/cache/nsICacheEntryDescriptor.idl @@ +122,5 @@ > + * Asynchronously doom an entry. Listener will be notified about the status > + * of the operation. Null may be passed if caller doesn't care about the > + * result. > + */ > + void asyncDoom(in nsICacheListener listener); Do we have any plans to actually use the listener? I notice you pass nullptr everywhere, and it seems odd to me to have an argument we never use. Or are you intending this for extensions to be able to move forward into the new, async cache world? If we have no plan for the listener (and don't think addons should care), I propose we trash the argument, for cleanliness.

(In reply to Nick Hurley [:hurley] from comment #7) > Do we have any plans to actually use the listener? I notice you pass nullptr > everywhere, and it seems odd to me to have an argument we never use. Or are > you intending this for extensions to be able to move forward into the new, > async cache world? > > If we have no plan for the listener (and don't think addons should care), I > propose we trash the argument, for cleanliness. I wanted to have a similar method to nsICacheSession::doomEntry(). And although we pass nullptr everywhere now, I think there could be cases when we would need to be notified when the entry is doomed.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8e0ecc8a4f3e - https://tbpl.mozilla.org/php/getParsedLog.php?id=14546953&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=14546342&tree=Mozilla-Inbound are previously-unseen mochitest-1 leaks that include an nsCacheEntryDescriptor; https://tbpl.mozilla.org/php/getParsedLog.php?id=14205502&tree=Try and https://tbpl.mozilla.org/php/getParsedLog.php?id=14209613&tree=Try are that same previously-unseen leak, on your try push.

This is likely also the cause of bug 784253 (another new orange), though I filed it as its own bug.

I've changed nsCacheService::ClearActiveEntries() so that it doesn't deactivate entries while it enumerates the hash table. I wasn't able to manually test the patch because I can't reproduce the case when we have any active entry in nsCacheService::Shutdown(). Pushed to try server https://tbpl.mozilla.org/?tree=Try&rev=d836b5b3b76f

Comment on attachment 658060 [details] [diff] [review] patch v2 Review of attachment 658060 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, assuming try passes.

I've finaly found the cause of the leak. nsCacheEntryDescriptor::AsyncDoom() is called after the cache service is shut down, nsCacheService::DispatchToCacheIOThread() fails and the event leaks. The new patch fixes this. It seems that there is some bug in media cache code. The only entry that we leak this way while running mochitests is browser/base/content/test/video.ogg that is used in test browser/base/content/test/test_contextmenu.html. It can't be reproduced consistently, especially it's hard to trigger that with debugger attached. The channel for the video is suspended and resumed several times by the media cache but at the end the resume is missing and the channel stays suspended. When the test finishes the channel is canceled but not resumed. In my case it is not resumed until shutdown: #0 mozilla::net::nsHttpChannel::Resume (this=0xe1b7cc00) at /opt/moz/hg/netwerk/protocol/http/nsHttpChannel.cpp:4329 #1 0x023410a9 in mozilla::ChannelMediaResource::PossiblyResume (this=0xe14e7000) at /opt/moz/hg/content/media/MediaResource.cpp:923 #2 0x02340196 in mozilla::ChannelMediaResource::CloseChannel (this=0xe14e7000) at /opt/moz/hg/content/media/MediaResource.cpp:563 #3 0x0233fe43 in mozilla::ChannelMediaResource::Close (this=0xe14e7000) at /opt/moz/hg/content/media/MediaResource.cpp:508 #4 0x02359112 in nsBuiltinDecoder::Shutdown (this=0xe1727230) at /opt/moz/hg/content/media/nsBuiltinDecoder.cpp:277 #5 0x0235ab93 in nsBuiltinDecoder::Observe (this=0xe1727230, aSubjet=0xf755b804, aTopic=0x43b0c08 "xpcom-shutdown", someData=0x0) at /opt/moz/hg/content/media/nsBuiltinDecoder.cpp:715 #6 0x02ed2eff in nsObserverList::NotifyObservers (this=0xe5aec2a0, aSubject=0xf755b804, aTopic=0x43b0c08 "xpcom-shutdown", someData=0x0) at /opt/moz/hg/xpcom/ds/nsObserverList.cpp:99 #7 0x02ed49a2 in nsObserverService::NotifyObservers (this=0xf6123a00, aSubject=0xf755b804, aTopic=0x43b0c08 "xpcom-shutdown", someData=0x0) at /opt/moz/hg/xpcom/ds/nsObserverService.cpp:149 #8 0x02ebba74 in mozilla::ShutdownXPCOM (servMgr=0xf755b804) at /opt/moz/hg/xpcom/build/nsXPComInit.cpp:581 #9 0x02ebb865 in NS_ShutdownXPCOM_P (servMgr=0xf755b804) at /opt/moz/hg/xpcom/build/nsXPComInit.cpp:541 #10 0x014bbea8 in ScopedXPCOMStartup::~ScopedXPCOMStartup (this=0xf755f218, __in_chrg=<value optimized out>) at /opt/moz/hg/toolkit/xre/nsAppRunner.cpp:1113 #11 0x014c5662 in XREMain::XRE_main (this=0xfff2cc30, argc=5, argv=0xfff2ef04, aAppData=0x8075040) at /opt/moz/hg/toolkit/xre/nsAppRunner.cpp:3934 #12 0x014c57fa in XRE_main (argc=5, argv=0xfff2ef04, aAppData=0x8075040, aFlags=0) at /opt/moz/hg/toolkit/xre/nsAppRunner.cpp:3988 #13 0x0804a0cd in do_main (argc=5, argv=0xfff2ef04) at /opt/moz/hg/browser/app/nsBrowserApp.cpp:174 #14 0x0804a39b in main (argc=5, argv=0xfff2ef04) at /opt/moz/hg/browser/app/nsBrowserApp.cpp:279