1579943 - ext-theme.js leaks memory because per-window themes are not cleared when a window is closed

Status: RESOLVED FIXED

(WebExtensions :: Themes, defect, P2)

Description

[Rob Wu [:robwu]]

I spotted the following error while running toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors_perwindow.js.
The test creates a new window, applies a theme to it, closes the window and then unloads the extension (relevant source). The error happens because the unload handler iterates over all keys in windowOverrides and then tries to detach the theme from the window. But as the error shows, the window isn't around any more. If you look at ext-theme.js, then it is obvious that the Theme is never cleaned up when a window is removed. This should be fixed.

Console message: [JavaScript Error: "Invalid window ID: 32" {file: "resource://gre/modules/ExtensionUtils.jsm" line: 54}]
ExtensionError@resource://gre/modules/ExtensionUtils.jsm:54:49
getWindow@chrome://extensions/content/parent/ext-tabs-base.js:1535:13
unload@chrome://extensions/content/parent/ext-theme.js:392:23
onShutdown@chrome://extensions/content/parent/ext-theme.js:428:15
ExtensionAPI/<@resource://gre/modules/ExtensionCommon.jsm:356:14
wrapper@resource://gre/modules/ExtensionCommon.jsm:300:14
emit@resource://gre/modules/ExtensionCommon.jsm:327:32
emit@resource://gre/modules/Extension.jsm:1752:25
shutdown@resource://gre/modules/Extension.jsm:2378:10
receiveMessage@resource://specialpowers/SpecialPowersParent.jsm:1048:26

Comment 1

[Rob Wu [:robwu]]

This bug in themes code results in memory leaks. Do you know who can work on this?

Comment 2

[Tim Nguyen :ntim]

I don't actively have time to work on this, sorry.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1579943 - Use a WeakMap of BrowserWindow->Theme to store per-window themes so the entries in the map can get cleaned up when the BrowserWindow is destroyed. r?robwu,ntim

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

needinfo for https://phabricator.services.mozilla.com/D47663#1526885

Comment 5

[Rob Wu [:robwu]]

I'll take over. Thanks for the initial patch.

Comment 6

[Rob Wu [:robwu]]

(not a P1, and I'm not actively working on this because I'm mentoring a couple of bugs related to the theme API, and don't want to stomp on that with this change. Will revisit this bug later)

Comment 7

[Andrea Marchesini [:baku]]

Attachment: Bug 1579943 - remove webext theme memory leak with closing windows, r?rpl

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4de5fe0d2708
remove webext theme memory leak with closing windows, r=robwu

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/4de5fe0d2708