1464781 - Make the observer service us nsTObserverArray for storage

Status: NEW

(Core :: XPCOM, enhancement)

Description

[Gabriele Svelto [:gsvelto]]

+++ This bug was initially created as a clone of Bug #1435683 +++

The observer service needs to enumerate the lists of observers it holds but while it doesn't allow for adding or removing observers from within a a call to notify() [1] it does make an explicit copy of the observers array before iterating over it [2]. It does so because it needs to remove weakly held observers [3] and explicitly iterates backwards to do so reliably [4]. Additionally, since it's iterating over a copy, adding or removing observers within an observe() implementation wouldn't affect the notify() call anyway.

Since nsTObserverArray was designed to deal with this kind of scenario (reliable and predictable iteration over a potentially changing array) we might just as well use it instead of this manual implementation and drop the constraints in observe().

[1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/xpcom/ds/nsIObserver.idl#25
[2] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/xpcom/ds/nsObserverList.cpp#109
[3] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/xpcom/ds/nsObserverList.cpp#83
[4] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/xpcom/ds/nsObserverList.cpp#76

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Taking a copy is also about keeping the observer object alive while calling Observe().

Comment 2

[Gabriele Svelto [:gsvelto]]

(In reply to Olli Pettay [:smaug] from comment #1)
> Taking a copy is also about keeping the observer object alive while calling
> Observe().

Thanks for pointing that out. So if we use nsTObserverArray::ForwardIterator it would change the semantics in two ways: it would skip elements that have been removed during iteration and it would allow a listener to destroy itself before observe() finishes execution (if it removes itself from the service and there's no other reference to it).

This made me realize that while the documentation in nsIObserve states that you should not remove a listener within observe() we do it all over the place. Typically within "xpcom-shutdown" / "xpcom-will-shutdown" observers which remove themselves within the observe() implementation. So at the very least we should update the documentation because it doesn't reflect reality.

enumerateObservers() would also be affected since it makes a copy of the current listeners and it doesn't give any restriction to what the client can do with the service while the enumerator is alive.

Comment 3

[Gabriele Svelto [:gsvelto]]

This also made me wonder if we have places where we call notify() recursively. My gut feeling is that we might have observe() calls for a topic notify()ing another topic.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Keeping alive while calling Observe() is about preventing UAF issues (by following COM rules that caller should keep the callee alive).

Comment 5

[Gabriele Svelto [:gsvelto]]

I've got a WIP patch for this but I bumped my head against an issue I hadn't thought about: the code is not copying the array when calling notify() only to keep the objects alive but also because otherwise the array could be clobbered if one of the observe() implementations calls addObserver()/removeObserver(). The issue stems from the fact that PLDHashTable (which nsTHashTable builds upon) uses open addressing with inline key storage. So, if an observe() call invokes addObserver() it can cause the hash-table to be resized which in turn would clobber the observer list the notify() method was already iterating upon.

Armed with this knowledge I should be able to clean up this code quite a bit, I just feel silly because it took me far longer than I had anticipated to realize why removing the array copy in notify() wasn't working properly.

Comment 6

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] ObserverList conversion to nsTObserverArray WIP

This is a quick patch I've cobbled together to use nsTObserverArrays in the ObserverList implementation. There's still a couple of things I'm not happy with and it can probably be made shorter by using lambda expressions when walking the arrays to cut on some code duplication. Besides there's still a couple of issues to iron out:

- Firefox starts correctly but crashes in some unrelated code after a few seconds. I'm unsure what the problem is, the only thing that comes to mind is that it happens some time after UnmarkGrayStrongObservers() is called (and this function is called within one Notify() call so I'm wondering if I did something wrong there).

- The iterator used to implement EnumerateObservers() is still making a copy of the listener arrays, I'd like to get rid of that too.