1502284 - Cleanup of the observer service code

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Gabriele Svelto [:gsvelto]]

The observer service code could use some cleanups before I proceed to introduce the new interface in bug 1435683. Some of its code can be factored to make it less verbose and there's some odd stuff that might be removed if it's not needed anymore.

Comment 1

[Gabriele Svelto [:gsvelto]]

I've stumbled upon this code that "filters" certain topics related to Necko within the observer service implementation:

https://searchfox.org/mozilla-central/rev/e0c879c86b95bdc752b1dbff6088169735674e4a/xpcom/ds/nsObserverService.cpp#213

Dragana is this something that's still needed? If it is then I'd like to take it out of the base observer service and move it to the Necko code by wrapping the observer service in a Necko-specific object that would do this filtering.

Comment 2

[Dragana Damjanovic [:dragana]]

This is introduce for dev tools. Asking Alexander who made bug 1269765.

Comment 3

[Alexandre Poirot [:ochameau]]

The original filtering isn't related to bug 1269765, but to bug 806753.
And I think it was to avoid confusion with addons trying to listen for these necko events from the child process.
I imagine you could do what you said and move this to necko.
You may also avoid emitting the events if that's any easier. I'm not sure it is critical anymore to have an exeption on listening.
You might as well be able to just drop this safety code if that was here only to protect against legacy addons now that they are no more!

Comment 4

[Gabriele Svelto [:gsvelto]]

If that's the case I'll gladly drop it entirely, thanks!

Comment 5

[Gabriele Svelto [:gsvelto]]

Blocking bug 1436250 since modifying the way observer lists store listeners can save some memory. In my tests this is up to ~16KiB in the main process and ~4KiB in content processes.

Comment 6

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1502284 - Extend nsMaybeWeakPtr and make expose it to the rest of the code

This adds a way to detect if an instance is holding a weak reference or a
strong one, makes non-critical failures less chatty and adds separate methods
for adding unique and non-unique instances to an array.

Comment 7

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1502284 - Cleanup of the observer service code

This refactors the observer service code to improve readability, uses MOZ_TRY
and other checking macros wherever possible to simplify error handling and
replaces the ObserverRef class with the more generic nsMaybeWeakPtr class.
This cuts away some code and halves the amount of memory needed to store an
event listener. The external behavior is almost unchanged save for some error
codes which are now more specific.

Depends on D11645

Comment 8

[Gabriele Svelto [:gsvelto]]

I've updated the patches with the review comments addressed. For now I've settled with a bool within the nsMaybeWeakPtr structure to reduce the number of calls to do_QueryInterface() (especially during GC/CC; I hadn't realized it but it was quite noticeable over a large number of listeners). I'll split the memory consumption improvements into a separate bug so that we can land this.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/717c99f90176
Extend nsMaybeWeakPtr and make expose it to the rest of the code r=erahm

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/717c99f90176

Comment 11

[Gabriele Svelto [:gsvelto]]

I just realized that Lando only landed the patch related to nsMaybeWeakPtr, not the cleanup itself. I'll land it manually.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c77a41fad0e6
Cleanup of the observer service code r=erahm

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c77a41fad0e6