XPCJSStackFrame::CreateStack cause permanent memory grow ~1Mb per second

RESOLVED FIXED in mozilla1.9.2

Status

()

defect
P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: romaxa, Assigned: smaug)

Tracking

1.9.2 Branch
mozilla1.9.2
x86
Linux
Points:
---
Bug Flags:
blocking1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta2-fixed)

Details

(URL)

Attachments

(13 attachments, 1 obsolete attachment)

3.57 KB, application/x-gzip
Details
25.99 KB, application/x-debian-package
Details
6.22 KB, text/plain
Details
3.67 KB, application/x-gzip
Details
671 bytes, patch
smaug
: review-
Details | Diff | Splinter Review
4.52 KB, patch
Details | Diff | Splinter Review
9.07 KB, patch
Details | Diff | Splinter Review
304 bytes, text/html
Details
2.67 KB, patch
Details | Diff | Splinter Review
9.89 KB, patch
Details | Diff | Splinter Review
8.15 KB, patch
Details | Diff | Splinter Review
1019 bytes, patch
peterv
: review+
mrbkap
: review+
Details | Diff | Splinter Review
968 bytes, patch
mrbkap
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
Posted file mybrowser
Download:
http://ftp.mozilla.org/pub/mozilla.org/xulrunner/nightly/latest-mozilla-1.9.2/xulrunner-1.9.2a2pre.en-US.linux-i686.tar.bz2

http://benjamin.smedbergs.us/xulrunner/mybrowser-0.2.2.xulapp - or take from attachment

./run-mozilla.sh ./xulrunner-bin /tmp/mybrowser/application.ini 

In parallel run (sp-memusage package from maemo)
mem-monitor-smaps -c -p xulrunner-bin

Open facebook.com page (startup page, no credentials available)
Reload that page 2-3 times (probably it is not required)

Check mem-monitor-smaps output

EXP: Memory is not growing
ACT:
for ./xulrunner-bin[21230] according to SMAPS.
(without swap as SMAPS doesn't report swap correctly)

		system	process		private	/------ dirty ---------\
time:		avail:	size:	rss:	clean:	shared:	private: change:
14:28:34	2810064	115764	38036	13320	0	18492	   +0 kB
14:28:38	2831840	116788	39060	13320	0	19516	+1024 kB
14:28:40	2844172	118836	37220	13324	0	17672	-1844 kB
14:28:43	2868524	118836	37996	13324	0	18448	 +776 kB
14:28:45	2812312	118836	38536	13324	0	18988	 +540 kB
14:28:47	2841360	118836	39256	13324	0	19708	 +720 kB
14:28:49	2842816	118836	40004	13324	0	20456	 +748 kB
14:28:51	2816892	118836	40460	13324	0	20912	 +456 kB
14:28:53	2819824	118836	41000	13324	0	21452	 +540 kB
14:28:55	2770896	118836	41112	13324	0	21564	 +112 kB
14:29:00	2857716	119860	42136	13324	0	22588	+1024 kB
14:29:02	2823132	120884	43160	13324	0	23612	+1024 kB
14:29:06	2839360	121908	44184	13324	0	24636	+1024 kB
14:29:08	2782144	122932	45208	13324	0	25660	+1024 kB
14:29:10	2857352	123956	46232	13324	0	26684	+1024 kB
14:29:14	2819664	124980	47256	13324	0	27708	+1024 kB
14:29:16	2836336	126004	48280	13324	0	28732	+1024 kB
14:29:20	2792724	127028	49304	13324	0	29756	+1024 kB
14:29:23	2790648	128052	50328	13324	0	30780	+1024 kB
14:29:25	2876252	129076	51352	13324	0	31804	+1024 kB
14:29:27	2831828	130100	52376	13324	0	32828	+1024 kB
14:29:31	2819000	131124	53400	13324	0	33852	+1024 kB
....
14:30:37	2756368	155700	77988	13308	0	58428	+1024 kB
....
(Reporter)

Comment 1

10 years ago
(Reporter)

Comment 2

10 years ago
I found that facebook page executing script
http://static.ak.fbcdn.net/rsrc.php/z4QA3/hash/9y5lydl4.js:139
And it always throw exception...
Memory grow, but never free.
(Assignee)

Updated

10 years ago
Flags: blocking1.9.2?
(Reporter)

Comment 3

10 years ago
Also if I call memory-pressure/heapminimize, then memory returns back to the system (nsJSContext:CC() executed).
Or page reload
5:04:44	2844052	172104	86160	13276	0	66556	+1024 kB
15:04:48	2842636	173128	87184	13276	0	67580	+1024 kB
15:05:11	2835808	178256	92372	13276	0	72768	+5188 kB
15:05:13	2887204	174176	40480	13276	0	20876	-51892 kB
15:05:15	2887372	174176	40644	13276	0	21040	 +164 kB
15:05:17	2887440	174176	41228	13276	0	21624	 +584 kB
15:05:19	2886684	174176	41744	13276	0	22140	 +516 kB
15:05:21	2886716	174176	42356	13276	0	22752	 +612 kB
One problem might be that nsXPCException isn't cycle collected, so we don't try to CC even though a number of them got created (but even then, we don't take into account how big the objects are).
(Reporter)

Comment 5

10 years ago
Attachment #401635 - Flags: review?(Olli.Pettay)
Assignee: nobody → Olli.Pettay
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2
(Assignee)

Comment 6

10 years ago
Comment on attachment 401635 [details] [diff] [review]
Can we call CC sometime if use inactive?

I thought I reviewed this already.
I don't think we want to call CC too often. Better to fix nsXPCException handling.
Attachment #401635 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 7

10 years ago
(In reply to comment #6)
> Better to fix nsXPCException handling.
Or whatever causes this bug.

Romaxa, I'm having hard time trying to reproduce this.
Any help appreciated.

(Though, if the problem is something that only sp-memusage debian package shows,
I have the problem that I don't have debian nearby.)
(Reporter)

Comment 8

10 years ago
> (Though, if the problem is something that only sp-memusage debian package
> shows,
> I have the problem that I don't have debian nearby.)

What do you have ? is it linux? if it is linux, then you can just unpack debian package and use library and binary from there... otherwise (Win/MacOS/Solaris) you better know some memory measurement (per process) tool for your system.
(Assignee)

Comment 9

10 years ago
Romaxa, with this bug report, do you mean the loop is endless and you
constantly leak something, until a new page is loaded so that CC is triggered?

(I didn't use sp-memusage since I don't have any unpack deb tool here, and couldn't find such immediately. But other tools should work well enough.)
(Assignee)

Comment 10

10 years ago
Romaxa, any help here would be great.
What kind of leak do you see? Is it a constant 1Mb per second? or what?
Hard to reproduce.
(Reporter)

Comment 11

10 years ago
Checked again by using xulrunner:
http://hg.mozilla.org/mozilla-central/rev/1904598c4caf

extract mybrowser application from attachments:
./run-mozilla.sh ./xulrunner-bin  /tmp/mybrowser/application.ini

open facebook.com
run in other terminal:
while true; do cat /proc/30946/status  | grep Vm; sleep 1; done

reload it facebook page, keep mouse focus in browser facebook window (window is focused).

Check for VmRSS: state in other terminal.

You should see that it is growing.
(Assignee)

Comment 12

10 years ago
(In reply to comment #11)
> Check for VmRSS: state in other terminal.
> 
> You should see that it is growing.
In my earlier testing I did see it growing a bit, but then it dropped down.
I'll test this again.
(Reporter)

Comment 13

10 years ago
When test is started, don't do anything, don't touch mouse, don't change focus, no any actions which may cause user-active/inactive switch.
(Assignee)

Comment 14

10 years ago
Posted patch for debuggingSplinter Review
I wonder how to really make this work nicely.
Exceptions are "thread safe", so I don't think we can cycle collect all of them.
But would it be bad to have main thread only exceptions, which would be
CC-able. That might break something. And is ugly too.

Peterv, any suggestions?
(Assignee)

Comment 15

10 years ago
Just an idea. Need to think about cross thread exception handling.
(Assignee)

Comment 16

10 years ago
Posted file testcase
Run in FF. Don't move mouse over the window.
(Assignee)

Comment 17

10 years ago
So we create lots of XPCWrappedNative objects, but there is nothing
which would trigger cycle collector which could then clean up
unnecessary wrappers.
(Assignee)

Comment 18

10 years ago
This is an old regression. 1.8.x cleans up the objects when GC runs.
1.9.x doesn't.
(Assignee)

Comment 19

10 years ago
Or not quite that way. We don't seem to trigger gc often enough.
A patch coming.
(Assignee)

Comment 20

10 years ago
This is pretty safe, but ugly hack to trigger gc occasionally.
(Assignee)

Comment 21

10 years ago
This works pretty well with the testcase and with mybrowser+facebook.
And pages like gmail or gmaps don't trigger more cycle collections than currently.

I'm not quite sure if sXPCWrappedNativeCount should count only
wrappers created in/for main thread, but seems like this approach is simpler 
and NS_MAX_NEW_WRAPPED_NATIVES is pretty high so it doesn't get triggered too
easily.

Note, this is 1.9.2+ so I don't want to do too big changes.
Attachment #405149 - Flags: review?(peterv)
Since we're looking at hacks anyway and using the number of XPCWrappedNatives created to trigger GCs, can you try calling updateMallocCounter with the size of the XPCWrappedNative in XPCWrappedNative::FinishInit?

I'm not too fond of adding a counter for XPCWrappedNatives (btw, if we do we can probably use the cheaper non-atomic increment/decrement).
(Assignee)

Comment 23

10 years ago
Instead of increasing memory usage 150MB, this increases only 100MB before GC.
(Assignee)

Comment 24

10 years ago
Posted patch updateMallocCounter * 2 (obsolete) — Splinter Review
This gives a bit more reasonable behavior.
Pure hack.
Attachment #406928 - Flags: review?(peterv)
(Assignee)

Comment 25

10 years ago
Comment on attachment 406928 [details] [diff] [review]
updateMallocCounter * 2

Argh, this contains non-sense libffi.info part
(Assignee)

Comment 26

10 years ago
Attachment #406928 - Attachment is obsolete: true
Attachment #406933 - Flags: review?(peterv)
Attachment #406928 - Flags: review?(peterv)
Comment on attachment 406933 [details] [diff] [review]
updateMallocCounter * 2

I don't like it, but I don't really know a fix I would like better.
Attachment #406933 - Flags: review?(peterv) → review+
Blake, Igor, hope you guys are ok with this too.
(Assignee)

Updated

10 years ago
Attachment #406933 - Flags: review?(mrbkap)
Comment on attachment 406933 [details] [diff] [review]
updateMallocCounter * 2

>diff --git a/js/src/xpconnect/src/xpcwrappednative.cpp b/js/src/xpconnect/src/xpcwrappednative.cpp
>+    // A hack for bug 517665, increase the probability for GC.
>+    JSContext* cx = ccx.GetJSContext();
>+    if (cx) {
>+        cx->updateMallocCounter(2 * sizeof(XPCWrappedNative));
>+    }

We cannot have a null cx here (the ccx would be invalid, which should have been caught earlier). I'm about as happy with the patch as everybody else, but given how much science has gone into GC until now (tongue firmly in cheek), this doesn't seem to be any worse than anything else we've done.
Attachment #406933 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 30

10 years ago
Posted patch no null checkSplinter Review
Attachment #409065 - Flags: review?(mrbkap)
Attachment #409065 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 31

10 years ago
http://hg.mozilla.org/mozilla-central/rev/554d3a8cc6ff
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 33

10 years ago
Not likely, but possible.
(Assignee)

Comment 34

10 years ago
I yet decided to try this on 1.9.2.
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3a99a35cf4c5
(Assignee)

Comment 35

10 years ago
At least on 1.9.2 I can see any tdhtml changes on Windows.
(Assignee)

Updated

10 years ago
Attachment #405149 - Flags: review?(peterv)
You need to log in before you can comment on or make changes to this bug.