398270 - [mlk] Loading a feed leaks

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect, P1)

Description

[pd]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100205 Minefield/3.0a9pre

Memory leak according to leak-gauge.pl after clicking feed discovery icon then closing minefield.

SET MOZ_CRASHREPORTER_DISABLE
SET NSPR_LOG_MODULES=DOMLeak:5,DocumentLeak:5,nsDocShellLeak:5
SET NSPR_LOG_FILE=nspr_leaks.log
"C:\Apps\MInefield\firefox.exe"

Click location bar drop down remembered locations menu, left click first remembered URL, which happens to be:

http://blackwhitestripes.wordpress.com/

Wait for site to fully load.

Left mouse click on location bar feed discovery icon.

"Live Bookmark" Feed preview page/UI loads as expected.

Left click the red X window close 'button'.

Run leak-guage.pl

Reproducible: Always

Steps to Reproduce:
1. 

Run batch file containing the folllowing lines:

SET MOZ_CRASHREPORTER_DISABLE
SET NSPR_LOG_MODULES=DOMLeak:5,DocumentLeak:5,nsDocShellLeak:5
SET NSPR_LOG_FILE=nspr_leaks.log
"C:\Apps\MInefield\firefox.exe"

2. 

Click location bar drop down menu button

3.

Left click first remembered URL, which happens to be:

  http://blackwhitestripes.wordpress.com/

Wait for site to fully load.

4.

Left mouse click on location bar feed discovery icon.

"Live Bookmark" Feed preview page/UI loads as expected.

5.

Left click the windows red X close 'button' to close the browser.

6.

Run "leak-gauge.pl nspr_leaks.log"
Actual Results:  
C:\leak-gauge.pl nspr_leaks.log
Leaked document at address 229f5e0.
 ... with URI "jar:file:///C:/Apps/MInefield/chrome/toolkit.jar!/content/global/
bindings/menulist.xml".
 ... with URI "chrome://global/content/bindings/menulist.xml".
Leaked document at address 1791708.
 ... with URI "jar:file:///C:/Apps/MInefield/chrome/toolkit.jar!/content/global/
bindings/menu.xml".
 ... with URI "chrome://global/content/bindings/menu.xml".
Leaked document at address 17821c0.
 ... with URI "jar:file:///C:/Apps/MInefield/chrome/toolkit.jar!/content/global/
bindings/general.xml".
 ... with URI "chrome://global/content/bindings/general.xml".
Leaked document at address 227ea50.
 ... with URI "chrome://global/content/bindings/checkbox.xml".
 ... with URI "jar:file:///C:/Apps/MInefield/chrome/toolkit.jar!/content/global/
bindings/checkbox.xml".
Summary:
Leaked 0 out of 8 DOM Windows
Leaked 4 out of 42 documents
Leaked 0 out of 3 docshells

Expected Results:  
Memory allocated as required and regained when no longer needed. i.e. no leaking.

Comment 1

[pd]

Attachment: NSPR leak log

Attached is the log file I processed with leak-gauge.pl to discover the memory leak.

Comment 2

[pd]

Apologies for the messy duplicated content in this bug report. The "steps to reproduce" are also included in the description.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Mossop, did you mean to confirm and nominate for blocking?

Comment 4

[Peter Van der Beken [:peterv]]

I'm seeing this by just starting up the browser, loading a feed and shutting down. we leak about 300K on OS X, including 4 documents.
Some tracing led me to this:

0x183b21c0 [JS Object (Function - FeedWriter)] -> 0x183b4be0 [JS Object (BackstagePass)] -> 0x183b6ce0 [XPCNativeWrapper (XULElement)] -> 0x183b6cc0 [JS Object (XULElement)] -> 0x183a5000 [JS Object (Window)] -> 0x186d704c [nsGlobalWindow]

which shows that the FeedWriter JS component sets a XULElement as a property on its global object. The nsGlobalWindow that this holds is released properly and doesn't leak when the JS component is shut down, but because the last cycle collection happens before shutting down JS components we don't cycle collect a bunch of other stuff that the XUL element ends up holding.

The problem is a missing var in the FeedWriter code, patch coming up.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 6

[Brendan Eich [:brendan]]

This assignment to an undeclared global variable should result in a strict warning at runtime. Does it?

/be

Comment 7

[Peter Van der Beken [:peterv]]

(In reply to comment #6)
> This assignment to an undeclared global variable should result in a strict
> warning at runtime. Does it?

The JS component loader doesn't follow the javascript.options.strict pref (filed bug 398836), but even after patching it to always set JSOPTION_STRICT I didn't see a warning.

Comment 8

[Jeff Walden [:Waldo]]

This showed up in Mochitest runs, so it's just waiting on bug 393993 to be fixed to be in-testsuite+.