901753 - Switch nsObserverList to use mozilla::MaybeWeakMultiset

Status: NEW

(Core :: XPCOM, defect)

Description

[Justin Lebar (not reading bugmail)]

Right now nsObserverList implements weak observers by adding nsIWeakReference objects to a list and, when the observer fires, removing those weakrefs from the list.

This is not optimal when the observer in question fires infrequently.  For example, if you register a weak xpcom-shutdown observer, that nsIWeakReference object is leaked essentially forever.

mozilla::WeakSet solves this problem, and as proof-of-concept, I've converted nsObserverList to use it.

Comment 1

[Justin Lebar (not reading bugmail)]

I don't expect this will save a ton of memory, but it might be more relevant change elsewhere.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Comment 3

[Benjamin Smedberg]

Comment on attachment 786022 [details] [diff] [review]
Patch, v1

Observers must fire in FILO order for compatibility. We can't use a hashset for them.

Comment 4

[Justin Lebar (not reading bugmail)]

That's a bummer.

I'll have to implement a linked hashset then, because I'm /sick and tired/ of us using O(n) algorithms where O(1) is easy.

http://docs.oracle.com/javase/6/docs/api/java/util/LinkedHashMap.html

Comment 5

[Justin Lebar (not reading bugmail)]

Sounds like I should just write a class which lets you add strong or weak refs and iterate over them in order.  We could use this in the observer service and in the message manager.

Comment 6

[Justin Lebar (not reading bugmail)]

(In reply to Benjamin Smedberg)
> Observers must fire in FILO order for compatibility. We can't use a hashset
> for them.

Right now the observer list allows duplicate observers.

Do we have to keep that behavior as well?  Doing so essentially forces us to use a list, which would be unfortunate from an asymptotic PoV.  (With a list, a weakref being destroyed could cause an O(n) operation in the observer service.  This seems like a pretty bad footgun.)

Comment 7

[Justin Lebar (not reading bugmail)]

dbaron, do you know if we need to preserve the current behavior of the observer service, where it allows you to register the same observer multiple times?

This is doable, but I don't want to write the data structure if we don't need it.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I don't know.

I guess if you wanted to try not preserving it, you could at least add assertions to catch cases where we were depending on it.

Comment 9

[Justin Lebar (not reading bugmail)]

I guess I could add assertions to catch when we add a duplicate observer, but of course that assertion may trip for places where we don't depend on getting duplicate notifications.

Comment 10

[Justin Lebar (not reading bugmail)]

I guess the better question is, do we feel like this behavior is something we need to preserve for add-ons?  We can fix instances in the tree...

Given that add-ons have committed every bug conceivable, I suppose we probably do want to preserve the current behavior for add-ons.  Duplicate notifications might not be as big an issue as the fact that, if we only store one ref to a given observer, then the first remove() call will remove the observer, while before you'd have to remove() an observer once for every time you added it.

Comment 11

[Benjamin Smedberg]

I don't have a good answer about how likely it is that addons register the same observer multiple times. Options are:

A. add telemetry for multiply-registered observers and see what it says (low risk, high lead time)
B. assert/fail if an observer is registered multiple times (medium risk)
C. register with a counter so that multiple registrations continue to work (with slightly different ordering, but I highly doubt that matters) (low risk, low lead time, slightly more complex)

Given lack of data, I'm more comfortable with A or C (both would be ok also).

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v2

This is ready for review, but I want to get at least preliminary f+ on the patches this depends on before asking someone to look at it.

This patch should not affect the externally-observable behavior of the observer service in any way.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

khuey, you want this one too?

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

While this would be nice, I don't think I'm going to have time to work on it, seeing as I haven't in the last year :/

Comment 15

[Andrew McCreight [:mccr8]]

I'm going to assume Justin is not actively working on this either...