small cleanups to XPCOM services utilities

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

unspecified
mozilla32
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

No description provided.
On a whim, I instrumented nsGetServiceByContractID to print out contract IDs as
it was called.  I noticed an abnormally high number of observer service
contract IDs, which I found odd, given that there's already a cached getter for
the observer service in mozilla::services.

The reason for this appears to be a lot of things trying to access the observer
service *after* mozilla::services::Shutdown.  Since the cached pointer is null,
we'll try calling into do_GetService and the associated machinery.  Of course,
since XPCOM is shutdown, we'll just return an error.  This patch makes us
return an error a little sooner, which might improve shutdown performance
slightly (though shutdown is probably dominated by I/O anyway...).
Attachment #8414614 - Flags: review?(benjamin)
This is really two things:

- using swap() instead of forget().take(), which IMHO is a little cleaner;
- using nsCOMPtr instead of nsRefPtr, since the TYPE in this instance is an
  interface type, which we're supposed to be using with nsCOMPtr anyway.
Attachment #8414615 - Flags: review?(benjamin)
Attachment #8414614 - Flags: review?(benjamin) → review+
Comment on attachment 8414615 [details] [diff] [review]
part 2 - tidy up refcount management in mozilla::services::Get*

I wonder if we should add a NS_WARNING to the other patch: code really shouldn't be asking for services this late in shutdown anyway. Or maybe spend a few minutes in a debugger seeing if there's some common/stupid pattern to it.
Attachment #8414615 - Flags: review?(benjamin) → review+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> I wonder if we should add a NS_WARNING to the other patch: code really
> shouldn't be asking for services this late in shutdown anyway. Or maybe
> spend a few minutes in a debugger seeing if there's some common/stupid
> pattern to it.

I didn't step through several dozen accesses of the observer service in shutdown, but the major things I noticed were:

- the cycle collector notifies observers when it's doing things;
- pref branches (possibly just the root ones?) remove observers they've added;
- nsExpirationTrackers from nsIDocument observers trying to remove themselves;
- etc.

I think with the current state of things, you'd get a flood of NS_WARNINGs.  Maybe some of the removals should be listening for XPCOM shutdown and auto-remove themselves then.
> the cycle collector notifies observers when it's doing things;
I don't know if it is worth fixing necessarily, but the CC does know when it is in shutdown, so it could just skip observer calls, rather than having them fail.
https://hg.mozilla.org/mozilla-central/rev/1f6e71553333
https://hg.mozilla.org/mozilla-central/rev/060bea047502
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.