The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: dbaron, Assigned: jst)

Tracking

({fixed1.8.0.1, fixed1.8.1, mlk})

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch], URL)

Attachments

(4 attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Updated

12 years ago
Whiteboard: [patch]
(Reporter)

Comment 1

12 years ago
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).
(Reporter)

Updated

12 years ago
Summary: leaks due to nsJSEventListener locking the window → leaks due to global scope polluter being removed from not enough (?) prototype chains
(Reporter)

Comment 2

12 years ago
Created attachment 204018 [details]
IRC log (from #content) in which we diagnosed the problem
(Reporter)

Comment 3

12 years ago
Created attachment 204019 [details] [diff] [review]
logging code that I mentioned in the IRC discussion
(Reporter)

Comment 4

12 years ago
Created attachment 204020 [details] [diff] [review]
jst's preferred approach for fixing the bug
(Reporter)

Comment 5

12 years ago
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)
(Reporter)

Comment 6

12 years ago
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

12 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: 12 years ago
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?
Resolution: --- → FIXED
(Reporter)

Updated

11 years ago
Assignee: dbaron → jst
(Reporter)

Updated

11 years ago
Attachment #204020 - Flags: approval1.8.1?
Attachment #204020 - Flags: approval1.8.0.1?
(Reporter)

Updated

11 years ago
Blocks: 320915
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+
(Reporter)

Comment 11

11 years ago
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
(Reporter)

Updated

11 years ago
Keywords: fixed1.8.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.