Last Comment Bug 170022 - leaks of Observer service through cycles
: leaks of Observer service through cycles
Status: RESOLVED FIXED
[patch]
: mlk
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: x86 Linux
: P3 normal (vote)
: Future
Assigned To: David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
: Paul Wyskoczka
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-09-20 21:24 PDT by David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
Modified: 2004-11-23 18:54 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (diff -u) (4.69 KB, patch)
2002-09-20 21:25 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (diff -uw) (3.32 KB, patch)
2002-09-20 21:25 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review
revised patch (diff -u) (4.87 KB, patch)
2002-10-13 07:53 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review
revised patch (diff -uw) (4.00 KB, patch)
2002-10-13 07:54 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
morse: review+
jag-mozilla: superreview+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-09-20 21:24:44 PDT
In my build I'm seeing some leaks of the observer service and some corresponding
XPConnect wrappers (going both ways) through cycles where observers implemented
in JS have some path to the observer service (which prevents the observer
service's wrapper from being GCed), and the observer service then in turn owns
the observers (through an nsSupportsArray).

I'm not sure why these leaks don't show up on tinderbox.  There were two GC
paths I fixed:

08370748 object 0x83a9c40 XPCWrappedNative_NoHelper via
nsXPCWrappedJS::mJSObj(Function).__parent__(ChromeWindow).observerService(XPCWrappedNative_NoHelper).
I fixed with the changes to cookieTasksOverlay.xul

086b4958 object 0x86c3970 XPCWrappedNative_NoHelper via
nsXPCWrappedJS::mJSObj(Object).observe(Function).__parent__(Call).service(XPCWrappedNative_NoHelper).
I fixed with changes to navigator.js

There are a bunch of other places that look suspicious but that probably didn't
get executed:
http://lxr.mozilla.org/seamonkey/source/extensions/cookie/resources/content/cookieTasksOverlay.xul#100
http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/content/pref-charset.xul#37
http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/content/pref-languages.xul#35
http://lxr.mozilla.org/seamonkey/source/mailnews/base/prefs/resources/content/pref-viewing_messages.xul#15
http://lxr.mozilla.org/seamonkey/source/mailnews/base/resources/content/folderProps.xul#41
http://lxr.mozilla.org/seamonkey/source/mailnews/compose/prefs/resources/content/pref-composing_messages.xul#14
Comment 1 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-09-20 21:25:32 PDT
Created attachment 100072 [details] [diff] [review]
patch (diff -u)
Comment 2 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-09-20 21:25:53 PDT
Created attachment 100073 [details] [diff] [review]
patch (diff -uw)
Comment 3 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-09-22 12:46:31 PDT
Brendan pointed out that an alternative (and preferred) solution for the closure
cycle is to either null out the service variable or not use a variable at all
(since it's only used once).
Comment 4 Brendan Eich [:brendan] 2002-09-22 12:56:40 PDT
... so service would have to be changed from const to var if it were kept.

The cookie patch looks like it just avoids unnecessary global variables, which
can entrain lots of garbage too.

/be
Comment 5 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-10-13 07:53:55 PDT
Created attachment 102720 [details] [diff] [review]
revised patch (diff -u)
Comment 6 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-10-13 07:54:10 PDT
Created attachment 102721 [details] [diff] [review]
revised patch (diff -uw)
Comment 7 Stephen P. Morse 2002-10-13 20:41:59 PDT
Comment on attachment 102721 [details] [diff] [review]
revised patch (diff -uw)

r=morse
Comment 8 jag (Peter Annema) 2002-10-13 21:06:15 PDT
Comment on attachment 102721 [details] [diff] [review]
revised patch (diff -uw)

sr=jag
Comment 9 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2002-11-06 06:22:50 PST
Fix checked in to trunk, 2002-11-06 04:54/55 PDT.

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