window.open()/.close() memory leak in content process

RESOLVED DUPLICATE of bug 961793

Status

Firefox OS
General
P1
blocker
RESOLVED DUPLICATE of bug 961793
4 years ago
3 years ago

People

(Reporter: m1, Assigned: huseby)

Tracking

(Blocks: 1 bug, {mlk, perf, regression})

unspecified
1.3 C3/1.4 S3(31jan)
ARM
Gonk (Firefox OS)
mlk, perf, regression

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [c=memory p=2 s= u=1.3] [CR 606932],QARegressExclude)

Attachments

(5 attachments)

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

4 years ago
Opps, test app is really at bug 833077 attachment 704662 [details]
(Reporter)

Updated

4 years ago
blocking-b2g: --- → 1.3+
status-b2g-v1.3: --- → affected

Updated

4 years ago
Whiteboard: [CR 606932]
Mike,

Please help with next steps here as it blocks QC.
Flags: needinfo?(mlee)

Comment 3

4 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
Assignee: nobody → dhuseby
Status: NEW → ASSIGNED
Flags: needinfo?(mlee)
Keywords: mlk, perf
Whiteboard: [CR 606932] → [c=memory p= s= u=1.3] [CR 606932]
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Any idea of a regression range?
(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

4 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.
Component: Gaia::System::Window Mgmt → General
Created attachment 8366455 [details]
Test App
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

4 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

4 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

4 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?
Is this reproducible on 1.3 after 20 mins?

Please post logs here post 20 mins.
(Reporter)

Comment 13

4 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

4 years ago
Flags: needinfo?(jsmith)
(Assignee)

Comment 14

4 years ago
Created attachment 8366998 [details]
v1.4_WindowTest_Memory.png

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

4 years ago
Created attachment 8367023 [details]
v1.3_WindowTest_Memory.png

Here's a graph on v1.3 tip.  The memory leak is definitely present.
(Assignee)

Comment 16

4 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

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

4 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

4 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

4 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

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

4 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

4 years ago
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86_64 → ARM
(Assignee)

Comment 24

4 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

4 years ago
*drum roll*
(Assignee)

Comment 26

4 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

4 years ago
Keywords: regressionwindow-wanted
(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.
It looks like this is Bug 961793
fwiw, this looks safe to uplift to 1.3

Updated

4 years ago
Depends on: 961793
(Assignee)

Comment 30

4 years ago
Created attachment 8367821 [details] [diff] [review]
Bug_964386.patch

Here's the final patch from Bug 961793.  This fixes the memory leak.
Attachment #8367821 - Flags: review?(fabrice)
(Assignee)

Comment 31

4 years ago
Created attachment 8367822 [details]
v1.3_Patched_WindowTest_Memory.png

Here's memory consumption of the WindowTest app over a 20 minute test period.  Notice that the memory leak is gone after patching.
(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

4 years ago
Jason -- Awesome!  Thanks.
(Reporter)

Comment 34

4 years ago
Thanks!
Status: ASSIGNED → RESOLVED
blocking-b2g: 1.3+ → ---
Last Resolved: 4 years ago
status-b2g-v1.3: affected → ---
No longer depends on: 961793
Resolution: --- → DUPLICATE
Duplicate of bug: 961793
Attachment #8367821 - Flags: review?(fabrice)

Updated

4 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

4 years ago
See Also: → bug 998504
(Reporter)

Updated

3 years ago
See Also: → bug 1137875
You need to log in before you can comment on or make changes to this bug.