Closed
Bug 964386
Opened 11 years ago
Closed 11 years ago
window.open()/.close() memory leak in content process
Categories
(Firefox OS Graveyard :: General, defect, P1)
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 961793
1.3 C3/1.4 S3(31jan)
People
(Reporter: m1, Assigned: huseby)
References
Details
(Keywords: memory-leak, perf, regression, Whiteboard: [c=memory p=2 s= u=1.3] [CR 606932],QARegressExclude)
Attachments
(5 files)
A memory leak is observed on v1.3 in content processes that repeatedly invoke window.open()/close() (eg. Communication app receiving MT calls repeatedly).
The test app at bug 833077 attachment 704697 [details] can be used to easily reproduce after 10-20min.
Reporter | ||
Comment 1•11 years ago
|
||
Opps, test app is really at bug 833077 attachment 704662 [details]
Reporter | ||
Updated•11 years ago
|
blocking-b2g: --- → 1.3+
status-b2g-v1.3:
--- → affected
Comment 2•11 years ago
|
||
Mike,
Please help with next steps here as it blocks QC.
Flags: needinfo?(mlee)
Comment 3•11 years ago
|
||
Dave,
Need you to take this issue and consider it your highest priority this sprint. Reach out to Ben Kelly, Kyle Huey, and/or Nicholas Nethercote as needed for help troubleshooting and resolving this.
Thanks,
Mike
Comment 4•11 years ago
|
||
Any idea of a regression range?
Comment 5•11 years ago
|
||
(In reply to Dietrich Ayala (:dietrich) from comment #4)
> Any idea of a regression range?
If we can get clear STR with expected/actual results off that test app, then I can put someone on here to get a regression range.
Reporter | ||
Comment 6•11 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #5)
> If we can get clear STR with expected/actual results off that test app, then
> I can put someone on here to get a regression range.
STR:
1. Run the app, hit "Start"
2. Run |adb shell watch b2g-procrank|. Watch the USS field for the app grow, within 10-15 min the memory usage is clearly abnormal. Keep going until it gets LMKed.
Updated•11 years ago
|
Component: Gaia::System::Window Mgmt → General
Comment 7•11 years ago
|
||
Comment 8•11 years ago
|
||
To get the window, do the following:
1. Push the test app to your device using the app manager
** https://developer.mozilla.org/en-US/Firefox_OS/Using_the_App_Manager
2. Follow the STR in comment 6 on a Buri 1.1 device & the latest 1.3 Buri device and identify the bug present here via comparing 1.1 vs. 1.3 behavior based on what comment 6 states
3. After you see the bug present here, work on getting the window here using the directions from comment 6
Keywords: regression,
regressionwindow-wanted
Reporter | ||
Comment 9•11 years ago
|
||
FWIW, I don't observe this issue at the tip of v1.2. Who is running the steps described in comment 8?
Flags: needinfo?(jsmith)
Comment 10•11 years ago
|
||
(In reply to Michael Vines [:m1] [:evilmachines] from comment #9)
> FWIW, I don't observe this issue at the tip of v1.2. Who is running the
> steps described in comment 8?
Hello Michael, on 1.1 when running the Window test no windows actually open and close, but the completed count still rises along with the memory going up very slowly compared to 1.3. It's been going for about an hour now w/o LMKing. On 1.3 the app works properly (opening and closing windows), but I didn't get a LMK after 20min so I went to check 1.1. In short, what is the time range to LMK that I should be looking for when finding the regression window?
Reporter | ||
Comment 11•11 years ago
|
||
Not sure about 1.1, I only tried on 1.2 and 1.3.
What does |adb shell b2g-procrank| output for you on 1.3 after running the app for 10-20min?
Comment 12•11 years ago
|
||
Is this reproducible on 1.3 after 20 mins?
Please post logs here post 20 mins.
Reporter | ||
Comment 13•11 years ago
|
||
It is reproduceable in ~3minutes:
Start:
Every 2s: b2g-procrank 1970-01-01 20:59:22
APPLICATION PID Vss Rss Pss Uss cmdline
b2g 326 84800K 72744K 58957K 54200K /system/b2g/b2g
Homescreen 953 33996K 33992K 20800K 16344K /system/b2g/plugin-container
WindowTest 1193 33956K 30944K 18317K 14380K /system/b2g/plugin-container
(Preallocated a 1466 24240K 24236K 12777K 9448K /system/b2g/plugin-container
Stop:
Every 2s: b2g-procrank 1970-01-01 21:02:44
APPLICATION PID Vss Rss Pss Uss cmdline
b2g 326 90696K 71824K 57952K 53188K /system/b2g/b2g
WindowTest 1193 55952K 46124K 33406K 29440K /system/b2g/plugin-container
Homescreen 953 33984K 33980K 20736K 16256K /system/b2g/plugin-container
(Preallocated a 1466 24228K 24224K 12674K 9320K /system/b2g/plugin-container
Note the 100% growth in WindowTest USS. On v1.2 this is not observed.
Reporter | ||
Updated•11 years ago
|
Flags: needinfo?(jsmith)
Assignee | ||
Comment 14•11 years ago
|
||
I just ran a 15 minute test on master tip (v1.4) and there doesn't appear to be a memory leak there. See graph of memory usage over time.
Assignee | ||
Comment 15•11 years ago
|
||
Here's a graph on v1.3 tip. The memory leak is definitely present.
Assignee | ||
Comment 16•11 years ago
|
||
I'm going to try out v1.3 from 1/1/14 and see if the memory leak is there. I'm hoping that will time box it to some recent changes that we can bisect.
Comment 17•11 years ago
|
||
I was able to repro this issue on the first build App Manager is able to install WindowTest, anything before the below build 10/24/2013 App Manager is not able to install WindowTest.
.:First Broken Build:.
Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20131024154023
Gaia: 2feebdb1a2583928f32407d76d798f8654621e2b
Gecko: 19fd3388c372
Version: 27.0a1
Firmware Version: v1.2-device.cfg
Keywords: regressionwindow-wanted
Comment 18•11 years ago
|
||
(In reply to gbennett from comment #17)
> I was able to repro this issue on the first build App Manager is able to
> install WindowTest, anything before the below build 10/24/2013 App Manager
> is not able to install WindowTest.
>
> .:First Broken Build:.
> Environmental Variables:
> Device: Buri 1.3 MOZ
> BuildID: 20131024154023
> Gaia: 2feebdb1a2583928f32407d76d798f8654621e2b
> Gecko: 19fd3388c372
> Version: 27.0a1
> Firmware Version: v1.2-device.cfg
Keep going farther back than this - there might have been a regression on the 10/24 Buri build where the app manager broke. I would imagine it wouldn't have been broken on every build before 10/24.
Keywords: regressionwindow-wanted
Assignee | ||
Comment 19•11 years ago
|
||
Since the bug is not found in master, it must have fixed in there between the v1.3 branch point and HEAD. I've been bisecting master to find when/where the memory leak was fixed. So far I've narrowed it down to being between 4e04586 and 678f815 on the master branch. I'll pretty soon which revision fixed the memory leak so we can figure out where the bug is.
Assignee | ||
Comment 20•11 years ago
|
||
I should point out that revision 4e04586 was on 1/23/14 and 678f815 was on 1/27/14. So I've narrowed it down to a small time window and about 100 revisions. I'm getting close.
Updated•11 years ago
|
Whiteboard: [c=memory p= s= u=1.3] [CR 606932] → [c=memory p= s= u=1.3] [CR 606932],QARegressExclude
Assignee | ||
Comment 21•11 years ago
|
||
I've got this bug down to a window of just 25 commits between 2457895 (bad) and 4544e52 (good) on the master branch.
I was scanning the commits between those two points and found commit 0a07322 with the description: Bug 961802 -- Plugged leak in getUserMedia Denied code-path.
Looking at the diff the change is in code related to windows. I'm testing that build right now to see if it fixes the memory leak.
Comment 22•11 years ago
|
||
(In reply to Dave Huseby [:huseby] from comment #21)
> I've got this bug down to a window of just 25 commits between 2457895 (bad)
> and 4544e52 (good) on the master branch.
>
> I was scanning the commits between those two points and found commit 0a07322
> with the description: Bug 961802 -- Plugged leak in getUserMedia Denied
> code-path.
>
> Looking at the diff the change is in code related to windows. I'm testing
> that build right now to see if it fixes the memory leak.
I don't think that's going to be cause of this leak. That bug deals with gUM, which isn't even involved in window.open & window.close operations.
Assignee | ||
Comment 23•11 years ago
|
||
alrighty. well, I'm down to just 20 change sets, four more builds and I'll know which revision fixed it.
Assignee | ||
Updated•11 years ago
|
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Assignee | ||
Comment 24•11 years ago
|
||
I'm checking my last revision. I'm down to two and will know very shortly which revision fixed the leak.
Reporter | ||
Comment 25•11 years ago
|
||
*drum roll*
Assignee | ||
Comment 26•11 years ago
|
||
So the winner is: d906a1c
That's the revision that fixes the memory leak. I'm going through it now to see what changes would fix the memory leak.
Updated•11 years ago
|
Keywords: regressionwindow-wanted
Comment 27•11 years ago
|
||
(In reply to Dave Huseby [:huseby] from comment #26)
> So the winner is: d906a1c
>
> That's the revision that fixes the memory leak. I'm going through it now to
> see what changes would fix the memory leak.
Gaia or Gecko? I'm trying to find the specific changeset this maps to hg or git.
Comment 28•11 years ago
|
||
It looks like this is Bug 961793
Comment 29•11 years ago
|
||
fwiw, this looks safe to uplift to 1.3
Assignee | ||
Comment 30•11 years ago
|
||
Here's the final patch from Bug 961793. This fixes the memory leak.
Attachment #8367821 -
Flags: review?(fabrice)
Assignee | ||
Comment 31•11 years ago
|
||
Here's memory consumption of the WindowTest app over a 20 minute test period. Notice that the memory leak is gone after patching.
Comment 32•11 years ago
|
||
(In reply to Dave Huseby [:huseby] from comment #30)
> Created attachment 8367821 [details] [diff] [review]
> Bug_964386.patch
>
> Here's the final patch from Bug 961793. This fixes the memory leak.
Dave - I already marked the dependency a blocker here, so it will be automatically uplifted to 1.3.
Assignee | ||
Comment 33•11 years ago
|
||
Jason -- Awesome! Thanks.
Reporter | ||
Comment 34•11 years ago
|
||
Thanks!
Status: ASSIGNED → RESOLVED
blocking-b2g: 1.3+ → ---
Closed: 11 years ago
status-b2g-v1.3:
affected → ---
No longer depends on: 961793
Resolution: --- → DUPLICATE
Updated•11 years ago
|
Attachment #8367821 -
Flags: review?(fabrice)
Updated•11 years ago
|
Whiteboard: [c=memory p= s= u=1.3] [CR 606932],QARegressExclude → [c=memory p=2 s= u=1.3] [CR 606932],QARegressExclude
Reporter | ||
Updated•11 years ago
|
Reporter | ||
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•