Closed Bug 1339782 Opened 7 years ago Closed 7 years ago

FinderHighlighter.jsm leaks chrome documents causing findbar.xml to leak browser windows

Categories

(Toolkit :: Find Toolbar, defect)

51 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: bugzilla-mozilla, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, Whiteboard: [MemShrink:P1])

Attachments

(1 file, 1 obsolete file)

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0 Build ID 20170125094131
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0 Build ID 20170214030231

 0. Clean profile
 1. Ctrl+T		(open about:newtab in a new Tab)
 2. Ctrl+F		(open Find Toolbar)
 3. F5			(refresh)
 4. a			(search anything)
 5. Ctrl+W		(close the tab)
 6. Ctrl+N		(open a new window)
 7. Ctrl+T		(open about:newtab in a new Tab)
 8. Ctrl+F		(open Find Toolbar)
 9. a			(search anything)
10. Ctrl+Shift+W	(close the window)
11. Enter		(confirm dialog)

about:memory
│    ├──3,310,208 B (02.63%) -- top(none)/detached
│    │  ├──2,854,000 B (02.26%) ++ window(chrome://browser/content/browser.xul)
│    │  ├────454,160 B (00.36%) ++ window(about:newtab)
│    │  └──────2,048 B (00.00%) ── window([system])/dom/other [2]

instead of about:newtab any listed in about:about should work
Repeat Step 6-11 for more browser.xul leaks
Step 7 not needed for single process ( e10s disabled )
Step 3 and 4 not needed for Firefox < 54 ( bug 1335730 )

Browser Console (needs to be open to catch)
TypeError: window is null[Learn More]  FinderHighlighter.jsm:1159:5
	_removeHighlightAllMask resource://gre/modules/FinderHighlighter.jsm:1159:5
	hide resource://gre/modules/FinderHighlighter.jsm:382:5
	clear resource://gre/modules/FinderHighlighter.jsm:460:9
	destroy resource://gre/modules/Finder.jsm:65:7
	destroy chrome://global/content/bindings/findbar.xml:424:13
	findbar_XBL_Destructor chrome://global/content/bindings/findbar.xml:411:9
Flags: needinfo?(mdeboer)
Should also fix bug 1287237.

Finder.prototype.onLocationChange seems to be called twice now for all location changes, maybe only the unload listener would suffice?
Attachment #8837590 - Flags: review?(jaws)
(In reply to Dorando from comment #1)
> Finder.prototype.onLocationChange seems to be called twice now for all
> location changes, maybe only the unload listener would suffice?

No, browsing to another page, onLocationChange is called once. When the tab closes, it may be called twice.

I'm quite happy you filed this. would you mind if I steal this and submit my own patch?
Flags: needinfo?(mdeboer)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bugzilla-mozilla)
Whiteboard: [MemShrink]
Comment on attachment 8837590 [details] [diff] [review]
Clear unloaded window, add safeguards

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

This doesn't seem to fix the memory leak, just the JS error. We'll need to do a bit more here.
Attachment #8837590 - Flags: review?(jaws) → review-
(In reply to Mike de Boer [:mikedeboer] from comment #2)
> I'm quite happy you filed this. would you mind if I steal this and submit my
> own patch?
Sure, steal it.

> No, browsing to another page, onLocationChange is called once. When the tab
> closes, it may be called twice.
I meant with the patch, otherwise it only get called twice the first time after opening the findbar (the Finder constructor binds unload to onLocationChange for the initial content window).

(In reply to Mike de Boer [:mikedeboer] from comment #3)
> This doesn't seem to fix the memory leak, just the JS error.
Strange, I am not able to reproduce the leaks with those changes (applied directly to the files in omni.ja + -purgecaches + clearing omni.ja/jsloader/resource/gre/modules/ ). At least the browser.xul leak should be gone without the error, as the findbar.xml destructor should now be able to always call removeObserver.
Has STR: --- → yes
Flags: needinfo?(bugzilla-mozilla)
Version: unspecified → 51 Branch
Whiteboard: [MemShrink] → [MemShrink:P1]
Would be great if we could get an upliftable fix this week still. The Fx52 RC is being built on Monday.
Flags: needinfo?(mdeboer)
Keywords: mlk
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
Comment on attachment 8840398 [details]
Bug 1339782 - use a WeakMap to keep track of windows with active findbar highlighters and make sure no JS error occurs when a window is closed.

https://reviewboard.mozilla.org/r/114920/#review116802

::: toolkit/modules/FinderHighlighter.jsm:482
(Diff revision 1)
> +      return;
>      this.hide(window);
>      this.clear(window);
>      this._removeRangeOutline(window);
>  
>      gWindows.delete(window.top);

At the beginning of clear(), you check if window is truthy and then if window.top is truthy? Can window.top ever be not truthy? Should you have a similar check here to make sure that it's not null before calling gWindows.delete?
Attachment #8840398 - Flags: review?(jaws) → review+
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/54778f0ce12e
use a WeakMap to keep track of windows with active findbar highlighters and make sure no JS error occurs when a window is closed. r=jaws
https://hg.mozilla.org/mozilla-central/rev/54778f0ce12e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Probably too late for Fx52 at this point since we're already in the release candidate part of the cycle, but we should definitely get this uplifted to Aurora as soon as we're comfortable doing so. Also getting this on the radar for possible ESR52 uplift down the line.
Flags: needinfo?(mdeboer)
Well, since the modal highlighting mode is disabled by default on anything _but_ Nightly, I think you'll be hard-pressed to reproduce this.
I think the need to uplift this code is minimal.
Flags: needinfo?(mdeboer)
Firefox 51+ with findbar.modalHighlight;false is affected too.

TypeError: window is null[Learn More]  FinderHighlighter.jsm:1067:5
	_removeHighlightAllMask resource://gre/modules/FinderHighlighter.jsm:1067:5
	hide resource://gre/modules/FinderHighlighter.jsm:368:5
	clear resource://gre/modules/FinderHighlighter.jsm:467:9
	Finder.prototype.destroy resource://gre/modules/Finder.jsm:65:7
	destroy chrome://global/content/bindings/findbar.xml:427:13
	findbar_XBL_Destructor chrome://global/content/bindings/findbar.xml:414:9

│   ├───3,281,512 B (03.03%) -- top(none)/detached
│   │   ├──2,657,144 B (02.46%) ++ window(chrome://browser/content/browser.xul)
│   │   ├────621,296 B (00.57%) ++ window(about:newtab)
│   │   └──────3,072 B (00.00%) ── window([system])/dom/other [3]
Flags: needinfo?(ryanvm)
Flags: needinfo?(mdeboer)
Flags: needinfo?(ryanvm)
Okay, so that means this is also happening without modal highlighting and _with_ highlight all turned on. Requesting uplift.
Flags: needinfo?(mdeboer)
Attachment #8837590 - Attachment is obsolete: true
Comment on attachment 8840398 [details]
Bug 1339782 - use a WeakMap to keep track of windows with active findbar highlighters and make sure no JS error occurs when a window is closed.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Takes care of leaking a window object in certain cases when highlight all feature is used of the findbar
Fix Landed on Version: 54
Risk to taking this patch (and alternatives if risky): minor
String or UUID changes made by this patch: n/a

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: recent findbar highlight all improvements
[User impact if declined]: Leaking window object in relatively rare cases
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not verified by QA, no.
[Needs manual test from QE? If yes, steps to reproduce]:  If possible, yes. The STR is in comment 0.
[List of other uplifts needed for the feature/fix]: this one patch only.
[Is the change risky?]: no.
[Why is the change risky/not risky?]: because it changes the code to hold weak references instead, a JS only change.
[String changes made/needed]: n/a.
Attachment #8840398 - Flags: approval-mozilla-esr52?
Attachment #8840398 - Flags: approval-mozilla-beta?
Attachment #8840398 - Flags: approval-mozilla-aurora?
Comment on attachment 8840398 [details]
Bug 1339782 - use a WeakMap to keep track of windows with active findbar highlighters and make sure no JS error occurs when a window is closed.

Let's fix this in aurora 53 now. It's too late for 52/esr52.0, but we can keep it on the radar for 52.1.0 or in case of a dot release.
Attachment #8840398 - Flags: approval-mozilla-beta?
Attachment #8840398 - Flags: approval-mozilla-beta-
Attachment #8840398 - Flags: approval-mozilla-aurora?
Attachment #8840398 - Flags: approval-mozilla-aurora+
Comment on attachment 8840398 [details]
Bug 1339782 - use a WeakMap to keep track of windows with active findbar highlighters and make sure no JS error occurs when a window is closed.

fix a memory leak in esr52
Attachment #8840398 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.