If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Make an asynchronous variant of nsCacheEntryDescriptor::Doom

RESOLVED FIXED in mozilla18

Status

()

Core
Networking: Cache
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: michal, Assigned: michal)

Tracking

11 Branch
mozilla18
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
Summary: Make nsCacheEntryDescriptor::Doom asynchronous → Make an asynchronous variant of nsCacheEntryDescriptor::Doom
(Assignee)

Comment 6

5 years ago
Created attachment 650927 [details] [diff] [review]
fix

Tryserver looks good https://tbpl.mozilla.org/?tree=Try&rev=a520740b0f4e
Attachment #650927 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

5 years ago
Attachment #650927 - Flags: review?(jduell.mcbugs) → review?(hurley)
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.
Attachment #650927 - Flags: review?(hurley) → review+
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb72643bbae
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.
(Assignee)

Comment 12

5 years ago
Created attachment 658060 [details] [diff] [review]
patch v2

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
Attachment #650927 - Attachment is obsolete: true
Attachment #658060 - Flags: review?(hurley)
Comment on attachment 658060 [details] [diff] [review]
patch v2

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

Looks good to me, assuming try passes.
Attachment #658060 - Flags: review?(hurley) → review+
(Assignee)

Comment 14

5 years ago
Created attachment 661026 [details] [diff] [review]
patch v3 - fixes the leak

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
Attachment #658060 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/af44e1c20352
https://hg.mozilla.org/mozilla-central/rev/af44e1c20352
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.