Open Bug 1517787 Opened 1 year ago Updated 1 month ago
Observer()` calls for `xpcom-shutdown` observers from within observe()
47 bytes, text/x-phabricator-request
|Details | Review|
See: https://searchfox.org/mozilla-central/search?q=removeobserver.*xpcom-shutdown%22&case=false®exp=true&path= and https://searchfox.org/mozilla-central/search?q=removeobserver.*NS_XPCOM_SHUTDOWN_OBSERVER_ID&case=false®exp=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? :-)
+ni for comment 0, sigh.
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.
Summary: Delete all `removeObserver()` calls for `xpcom-shutdown` observers as they are unnecessary → Delete `removeObserver()` calls for `xpcom-shutdown` observers from within observe()
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.
(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.
Priority: -- → P3
Component: General → XPCOM
Component: XPCOM → General
You need to log in before you can comment on or make changes to this bug.