Closed
Bug 413296
Opened 17 years ago
Closed 15 years ago
Leaking object on window-close
Categories
(Calendar :: General, defect)
Calendar
General
Tracking
(Not tracked)
RESOLVED
FIXED
0.9
People
(Reporter: jminta, Unassigned)
References
Details
Attachments
(2 files, 1 obsolete file)
25.76 KB,
text/plain; charset=UTF-8
|
Details | |
18.87 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
Steps to reproduce:
1.) Open Sunbird with a clean profile
2.) Install dbaron's leak monitor extension (need to modify install.rdf)
3.) Restart to complete installation
4.) After restart, close the Sunbird window but do not quit the application.
Expected results:
Nothing
Actual results:
Leak report
Given the release goals of "no leaks" I assume this should be blocking.
Leaks in window 0x1a0ce00:
[+] [leaked object] (1a0da88) = [Object]
[ ] mDefaultCalendarItem = null
[+] QueryInterface (1a0da98, chrome://calendar/content/calendar-management.js, 623-630) = [Function]
[ ] prototype (18ad5c0) = [Object]
[+] initializeCalendar (1a0daa8, chrome://calendar/content/calendar-management.js, 638-651) = [Function]
[ ] prototype (18ad698) = [Object]
[+] setupWritableCalendars (1a0dab8, chrome://calendar/content/calendar-management.js, 654-661) = [Function]
[ ] prototype (18ad6b0) = [Object]
[+] onCalendarRegistered (1a0dac8, chrome://calendar/content/calendar-management.js, 666-672) = [Function]
[ ] prototype (18ad718) = [Object]
[+] onCalendarUnregistering (1a0dad8, chrome://calendar/content/calendar-management.js, 676-687) = [Function]
[ ] prototype (18ad728) = [Object]
[+] onCalendarDeleting (1a0dae8, chrome://calendar/content/calendar-management.js, 690-690) = [Function]
[ ] prototype (18ad748) = [Object]
[+] onStartBatch (1a0daf8, chrome://calendar/content/calendar-management.js, 695-695) = [Function]
[ ] prototype (18ad760) = [Object]
[+] onEndBatch (1a0db08, chrome://calendar/content/calendar-management.js, 696-696) = [Function]
[ ] prototype (18ad788) = [Object]
[+] onLoad (1a0db18, chrome://calendar/content/calendar-management.js, 697-697) = [Function]
[ ] prototype (18ad7c0) = [Object]
[+] onAddItem (1a0db28, chrome://calendar/content/calendar-management.js, 699-700) = [Function]
[ ] prototype (18ad830) = [Object]
[+] onModifyItem (1a0db38, chrome://calendar/content/calendar-management.js, 703-704) = [Function]
[ ] prototype (18adcc0) = [Object]
[+] onDeleteItem (1a0db48, chrome://calendar/content/calendar-management.js, 707-707) = [Function]
[ ] prototype (18adce8) = [Object]
[+] onError (1a0db58, chrome://calendar/content/calendar-management.js, 708-708) = [Function]
[ ] prototype (18ae3b0) = [Object]
[+] onPropertyChanged (1a0db68, chrome://calendar/content/calendar-management.js, 713-727) = [Function]
[ ] prototype (18ae3d0) = [Object]
[+] onPropertyDeleting (1a0db78, chrome://calendar/content/calendar-management.js, 732-736) = [Function]
[ ] prototype (18ae3e8) = [Object]
[+] observe (1a0db88, chrome://calendar/content/calendar-management.js, 740-785) = [Function]
[ ] prototype (1887fd0) = [Object]
Flags: blocking-calendar0.8?
Comment 1•17 years ago
|
||
It could possibly be that the calendar observer is not being removed. Putting on my plate to investigate.
Assignee: nobody → philipp
Comment 2•17 years ago
|
||
Sorry, not what I expected. I'm not sure what is causing this. Removing from plate.
Assignee: philipp → nobody
Updated•17 years ago
|
Flags: blocking-calendar0.8? → blocking-calendar0.8+
This leak is responsible for the second of the two "###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!" assertions that happen on shutdown of a clean profile.
This attachment contains the C++ and JS stacks for that assertion. (The JS stack is what I'm getting with all the printing of stuff.) This should show who it is that's holding on to the object -- although the JS stack is convoluted enough that I'm not entirely sure at first glance, although I think it's something like the calendarManager still having a pointer through its mCache.
Comment 4•17 years ago
|
||
My 'comment code out until the error is gone' way of debugging points to an observer added to all calendars that's never removed. At least, removing this line:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/calendar/base/content/calendar-management.js&rev=1.21&mark=682#668
makes the error from the leak-monitor extension go away.
Comment 5•17 years ago
|
||
(In reply to comment #4)
scrach that comment. It's only true if the observers are not added. Maybe there are two leaks, I don't know.
Comment 6•17 years ago
|
||
As we agreed upon in the last status call we should remove this from the blocking list.
Flags: blocking-calendar0.8+ → blocking-calendar0.8-
Comment 8•17 years ago
|
||
See also related Bug 364513 for leaked observers on close.
Comment on attachment 301188 [details]
C++ and JS stacks showing use of leaked object
Just to clarify what this stack is (as line numbers change):
> native = 0x2aaab12048d8 <XPC_WN_GetterSetter(JSContext*, JSObject*, unsigned int, long*, long*)>
> filename = 0x116679d "file:///home/dbaron/builds/trunk/obj/sunbird-debug/dist/bin/js/calUtils.js", lineno = 1055,
compareObjects
> filename = 0x116679d "file:///home/dbaron/builds/trunk/obj/sunbird-debug/dist/bin/js/calUtils.js", lineno = 1539,
neq
> filename = 0x116679d "file:///home/dbaron/builds/trunk/obj/sunbird-debug/dist/bin/js/calUtils.js", lineno = 1536,
calInterfaceBag_remove
> filename = 0x13493fd "file:///home/dbaron/builds/trunk/obj/sunbird-debug/dist/bin/js/calProviderBase.js", lineno = 213,
cPB_removeObserver
> native = 0x2aaab1204b2e <XPC_WN_CallMethod(JSContext*, JSObject*, unsigned int, long*, long*)>
> filename = 0x11524cd "file:///home/dbaron/builds/trunk/obj/sunbird-debug/dist/bin/js/calAlarmService.js", lineno = 382,
cas_unobserveCalendar
> filename = 0x11524cd "file:///home/dbaron/builds/trunk/obj/sunbird-debug/dist/bin/js/calAlarmService.js", lineno = 335,
cas_shutdown
> filename = 0x11524cd "file:///home/dbaron/builds/trunk/obj/sunbird-debug/dist/bin/js/calAlarmService.js", lineno = 188,
cas_observe
(based on the C++ stack also in the attachment, we know it's an "xpcom-shutdown" notification)
This stack is showing how the object in comment 0 is still reachable -- I verified that it's that object in this log as well: the first property it has that's a function is cMO_QueryInterface.
Comment 10•17 years ago
|
||
This patch remove a bunch of observers on windowclose/shutdown. It does make the assertions go away. But unfortunately, the leak still shows. Not sure why. (fun fact: it doesn't show on trunk)
Some things about the patch: It makes the alarmservice and the calendarmanager real services, by starting them on app-startup, and closing when the app shuts down. For alarm service, that's really needed, because it should be able to fire alarms when the main window is closed.
The assertion was because the observer of the calendar manager was not removed at shutdown. By then, other observers were removed, which cause a comparison between those observers, requiring xpcom. But the window that created the calmgr observer was already gone. So, the assert showed.
Why the leak is still there is unclear to me.
Comment 11•17 years ago
|
||
Comment on attachment 304507 [details] [diff] [review]
Partial patch [checked in]
Asking review on this patch, because it fixes at least some issues. Maybe not all yet, but can't hurt to start with what I've got now.
Attachment #304507 -
Attachment description: WIP patch → Partial patch
Attachment #304507 -
Flags: review?(philipp)
Comment 12•17 years ago
|
||
Comment on attachment 304507 [details] [diff] [review]
Partial patch [checked in]
>Index: sunbird/base/content/calendar.xul
>===================================================================
>[...]
> <!-- All JS files that calendar.xul wishes to include *must* go into the calendar-scripts.inc
> file (all scripts shared with Lightning) or sunbird-scripts.inc file (scripts relevant
> for Sunbird-only)so that they can be shared by hiddenWindow.xul. -->
> #include ../../../base/content/calendar-scripts.inc
> #include sunbird-scripts.inc
>
>+ <script type="application/x-javascript" src="chrome://calendar/content/calendar-unifinder.js"/>
>+ <script type="application/x-javascript" src="chrome://calendar/content/calendar-unifinder-todo.js"/>
>+
>
>Index: sunbird/base/content/sunbird-scripts.inc
>===================================================================
>[...]
> <script type="application/x-javascript" src="chrome://calendar/content/calendarWindow.js"/>
> <script type="application/x-javascript" src="chrome://calendar/content/calendar-offline.js"/>
>- <script type="application/x-javascript" src="chrome://calendar/content/calendar-unifinder.js"/>
>- <script type="application/x-javascript" src="chrome://calendar/content/calendar-unifinder-todo.js"/>
mvl, what is the rationale for this change? I can't find any explanation related to this change in comment 0, comment 9 or comment 10.
You are partly reverting the change from attachment 294865 [details] [diff] [review] (Bug 386432). The aim of that patch was to keep all script inclusions in a separate file to improve code readability.
Comment 13•17 years ago
|
||
The unifinder scripts add listeners to window (un)load, by statements in the global scope. This means that the listeners are also added to the hiddenwindow. But the hiddenwindow does not use any of the functions in the unifinder scripts. Therefore, the addition if the eventlisteners is a waste of time. I could add some more runtime-checks in the unifinder scripts, but why not fix it earlier? The checks also take a little bit of time.
And it's in this bug, because one of the things that's being done is adding listeners to the composite calendar if it's not the hidden window, and removing it for all windows. That's not good. Removing something that doesn't exists. The opposite of the problem in this bug, but related.
Comment 14•17 years ago
|
||
Thanks for the explanation, mvl. Could you please add a comment to calendar.xul explaining why the JS files are directly referenced there and not in sunbird-scripts.inc
Otherwise I fear somebody will file a cleanup bug in six months and the reviewer will probably not remember this bug then.
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: PC → All
Version: Sunbird 0.7 → unspecified
Comment 15•17 years ago
|
||
Comment on attachment 304507 [details] [diff] [review]
Partial patch [checked in]
Looks good, r=philipp
Attachment #304507 -
Flags: review?(philipp) → review+
Updated•17 years ago
|
Attachment #304507 -
Attachment description: Partial patch → Partial patch [checked in]
Updated•17 years ago
|
Flags: blocking-calendar0.8- → wanted-calendar0.9+
Comment 16•17 years ago
|
||
(patch -l -p 1 -i file.patch)
Following comment 0 steps to reproduce, Leak Monitor 3.6 was showing the status observer leaking. This patch fixes that.
Attachment #337305 -
Flags: review?(Berend.Cornelius)
Attachment #337305 -
Attachment is obsolete: true
Attachment #337305 -
Flags: review?(Berend.Cornelius)
Comment 17•17 years ago
|
||
Comment on attachment 337305 [details] [diff] [review]
v1 patch: Patch calendarManager and compositeCalendar so its StatusObserver doesn't leak.
Oops, just noticed there's already a separate bug for this. Moving to bug 437944.
Comment 18•17 years ago
|
||
mvl, are you still working on this one?
Flags: wanted-calendar0.9+ → wanted-calendar1.0+
Updated•17 years ago
|
Status: ASSIGNED → NEW
Comment 20•15 years ago
|
||
I think we can close this bug. I've tested with the leak monitor 0.4.4 but no further leaks showed up.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.9
You need to log in
before you can comment on or make changes to this bug.
Description
•