Don't entrain numeric properties of windows

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

({memory-leak})

Trunk
x86
Linux
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
Currently, in order to resolve window[0], we define a JSVAL_VOID property and fill in the value in the getter. Unfortunately, we don't define the numeric properties with JSPROP_SHARED, which means that the resolved value gets stuck on the window object (or the XPCNativeWrapper around the window object).

peterv has been investigating a leak on gmail and seems to have found that we are entraining an iframe in an XPCNativeWrapper's "0" property. Because the only reference to this iframe appears to be from the XPCNativeWrapper and the wrapper probably lives in sessionstore, this seems like a good candidate for the leak.

The fix, whether or not it fixes the gmail leak, is a good one, and we should take it anyway. If it does fix the leak, then we'll probably want to backport it to older branches.
(Assignee)

Comment 1

8 years ago
Created attachment 443490 [details] [diff] [review]
Proposed fix
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #443490 - Flags: review?(jst)
Are there any other cases in classinfo that define non-shared props?  Other than window.document there shouldn't be any, I'd think....
Comment on attachment 443490 [details] [diff] [review]
Proposed fix

r=jst, and yeah, we should audit for other code like this as well...
Attachment #443490 - Flags: review?(jst) → review+
(Assignee)

Comment 5

8 years ago
I started looking through nsDOMClassInfo.cpp and didn't finish. I'll look now. There are a couple of cases like the fast expando code where lack of JSPROP_SHARED is actually correct.
Keywords: mlk
(Assignee)

Comment 6

8 years ago
http://hg.mozilla.org/mozilla-central/rev/78d48a1a8239
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.