Closed Bug 1387583 Opened 7 years ago Closed 7 years ago

browser.xul leaked due to selectRect in SelectParentHelper.jsm

Categories

(Core :: Layout: Form Controls, defect)

53 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: bkelly, Assigned: mconley)

References

Details

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

Attachments

(1 file)

I had a leaked perf-html.io window today.  When I closed tabs to try to isolate it the leak morphed to a leaked browser.xul.  (Or maybe they are separate leaks?)

Anyway, the one I caught here was a leaked browser.xul.  It seems to be due to selectRect in SelectParentHelper.jsm.

bkelly@valen:/mnt/c/devel/tmp/cclogs$ /mnt/c/devel/heapgraph/find_roots.py cc-edges.7172.1501876971.log 000001B62DA56000
Parsing cc-edges.7172.1501876971.log. Done loading graph.

000001B62D602060 [JS Object (Window)]
    --[UnwrapDOMObject(obj)]--> 000001B62DA56000 [nsGlobalWindow # 6 inner chrome://browser/content/browser.xul]

    Root 000001B62D602060 is a marked GC object.

bkelly@valen:/mnt/c/devel/tmp/cclogs$ /mnt/c/devel/heapgraph/find_roots.py gc-edges.7172.1501876971.log -bro 000001B62D6
02060
Parsing gc-edges.7172.1501876971.log. Done loading graph.

via persistent-Object :
000001B634E0B880 [BackstagePass 000001B62AF11CA0]
    --[selectRect]--> 000001B653174200 [Proxy <no private>]
    --[private]--> 000001B62EC1EDA0 [Object <no private>]
    --[group]--> 000001B628BFB670 [object_group]
    --[group_global]--> 000001B62D602060 [Window <no private>]

It seems selectRect was added in bug 1311279.  Not sure if it leaked then or if the leak started later, though.
Component: Form Manager → Layout: Form Controls
Product: Toolkit → Core
See Also: → 1388583
Mike, do you know who could look at this front end leak?
Flags: needinfo?(mconley)
Whiteboard: [MemShrink] → [MemShrink:P2]
Whoa, that's weird. Rect is just a dumb object with left, top, width and height numeric properties. How could that be keeping a window alive?

If an object was passed through the message manager (via structured clone), does holding onto that object somehow keep the window that it passed up through alive? Even if it's just holding onto primitives? That'd be kinda mind-blowing.
Flags: needinfo?(mconley) → needinfo?(bugs)
Well, the rect is coming from some window context. How could the global go away if there are still objects kept alive from that scope.
Flags: needinfo?(bugs)
Yeah, the rect object is being held alive across compartments. An easy fix would be to create a new rect object from the fields on the object that was passed in.
Assignee: nobody → mconley
Comment on attachment 8895996 [details]
Bug 1387583 - Avoid leaking a window in SelectParentHelper.

https://reviewboard.mozilla.org/r/167264/#review172510

TIL...
Attachment #8895996 - Flags: review?(jaws) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24449b2f0d3b
Avoid leaking a window in SelectParentHelper. r=jaws
https://hg.mozilla.org/mozilla-central/rev/24449b2f0d3b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Could you uplift this to beta? It seems fairly low risk.
Flags: needinfo?(mconley)
Keywords: mlk, regression
Version: unspecified → 53 Branch
Comment on attachment 8895996 [details]
Bug 1387583 - Avoid leaking a window in SelectParentHelper.

Approval Request Comment
[Feature/Bug causing the regression]:

e10s.

[User impact if declined]:

It's possible to leak one window if a <select> dropdown was opened for that window.

[Is this code covered by automated tests?]:

Yes, select dropdowns are.

[Has the fix been verified in Nightly?]:

Insomuch as the dropdowns still work properly, yes. The leak was properly identified and cleared here, but I didn't manually confirm this (though the culprit is being nulled out here, so if that doesn't clear the leak, we have bigger problems).

[Needs manual test from QE? If yes, steps to reproduce]: 

No.

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

Absolutely not.

[Why is the change risky/not risky?]:

We're just clearing a variable once the dropdown popup goes away. There was never really a reason to hold onto it in the first place.

[String changes made/needed]:

None.
Flags: needinfo?(mconley)
Attachment #8895996 - Flags: approval-mozilla-beta?
Comment on attachment 8895996 [details]
Bug 1387583 - Avoid leaking a window in SelectParentHelper.

Fix a regression. Beta56+.
Attachment #8895996 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.