413296 - Leaking object on window-close

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Joey Minta]

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]

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

It could possibly be that the calendar observer is not being removed. Putting on my plate to investigate.

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Sorry, not what I expected. I'm not sure what is causing this. Removing from plate.

Comment 3

[David Baron :dbaron:]

Attachment: C++ and JS stacks showing use of leaked object

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

[Michiel van Leeuwen (email: mvl+moz@)]

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

[Michiel van Leeuwen (email: mvl+moz@)]

(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

[Berend Cornelius]

As we agreed upon in the last status call we should remove this from the blocking list.

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

I found a few non-removed observers. taking.

Comment 8

[Stefan Sitter [:ssitter]]

See also related Bug 364513 for leaked observers on close.

Comment 9

[David Baron :dbaron:]

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

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: Partial patch [checked in]

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

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 12

[Simon Paquet [:sipaq]]

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

[Michiel van Leeuwen (email: mvl+moz@)]

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

[Simon Paquet [:sipaq]]

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.

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 304507 [details] [diff] [review]
Partial patch [checked in]

Looks good, r=philipp

Comment 16

[gekacheka]

Attachment: v1 patch: Patch calendarManager and compositeCalendar so its StatusObserver doesn't leak.

(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.

Comment 17

[gekacheka]

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

[Daniel Boelzle [:dbo]]

mvl, are you still working on this one?

Comment 19

[Michiel van Leeuwen (email: mvl+moz@)]

No, not really.

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I think we can close this bug. I've tested with the leak monitor 0.4.4 but no further leaks showed up.