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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

Attachment: IRC log (from #content) in which we diagnosed the problem

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: logging code that I mentioned in the IRC discussion

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: jst's preferred approach for fixing the bug

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

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 [:bzbarsky]]

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) (inactive)]

Comment on attachment 204020 [details] [diff] [review]
jst's preferred approach for fixing the bug

r=mrbkap

Comment 9

[Johnny Stenback (:jst)]

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]]

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-4, no longer working on Mozilla)]

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.