Last Comment Bug 312769 - Webshell leak when closing any page with meta refresh
: Webshell leak when closing any page with meta refresh
: fixed1.8, mlk, regression, testcase
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: x86 Linux
: P1 normal (vote)
: mozilla1.8rc1
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: Adam Lock
: Andrew Overholt [:overholt]
Depends on:
Blocks: 301358
  Show dependency treegraph
Reported: 2005-10-17 15:17 PDT by Blake Kaplan (:mrbkap)
Modified: 2006-03-12 18:59 PST (History)
5 users (show)
mscott: blocking1.8rc1+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (46 bytes, text/html)
2005-10-17 15:29 PDT, Blake Kaplan (:mrbkap)
no flags Details
Proposed fix (3.17 KB, patch)
2005-10-17 15:58 PDT, Boris Zbarsky [:bz] (still a bit busy)
darin.moz: review+
bryner: superreview+
Details | Diff | Splinter Review
Updated to comments (3.15 KB, patch)
2005-10-17 18:44 PDT, Boris Zbarsky [:bz] (still a bit busy)
asa: approval1.8rc1+
Details | Diff | Splinter Review

Description Blake Kaplan (:mrbkap) 2005-10-17 15:17:33 PDT
I noticed today that visiting caused us
to leak webshells (the last line I get while shutting down is --WEBSHELL
0x86c1268 == 3).

1. Start browser with the URL
2. Close browser.
3. Notice the leaks.

I confirmed that I see this both on trunk and the 1.8 branch.
Comment 1 Blake Kaplan (:mrbkap) 2005-10-17 15:29:36 PDT
Created attachment 199858 [details]

More digging shows that simply loading this testcase and closing the browser
causes a leak.
Comment 2 Blake Kaplan (:mrbkap) 2005-10-17 15:51:49 PDT
bz debugged this and pointed his finger at bug 301358. This leak affects any
pages that use <meta http-equiv='refresh'> and bz says it's a big leak, so I'm
nominating this to block 1.8rc1.
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2005-10-17 15:58:17 PDT
Created attachment 199861 [details] [diff] [review]
Proposed fix

This just makes sure to cancel all the timers in Destroy().
Comment 4 Darin Fisher 2005-10-17 17:23:14 PDT
Comment on attachment 199861 [details] [diff] [review]
Proposed fix

>+    // Cancel any timers that were set for this docshell; this is need
>+    // to break the cycle between us and the timers.
>+    CancelRefreshURITimers();

"this is need[ed] to break..."

>+DoCancelRefreshURITimers(nsISupportsArray* aTimerList)
> {
>     }

nit: kill this blank line

Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2005-10-17 18:44:25 PDT
Created attachment 199877 [details] [diff] [review]
Updated to comments
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2005-10-17 18:54:15 PDT
Comment on attachment 199877 [details] [diff] [review]
Updated to comments

Requesting approval.  This fixes a leak that happens any time the user closes a
window/tab that contains a meta refresh page or navigates away from a page
which has a subframe that has a meta refresh.  We're leaking the docshell and a
bunch of stuff attached to it.

All the patch does is make sure to break the cycle with the refresh timer when
the docshell is being destroyed.  It should be quite safe.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2005-10-17 18:55:02 PDT
Fixed on trunk.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2005-10-18 06:38:38 PDT
Fixed on branch.
Comment 9 Scott MacGregor 2005-10-18 13:14:56 PDT
making the blocking flag match reality

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