Closed Bug 413296 Opened 12 years ago Closed 10 years ago

Leaking object on window-close

Categories

(Calendar :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jminta, Unassigned)

References

Details

Attachments

(2 files, 1 obsolete file)

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?
It could possibly be that the calendar observer is not being removed. Putting on my plate to investigate.
Assignee: nobody → philipp
Sorry, not what I expected. I'm not sure what is causing this. Removing from plate.
Assignee: philipp → nobody
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.
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.
(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.
As we agreed upon in the last status call we should remove this from the blocking list.
Flags: blocking-calendar0.8+ → blocking-calendar0.8-
I found a few non-removed observers. taking.
Assignee: nobody → mvl
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.
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 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 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.
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.
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 on attachment 304507 [details] [diff] [review]
Partial patch [checked in]

Looks good, r=philipp
Attachment #304507 - Flags: review?(philipp) → review+
Attachment #304507 - Attachment description: Partial patch → Partial patch [checked in]
Depends on: 419601
Flags: blocking-calendar0.8- → wanted-calendar0.9+
(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 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.
Depends on: 437944
mvl, are you still working on this one?
Flags: wanted-calendar0.9+ → wanted-calendar1.0+
No, not really.
Assignee: mvl → nobody
Status: ASSIGNED → NEW
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: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.9
You need to log in before you can comment on or make changes to this bug.