Last Comment Bug 317478 - leaks due to global scope polluter being removed from not enough (?) prototype chains
: leaks due to global scope polluter being removed from not enough (?) prototyp...
Status: RESOLVED FIXED
[patch]
: fixed1.8.0.1, fixed1.8.1, mlk
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
: Hixie (not reading bugmail)
Mentors:
http://tv.yahoo.com/grid/?lineup=us_C...
Depends on:
Blocks: mlk1.8
  Show dependency treegraph
 
Reported: 2005-11-22 14:25 PST by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2006-03-12 19:02 PST (History)
4 users (show)
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
IRC log (from #content) in which we diagnosed the problem (13.99 KB, text/plain; charset=utf-8)
2005-11-22 23:24 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
logging code that I mentioned in the IRC discussion (2.73 KB, patch)
2005-11-22 23:26 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
jst's preferred approach for fixing the bug (2.99 KB, patch)
2005-11-22 23:26 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
mrbkap: review+
bzbarsky: superreview+
dveditz: approval1.8.0.1+
dveditz: approval1.8.1+
Details | Diff | Splinter Review
unsimplified testcase (.tar.gz) (39.82 KB, application/x-gzip)
2005-11-22 23:35 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-11-22 14:25:02 PST
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.
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-11-22 20:21:06 PST
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).
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-11-22 23:24:44 PST
Created attachment 204018 [details]
IRC log (from #content) in which we diagnosed the problem
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-11-22 23:26:20 PST
Created attachment 204019 [details] [diff] [review]
logging code that I mentioned in the IRC discussion
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-11-22 23:26:57 PST
Created attachment 204020 [details] [diff] [review]
jst's preferred approach for fixing the bug
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-11-22 23:35:50 PST
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 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-11-22 23:37:04 PST
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.
Comment 7 Boris Zbarsky [:bz] 2005-11-23 05:42:51 PST
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...
Comment 8 Blake Kaplan (:mrbkap) 2005-11-23 10:28:38 PST
Comment on attachment 204020 [details] [diff] [review]
jst's preferred approach for fixing the bug

r=mrbkap
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2005-11-23 15:35:46 PST
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).
Comment 10 Daniel Veditz [:dveditz] 2005-12-20 14:54:50 PST
Comment on attachment 204020 [details] [diff] [review]
jst's preferred approach for fixing the bug

a=dveditz for drivers
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-12-20 17:54:45 PST
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.

Note You need to log in before you can comment on or make changes to this bug.