1277405 - Places observers cause a shutdown leak when there are reference cycles

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Kris Maglione [:kmag]]

nsNavHistory and nsNavBookmarks don't implement cycle collection, so if there are any reference cycles between observers and the services themselves, we get a shutdown leak and crash.

This currently happens if any global which references the download service also has a live downloads observer, since the download service registers a history observer for the lifetime of the process.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1277405: Clear Places observers at XPCOM shutdown.

Review commit: https://reviewboard.mozilla.org/r/57020/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57020/

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Please just do this where we set mCanNotify to false (we wouldn't notify from that point on...)
Also please make addObserver throw if mCanNotify is false...

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8758938 [details]
Bug 1277405: Clear Places observers at XPCOM shutdown.

https://reviewboard.mozilla.org/r/57020/#review53912

Comment 4

[Kris Maglione [:kmag]]

(In reply to Marco Bonardo [::mak] from comment #2)
> Please just do this where we set mCanNotify to false (we wouldn't notify
> from that point on...)
> Also please make addObserver throw if mCanNotify is false...

I was assuming there was a reason that they weren't cleared at that point (namely preventing removeObserver from throwing if observers were cleared to early), but I'll do a full try run and see if it causes any problems.

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8758938 [details]
Bug 1277405: Clear Places observers at XPCOM shutdown.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57020/diff/1-2/

Comment 6

[Kris Maglione [:kmag]]

Well, it didn't seem to cause any problems on try

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8758938 [details]
Bug 1277405: Clear Places observers at XPCOM shutdown.

https://reviewboard.mozilla.org/r/57020/#review54320

Thanks, it looks good!

Comment 8

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/0e6dfa8fc2f4d513dba9f434fc3a92c32e6b9ad6
Bug 1277405: Clear Places observers at XPCOM shutdown. r=mak

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e6dfa8fc2f4