Closed
Bug 1291709
Opened 8 years ago
Closed 8 years ago
PdfjsChromeUtils.jsm leaks browser.xul windows
Categories
(Firefox :: PDF Viewer, defect)
Firefox
PDF Viewer
Tracking
()
RESOLVED
FIXED
Firefox 51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: bkelly, Assigned: bkelly)
References
Details
(Whiteboard: [MemShrink])
Attachments
(1 file)
(deleted),
patch
|
bdahl
:
review+
|
Details | Diff | Splinter Review |
Today in 50.0a1 (2016-07-31) I discovered I had a couple leaked browser.xul windows. Both were being retained via this path:
00000238DA4EBCE0 [BackstagePass 238e000a680]
--[PdfjsChromeUtils]--> 00000238DDD04EC0 [Object <no private>]
--[_browsers]--> 00000238DFC7F580 [Set 238ddb9b400]
--[key]--> 00000238EBA433E0 [Proxy <no private>]
--[private]--> 00000238EC67E190 [XULElement <no private>]
--[shape]--> 00000238ECFCCAC0 [shape]
--[base]--> 00000238ECFD4A60 [base_shape]
--[global]--> 00000238D9A9E060 [Window <no private>]
Seems like this is a pretty easy case where we could use a HashSet() instead of a Set().
Brendan, what do you think? Is this maintained upstream?
Flags: needinfo?(bdahl)
Assignee | ||
Comment 1•8 years ago
|
||
I meant, we could use a WeakSet(). :-)
Comment 2•8 years ago
|
||
It's maintained upstream, but we also will pull down changes from mc. Looks like bug 1072350 added this.
Flags: needinfo?(bdahl)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•8 years ago
|
||
Thanks, that helped me find steps to reproduce:
1) Launch fresh browser
2) open about:memory in a tab
3) Open a new window
4) Navigate to a pdf in the new window (I used an IRS form)
5) Use ctrl-f to find a word in the pdf
6) Close the new window
7) Minimize and measure memory in the about:memory tab
Assignee | ||
Comment 4•8 years ago
|
||
This fixes the problem for me locally.
Attachment #8777486 -
Flags: review?(bdahl)
Comment 5•8 years ago
|
||
I was hoping to look at this a bit more today, but didn't get a chance. FYI, we've been having some discussion upstream in https://github.com/mozilla/pdf.js/pull/7521 on why pdf.js's current code doesn't work.
Updated•8 years ago
|
Attachment #8777486 -
Flags: review?(bdahl) → review+
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeb1e15857b3
Make PdfjsChromeUtils.jsm use a WeakSet for tracking browsers. r=bdahl
Comment 7•8 years ago
|
||
Please push changes to browser/ to fx-team in the future to avoid merge conflicts.
Comment 8•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in
before you can comment on or make changes to this bug.
Description
•