835730 - nsBrowserGlue.js does not shut down properly when the profile shuts down

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Gabriele Svelto [:gsvelto]]

nsBrowserGlue.js currently listens to xpcom-shutdown for part of its cleanup, this should be done instead after a profile-before-change event, additionally the places shutdown code does not respond properly to profile-before-change events causing problems in some unit tests (see bug 820438).

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: Properly tear down components when the profile is being shut down

The patch removes the xpcom-shutdown hook and responds to profile-before-change instead and it also resolves the problem with places shutdown seen in bug 820438. This is the try run:

https://tbpl.mozilla.org/?tree=Try&rev=2ac1b575010c

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 707501 [details] [diff] [review]
Properly tear down components when the profile is being shut down

Review of attachment 707501 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserGlue.js
@@ +351,5 @@
>      if (this._isPlacesShutdownObserver)
>        os.removeObserver(this, "places-shutdown");
>      os.removeObserver(this, "defaultURIFixup-using-keyword-pref");
>      os.removeObserver(this, "handle-xul-text-link");
> +    os.removeObserver(this, "profile-before-change");

please file a bug in Firefox/General to use the fact nsBrowserGlue supports weak references, thus we can addObserver(,, true) and never RemoveObserver, that means we can likely kill _dispose() method, moving any remaining stuff to onProfileShutdown or onPlacesShutdown.

@@ +425,1 @@
>      BrowserNewTabPreloader.uninit();

please add a comment at the beginning of _onProfileShutdsown stating that any component depending on Places should be finalized in _onPlacesShutdown.

@@ +1025,3 @@
>    /**
>     * Places shut-down tasks
>     * - back up bookmarks if needed.

please add another bullet point at the beginning stating "- finalize components depending on Places"

@@ +1029,5 @@
>     *
>     * Note: quit-application-granted notification is received twice
>     *       so replace this method with a no-op when first called.
>     */
> +  _onPlacesShutdown: function BG__onPlacesShutdown() {

please while here remove the "Note" from the javadoc, since looks like it comes from an ancient time where we were finalizing at quit-application-granted.

Comment 3

[Gabriele Svelto [:gsvelto]]

Attachment: Revised patch with changes added as per review in comment 2

The changes apply to comments only so I won't re-submit it to try; the previous run was all green except for what looks like an intermittent issue on WinXP.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e881841cd418

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[Philip Chee]

> please file a bug in Firefox/General to use the fact nsBrowserGlue supports weak
> references, thus we can addObserver(,, true) and never RemoveObserver, that means
> we can likely kill _dispose() method, moving any remaining stuff to onProfileShutdown
> or onPlacesShutdown.
This is now Bug 836789