1818099 - FeatureCallout in chrome doesn't fully clean up after itself

Status: RESOLVED FIXED

(Firefox :: Messaging System, defect, P1)

Description

[Shane Hughes [:aminomancer]]

In tabbrowser.js, under some circumstances we end the tour and clear the reference to the FeatureCallout instance, so it can be garbage collected. But the event listeners it created are not removed, as endTour doesn't call _removePositionListeners. That means they can pile up. I haven't done a complete assessment so there might be other things that need to be cleaned up too.

Also, the pageEventManager we instantiate on the window isn't cleaned up, but it isn't reused by the next FeatureCallout instance, either. When the tour ends, its listeners are all cleared, but it hangs around. And when the next FeatureCallout is instantiated, a new lazy getter is created and the pageEventManager property on the window is redefined. So, the previous one should be garbage collected, but there's really no need to recreate it. We could just reuse the previous one, if it exists.

I don't really like the system that's in place for dynamically lazy-loading PageEventManager, though. It basically does this thing where it defines a lazy getter on the FeatureCallout which loads PageEventManager, and that lazy getter also adds a reference to the PageEventManager on the window. The reason is that it allows the page listeners to reference this.pageEventManager (which will load PageEventManager and instantiate one), while all the generic FeatureCallout code can reference this.win.pageEventManager instead (which will not load anything). That way, if none of the messages have page event listeners, we don't need to load the manager. It only gets loaded when a message with a page event listener is loaded.

But when I wrote that, it was still just a page script with a bunch of separate functions. Now it's a class in a system module. It would be better to replace this.pageEventManager with lazy.pageEventManager, and replace this.win.pageEventManager with this.pageEventManager. That way we don't need to leak anything to the window.

And while we're adding a lazy object, it would be wise to move all the other lazy getters outside of the constructor and into the top level of the module. That is more consistent with the rest of the codebase and it also avoids redundantly defining module getters. The system module is singleton and persistent so we don't need to worry about the references working. I think the only reason we put the getters inside the ctor is because of the PageEventManager lazy-loading system I described before.

Comment 1

[Shane Hughes [:aminomancer]]

note - bug 1809441 adds a new lazy import, so don't land this until rebasing off Jason's patch

Comment 2

[Shane Hughes [:aminomancer]]

Also, a related issue - aboutwelcome.bundle.js is added every time a callout screen is shown, and is not removed for teardown, so it also piles up

Comment 3

[Shane Hughes [:aminomancer]]

Attachment: Bug 1818099 - Feature Callout - more thorough cleanup. r=jprickett

Tear down all listeners when ending a Feature Tour. Also move some of
the references and module getters around to eliminate redundant imports
and define fewer properties on the window, to reduce our footprint on
the chrome window. Finally, make a few nitpick changes and improve the
documentation a bit.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:tspurway, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43460fb53708
Feature Callout - more thorough cleanup. r=jprickett

Comment 6

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/43460fb53708