sporadic 136-144 byte leak of nsAlertsIconListener + nsStringBuffer on Linux

RESOLVED FIXED in mozilla1.9.3a1

Status

()

defect
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: Dolske, Assigned: ventnor.bugzilla)

Tracking

({intermittent-failure, memory-leak})

Trunk
mozilla1.9.3a1
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +

Firefox Tracking Flags

(status1.9.2 ?)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

10 years ago
dholbert noticed this was leaking. Bug 516124 just touched this code, but I suspect the leak is more a result of the new test added there exposing an existing leak.

This is on the mochitest part 5 runs, which include that test.

s: moz2-linux-slave11TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 144 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of nsAlertsIconListener with size 60 bytes each (120 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of nsStringBuffer with size 8 bytes each (24 bytes total)

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258577061.1258578888.9742.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258580024.1258582032.13141.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258566221.1258567533.7279.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258559978.1258562085.6208.gz
Keywords: mlk
Assignee

Comment 1

10 years ago
Does disabling the newly added test (so only running the first alerts-related test) stop the leak?
Reporter

Comment 2

10 years ago
Don't know, sounds like something to try!
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258601414.1258602941.22908.gz
Linux mozilla-central debug test mochitests-5/5 on 2009/11/18 19:30:14
s: moz2-linux-slave26
Assignee

Comment 4

10 years ago
Ah! I know why!

We addref ourselves (ie. the nsAlertsIconListener) while the alert is being shown so we are able to listen for the "closed" GObject signal and fire the "alertfinished" callback. We don't unref ourselves until the alert disappears.

It seems mochitest is finishing before the alert disappears, so the ref is still being held.

This explains why we only see this on the async test (noobserve).

Damn it to hell. I don't know how to fix this without completely ruining the whole point of the test.
Assignee

Comment 5

10 years ago
By the way, because we unref it anyway when the alert disappears, this is not really a memory leak in the harmful sense of the word. It's more of a cheeky race condition between the amount of time the alert is shown, and the rest of the mochitests. The mochitests always win.
Can the shown alert be canceled during shutdown?
Assignee

Comment 7

10 years ago
Not that I know of, no.
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258647493.1258648435.22306.gz
Linux mozilla-central debug test mochitests-5/5 on 2009/11/19 08:18:13
s: moz2-linux-slave16
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 144 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of nsAlertsIconListener with size 60 bytes each (120 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of nsStringBuffer with size 8 bytes each (24 bytes total)
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258659065.1258661236.10899.gz
Linux mozilla-central debug test mochitests-5/5 on 2009/11/19 11:31:05
s: moz2-linux-slave18
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 144 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of nsAlertsIconListener with size 60 bytes each (120 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of nsStringBuffer with size 8 bytes each (24 bytes total)
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258680349.1258681772.17668.gz
Linux mozilla-central debug test mochitests-5/5 on 2009/11/19 17:25:49
s: moz2-linux-slave42
Assignee

Comment 11

10 years ago
Can we just disable the test?
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258792826.1258793807.18167.gz
Linux mozilla-central debug test mochitests-5/5 on 2009/11/21 00:40:26
s: moz2-linux-slave02
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1258799379.1258800534.28390.gz
Linux mozilla-central debug test mochitests-5/5 on 2009/11/21 02:29:39
s: moz2-linux-slave11
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1259126412.1259127368.4851.gz
Linux mozilla-central debug test mochitests-5/5 on 2009/11/24 21:20:12
s: moz2-linux-slave12
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1259214062.1259214993.31840.gz
Linux mozilla-central debug test mochitests-5/5 on 2009/11/25 21:41:02
s: moz2-linux-slave42
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6-Unittest/1259602213.1259604646.27388.gz&fulltext=1
Linux mozilla-1.9.2 test mochitests on 2009/11/30 09:30:13
Blocks: 438871
Whiteboard: [orange]
Duplicate of this bug: 531891
Linux mozilla-central debug test mochitests-5/5 [testfailed] Started 12:31, finished 12:51
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1259699461.1259700626.5751.gz&fulltext=1
How about each nsAlertsIconListener observes the nsIObserverService quit-application message and uses notify_notification_close() on receipt?
Assignee

Comment 22

10 years ago
Good idea. This seems to do it.

This is hg queued on top of bug 530921, so that has to land first.
Attachment #415557 - Flags: review?(roc)
Assignee

Updated

10 years ago
Attachment #415557 - Flags: approval1.9.2?
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6-Unittest/1259903391.1259905670.31594.gz
Linux mozilla-1.9.2 test mochitests on 2009/12/03 21:09:51
Summary: sporadic 144 byte leak of nsAlertsIconListener + nsStringBuffer on Linux → sporadic 136-144 byte leak of nsAlertsIconListener + nsStringBuffer on Linux
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1260302640.1260303345.10710.gz
Linux mozilla-central debug test mochitests-5/5 on 2009/12/08 12:04:00
s: moz2-linux-slave26
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1260604038.1260604879.12448.gz
Linux mozilla-central debug test mochitests-5/5 on 2009/12/11 23:47:18
s: moz2-linux-slave26
Keywords: checkin-needed
Whiteboard: [orange] → [orange][needs landing]
http://hg.mozilla.org/mozilla-central/rev/efa4edae64d9
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [orange][needs landing] → [orange]
Target Milestone: --- → mozilla1.9.3a1
Reopened, as this still appears to be happening:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1260789207.1260790395.15147.gz
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Also on 1.9.2 branch:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6/1260992517.1260996536.29119.gz
Linux mozilla-1.9.2 test mochitests on 2009/12/16 11:41:57
s: moz2-linux-slave11
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6/1261194108.1261197168.12123.gz
Linux mozilla-1.9.2 test mochitests on 2009/12/18 19:41:48
s: moz2-linux-slave08
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1261323464.1261324483.4126.gz
Linux mozilla-central debug test mochitests-5/5 on 2009/12/20 07:37:44
s: moz2-linux-slave09
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6/1261347744.1261350749.32367.gz
Linux mozilla-1.9.2 test mochitests on 2009/12/20 14:22:24
s: moz2-linux-slave14
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1261519325.1261520400.29183.gz
Linux mozilla-central debug test mochitests-5/5 on 2009/12/22 14:02:05
s: moz2-linux-slave20
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1262216742.1262217603.1421.gz
Linux mozilla-central debug test mochitests-5/5 on 2009/12/30 15:45:42
s: moz2-linux-slave23
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1262737090.1262738838.27680.gz
Linux mozilla-central debug test mochitests-5/5 on 2010/01/05 16:18:10
s: moz2-linux-slave14
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1262730783.1262732183.16693.gz
Linux mozilla-central debug test mochitests-5/5 on 2010/01/05 14:33:03
s: moz2-linux-slave25
(In reply to comment #11)
> Can we just disable the test?

Please do, unless you can figure out a workaround. Random orange is bad!
Assignee

Comment 44

10 years ago
Christ...
The patch is still in, right? Because without the patch I can reproduce the leak and with the path I can't...
Assignee

Comment 45

10 years ago
Posted patch Disable test (obsolete) — Splinter Review
I have no reservations with taking this option. This leak doesn't happen, and isn't a problem, in real world conditions.
Attachment #415557 - Attachment is obsolete: true
Attachment #420457 - Flags: review?(roc)
Attachment #415557 - Flags: approval1.9.2?
I'm not comfortable just disabling the test, since we don't know what the problem is.
Didn't he explain it in comment 4?
No, a patch for that was checked in in comment #29.
Assignee

Comment 49

10 years ago
Posted patch Patch 2 (obsolete) — Splinter Review
OK, I have one last theory.

The original patch requested libnotify to close the alert for us. It did so and sent out a "closed" signal, but we're not being receptive to it because we're busy shutting down. Within that callback was where the releasing happens.

So, let's try making the releasing synchronous.
Attachment #420457 - Attachment is obsolete: true
Attachment #420468 - Flags: review?(roc)
Attachment #420457 - Flags: review?(roc)
Assignee

Comment 50

10 years ago
Posted patch Patch 2.1 (obsolete) — Splinter Review
Attachment #420469 - Flags: review?
Assignee

Comment 51

10 years ago
Comment on attachment 420469 [details] [diff] [review]
Patch 2.1

Damn Enter key...

IRL review, use Release().

Like I said, I can't reproduce this so I don't know if this will work...
Attachment #420469 - Flags: review? → review?(roc)
Assignee

Updated

10 years ago
Attachment #420468 - Attachment is obsolete: true
Attachment #420468 - Flags: review?(roc)
Assignee

Comment 52

10 years ago
Comment from karlt, we should disconnect the closure.
Attachment #420469 - Attachment is obsolete: true
Attachment #420472 - Flags: review?(roc)
Attachment #420469 - Flags: review?(roc)
Comment on attachment 420472 [details] [diff] [review]
Patch 2.2
[Checked in: Comment 61]

>-    notify_notification_close(mNotification, NULL);

There is a change of behavior here.  Previously the notification would be closed during application quit; now the notification continues to show after the application quits.
I don't know whether that is good or bad.  Nice to give the user a chance to see it, but could be confusing if it has an action.

>+    g_signal_handler_disconnect(mNotification, mClosureHandler);
>+    g_object_unref(mNotification);
>+    mNotification = NULL;

Do we know that this is always the last reference to the NotifyNotification?
I'm guessing it probably is.  If not, this action will need to be cleared:
http://hg.mozilla.org/mozilla-central/annotate/82daaccfd2b9/toolkit/system/gnome/nsAlertsIconListener.cpp#l236
Assignee

Comment 54

10 years ago
(In reply to comment #53)
> (From update of attachment 420472 [details] [diff] [review])
> >-    notify_notification_close(mNotification, NULL);
> 
> There is a change of behavior here.  Previously the notification would be
> closed during application quit; now the notification continues to show after
> the application quits.
> I don't know whether that is good or bad.  Nice to give the user a chance to
> see it, but could be confusing if it has an action.

I just didn't want to run the risk of us getting the "closed" callback and doing a double-free.

> >+    g_signal_handler_disconnect(mNotification, mClosureHandler);
> >+    g_object_unref(mNotification);
> >+    mNotification = NULL;
> 
> Do we know that this is always the last reference to the NotifyNotification?
> I'm guessing it probably is.  If not, this action will need to be cleared:
> http://hg.mozilla.org/mozilla-central/annotate/82daaccfd2b9/toolkit/system/gnome/nsAlertsIconListener.cpp#l236

It would be. If not, the DBus callback would just be safely lost, wouldn't it? It goes through DBus rather than a straight C-callback IIRC, libnotify is just a library on our side that adds a nice clean C API.
(In reply to comment #54)
> > Do we know that this is always the last reference to the NotifyNotification?
> > I'm guessing it probably is.  If not, this action will need to be cleared:
> > http://hg.mozilla.org/mozilla-central/annotate/82daaccfd2b9/toolkit/system/gnome/nsAlertsIconListener.cpp#l236
> 
> It would be. If not, the DBus callback would just be safely lost, wouldn't it?

It gets safely lost if this is the last reference to the NotifyNotification because its destructor removes its dbus signal handler.
(If the NotifyNotification continues to live then it could receive the signal and effect the action.)
Assignee

Comment 56

10 years ago
I do believe its the last reference, when I first tried this out at an Ubuntu Developer Summit way back when I originally and naively just unref'ed after the notification was shown, and got crashes :)

There's a client-server structure here, the notification server will continue to show the alert even if the NotifyNotification doesn't exist anymore.
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1262973808.1262974666.1999.gz
Linux mozilla-central debug test mochitests-5/5 on 2010/01/08 10:03:28
s: moz2-linux-slave21
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1263224561.1263226146.18454.gz
Linux mozilla-central debug test mochitests-5/5 on 2010/01/11 07:42:41  
s: moz2-linux-slave07
Whiteboard: [orange] → [orange][needs landing]
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1263403945.1263405017.15004.gz
Linux mozilla-central debug test mochitests-5/5 on 2010/01/13 09:32:25
s: moz2-linux-slave08
http://hg.mozilla.org/mozilla-central/rev/9aa39a24d7fc
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [orange][needs landing] → [orange]
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6/1263600008.1263602254.6788.gz
Linux mozilla-1.9.2 test mochitests on 2010/01/15 16:00:08
s: moz2-linux-slave23

(don't forget we'll be wanting it on 1.9.2 too)
Flags: wanted1.9.2+
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6/1265134694.1265139419.16444.gz
Linux mozilla-1.9.2 test mochitests on 2010/02/02 10:18:14
s: moz2-linux-slave24
Comment on attachment 420472 [details] [diff] [review]
Patch 2.2
[Checked in: Comment 61]

One of the most common random oranges on the branch. No known regressions.
Attachment #420472 - Flags: approval1.9.2.4?
Comment on attachment 420472 [details] [diff] [review]
Patch 2.2
[Checked in: Comment 61]

Clearing old approval requests now that 1.9.2.4 has shipped. If you believe this patch is still necessary on the 1.9.2 branch please re-request approval along with a risk/benefit analysis explaining why we need it.
Attachment #420472 - Flags: approval1.9.2.4?
Linux mozilla-1.9.2 test mochitests on 2010/08/30 23:46:51  
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6/1283237211.1283239473.11478.gz
s: moz2-linux-slave18
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 136 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 136 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of nsAlertsIconListener with size 56 bytes each (112 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of nsAlertsIconListener with size 56 bytes each (112 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of nsStringBuffer with size 8 bytes each (24 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 3 instances of nsStringBuffer with size 8 bytes each (24 bytes total)
Attachment #420472 - Attachment description: Patch 2.2 → Patch 2.2 [Checked in: Comment 61]
Attachment #415557 - Attachment description: Patch → Patch [Checked in: Comment 29]
Attachment #415557 - Attachment is obsolete: false
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6/1296456113.1296458458.6338.gz
Linux mozilla-1.9.2 test mochitests on 2011/01/30 22:41:53
{
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 128 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 128 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of nsAlertsIconListener with size 56 bytes each (112 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of nsAlertsIconListener with size 56 bytes each (112 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of nsStringBuffer with size 8 bytes each (16 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of nsStringBuffer with size 8 bytes each (16 bytes total)
}
status1.9.2: --- → ?
Depends on: 635552
Depends on: 654411
No longer depends on: 654411
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.