1517787 - Delete `removeObserver()` calls for `xpcom-shutdown` observers from within observe()

Status: NEW

(Core :: XPCOM, enhancement, P3)

Description

[:Gijs (he/him)]

See:

https://searchfox.org/mozilla-central/search?q=removeobserver.*xpcom-shutdown%22&case=false&regexp=true&path=

and

https://searchfox.org/mozilla-central/search?q=removeobserver.*NS_XPCOM_SHUTDOWN_OBSERVER_ID&case=false&regexp=true&path=

These should all be removable. There may be some more ones that we could remove that are trickier to find because they pass the (string) topic some other way.

It may be possible to find such usecases by adding a MOZ_ASSERT or similar to the actual removeobserver implementation ( https://searchfox.org/mozilla-central/source/xpcom/ds/nsObserverService.cpp#229 ) if the topic passed is indeed `xpcom-shutdown`.

Of course, the *theory* is these should be removable (and not removing them shouldn't, say, cause crashes or start causing memory leaks, or have observers be notified twice if we do in-app app restarts). :emk agreed in bug 1348886 comment 8 . :froydnj, can you (or someone you forward this ni to) confirm if it is safe enough that we could just make this a mentored bug? :-)

Comment 1

[:Gijs (he/him)]

+ni for comment 0, sigh.

Comment 2

[:Gijs (he/him)]

Err, with my brain switched back on, of course it's possible that some component gets destroyed before shutdown and then wants to remove the observer, and that's fine. The issue is with removing the observer when notified for xpcom-shutdown... So we'll actually need to audit the two lists in comment #0 (some 30-odd sites), which shouldn't be too bad, but probably isn't mentored bug fodder.

Comment 3

[Nathan Froyd [:froydnj]]

I think what you describe in comment 2 should be safe enough...but what's the driving rationale here?  This bug blocks cutting down the number of xpcom-shutdown observers, but that relationship doesn't make any sense to me.  I agree that it'd save cycles, but I'm unclear on whether that'd be a good use of developer time.

Comment 4

[:Gijs (he/him)]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> I think what you describe in comment 2 should be safe enough...but what's
> the driving rationale here?  This bug blocks cutting down the number of
> xpcom-shutdown observers, but that relationship doesn't make any sense to
> me.

A significant number of the xpcom-shutdown observers seems to be per-tab/per-document. Because of how they work, I'm not convinced that there's an easy way to avoid this, short of having a per-process global collection of the relevant objects that share a single observer for cleanup tasks that then iterates, or something (which is generally what we seem to be using xpcom-shutdown for).

There being a 1-1 relationship with docs/tabs means that in large sessions, we can be running 1000 or more of those observers. At that point, omitting the spurious removeobserver calls seems like it'd make a dent (also because the observer service, last I checked, will just iterate over every observer (O(n)) to find the one you want to remove).

I'm also happy to be able to get rid of the observers completely, I just suspect that that's less likely to be feasible in the near-ish term.

>  I agree that it'd save cycles, but I'm unclear on whether that'd be a
> good use of developer time.

Fair. I'm hoping that we can get rid of the extant ones and then perhaps MOZ_ASSERT() about this case to avoid reoccurrances? I'll see if I can get some perf numbers for shutdown later this week.

Comment 5

[Andrew Overholt [:overholt]]

Let's park this in P3 until Gijs gets a chance to come back with perf numbers.

Comment 6

[:Gijs (he/him)]

This is a bug in consumers, not xpcom itself.

Comment 7

[:Gijs (he/him)]

So I finally picked this back up. I fiddled around with clang this morning and came up with a rudimentary check that logs warnings when calling RemoveObserver(..., "xpcom-shutdown") directly from within Observe(), and pushed to try. clang is somehow upset on all the debug builds because of some assertion, but the opt builds completed successfully and logged 20 warnings. As far as I can tell from a very quick look, they are all correct (ie no false positives), despite the checker not being failsafe (e.g. if you had registered for multiple observer notificiations, you could conceivably remove the shutdown observer when you get one of the other notifications). Full list is at the bottom of this comment.

Now, the trivial thing to do is to just remove all the spurious calls here. However, having looked at all of these, I've spotted a few patterns where maybe we can avoid having things observe shutdown at all.

• removing all the other observers
• updating a class member mIsShutdown bool
• cancelling timers
• joining threads / shutting down subprocesses

A bunch of these feel like they could be handled more gracefully and/or might also be unnecessary. Doug, in your investigations so far, do you have a rough idea how much time we spend handling xpcom-shutdown as a proportion of shutdown, and if it's worth coming up with better abstractions for some of this?

List:

https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/xpcom/threads/nsProcessCommon.cpp#530
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/netwerk/cookie/CookieServiceChild.cpp#591
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/netwerk/base/NetworkConnectivityService.cpp#167
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/netwerk/base/nsProtocolProxyService.cpp#917
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/netwerk/protocol/http/DelayHttpChannelQueue.cpp#90
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp#76
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/base/nsCCUncollectableMarker.cpp#299
https://firefoxci.taskcluster-artifacts.net/bbXA4iW3QduxZeIqvL1ywg/0/public/logs/live_backing.log
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/file/ipc/IPCBlobInputStreamStorage.cpp#60
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/geolocation/Geolocation.cpp#578
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/media/mediacontrol/MediaControlService.cpp#85
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/storage/StorageActivityService.cpp#181
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/storage/StorageIPC.cpp#401
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/ipc/ProcessHangMonitor.cpp#1252
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/workers/remoteworkers/RemoteWorkerService.cpp#162
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/dom/reporting/ReportDeliver.cpp#381
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/layout/xul/nsXULPopupManager.cpp#173
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/security/manager/ssl/nsNSSComponent.cpp#1308
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/toolkit/components/finalizationwitness/FinalizationWitnessService.cpp#216
https://searchfox.org/mozilla-central/rev/0439db3a39faddb90197a87cc184c57dcbf0a770/toolkit/components/antitracking/AntiTrackingCommon.cpp#710,747

Comment 8

[:Gijs (he/him)]

Attachment: Bug 1517787 - warn when removing shutdown observers from Observe()

Comment 9

[Alex Thayer [:alexical] (she/her)]

(In reply to :Gijs (he/him) from comment #7)

A bunch of these feel like they could be handled more gracefully and/or might also be unnecessary. Doug, in your investigations so far, do you have a rough idea how much time we spend handling xpcom-shutdown as a proportion of shutdown, and if it's worth coming up with better abstractions for some of this?

I'm not the most confident about my intuitions here, unfortunately. Bug 1613733 yielded substantial wins at both the 5th and 95th percentiles, and that does not match numbers which you would expect from shutdown profiling on reference hardware. In any case bug 1613733 saw these wins without removing any shutdown IO, so there's a solid case to be made for simply avoiding unnecessary freeing and cleanup on the CPU. xpcom-shutdown does certainly show up in profiles, so work to improve it will not be entirely wasted, but it's hard to say precisely how much we might expect from it.

Comment 10

[Jens Stutte [:jstutte]]

Moving this to XPCOM for further discussion. I am not sure I understand the intention, it seems we want to just avoid the removal of the observers during shutdown? That seems to not be the most impactful thing to me. Do we still think this is gaining us something?

Comment 11

[:Gijs (he/him)]

(In reply to Jens Stutte [:jstutte] from comment #10)

Moving this to XPCOM for further discussion. I am not sure I understand the intention, it seems we want to just avoid the removal of the observers during shutdown? That seems to not be the most impactful thing to me. Do we still think this is gaining us something?

Well, we used to have a substantial number of shutdown crashes because shutdown was too slow. And as I noted in the blocking bug 1348886, we still have way too many xpcom-shutdown observers.

When I filed this I was hoping that if we could automatically remove calls to RemoveObserver for xpcom-shutdown being called inside Observe, and then remove any cases where the relevant block was empty, that would help cut down on the number of xpcom-shutdown observers (ie maybe this could be the "easy" first bit of trying to get that number under control). I don't know if that's still the case.