leaks due to global scope polluter being removed from not enough (?) prototype chains

RESOLVED FIXED

Status

()

RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: dbaron, Assigned: jst)

Tracking

({fixed1.8.0.1, fixed1.8.1, memory-leak})

Trunk
fixed1.8.0.1, fixed1.8.1, memory-leak
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch], URL)

Attachments

(4 attachments)

I'm seeing leaks on the above URL due to nsJSEventListener calling JS_LockGCThing on the window (code added for bug 304430).  Presumably either (1) there's a property on the window that keeps the content alive (2) something else keeps the content alive via the window or (3) the event listener is on the window itself.  (1) and (3) could definitely cause cycles; not sure about (2) but I don't care all that much.

I think we need to do the alternate solution in bug 304430 comment 16.  (I don't see any way to do this using wrapper preservation, nor do I see any reason any wrappers need to be preserved.)

I plan to work on this after bug 241518 lands (to avoid adding merge conflicts with myself), although someone else could take it first.
So, actually, jst convinced me that this probably isn't the cycle that I think it is:  (1) isn't a problem because we clear scope and he says he tested (3).
Summary: leaks due to nsJSEventListener locking the window → leaks due to global scope polluter being removed from not enough (?) prototype chains
Created attachment 204021 [details]
unsimplified testcase (.tar.gz)

For the record, here's the testcase.

Steps to reproduce:
 1. tar xzvf 317478.tar.gz
 2. load 317478.html
 3. close the browser window *without leaving the page* (if you leave the page it doesn't leak)
Comment on attachment 204020 [details] [diff] [review]
jst's preferred approach for fixing the bug

I'll take the liberty of requesting reviews on jst's behalf.  I don't feel like a qualified reviewer for this.
Attachment #204020 - Flags: superreview?(bzbarsky)
Attachment #204020 - Flags: review?(mrbkap)
Comment on attachment 204020 [details] [diff] [review]
jst's preferred approach for fixing the bug

sr=bzbarsky, but please add a comment saying that InstallGlobalScopePolluter needs to be called after we've finished messing with the inner's prototype chain...
Attachment #204020 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 204020 [details] [diff] [review]
jst's preferred approach for fixing the bug

r=mrbkap
Attachment #204020 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 9

13 years ago
Fix checked in, with a comment added to explain why...

Nominating for 1.8.* since this could be leaking on a lot of sites (all sites that actually rely on the GSP).
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?
Resolution: --- → FIXED
Comment on attachment 204020 [details] [diff] [review]
jst's preferred approach for fixing the bug

a=dveditz for drivers
Attachment #204020 - Flags: approval1.8.1?
Attachment #204020 - Flags: approval1.8.1+
Attachment #204020 - Flags: approval1.8.0.1?
Attachment #204020 - Flags: approval1.8.0.1+
Fix checked in to MOZILLA_1_8_BRANCH, 2005-12-20 17:35 -0800.
Fix checked in to MOZILLA_1_8_0_BRANCH, 2005-12-20 17:38 -0800.
Keywords: fixed1.8.0.1
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1+
You need to log in before you can comment on or make changes to this bug.