Closed Bug 1095714 Opened 7 years ago Closed 7 years ago

SearchWindow registers but does not unregister settings observers

Categories

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.1+, b2g-v1.4 unaffected, b2g-v2.0 affected, b2g-v2.0M affected, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S9 (21Nov)
blocking-b2g 2.1+
Tracking Status
b2g-v1.4 --- unaffected
b2g-v2.0 --- affected
b2g-v2.0M --- affected
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: tkundu, Assigned: kgrandon)

References

Details

(Whiteboard: [systemsfe][MemShrink])

Attachments

(2 files)

Attached file memory-reports
STR:

1) Run an automation script similar to https://github.com/mozilla-b2g/gaia/blob/v2.1/tests/python/gaia-ui-tests/gaiatest/tests/functional/browser/test_browser_lan.py to open google website in browser app.
2) Run |adb shell kill `adb shell b2g-ps | grep Browser | awk '{print $4}'`| to kill both search app and browser app .
3) Repeat 1 and 2 .


@kyle: I attached memory reports . It seems like both keyboard app and b2g is leaking memory.
[Blocking Requested - why for this release]:
blocking-b2g: --- → 2.1?
Flags: needinfo?(khuey)
Flags: needinfo?(erahm)
Flags: needinfo?(erahm)
Whiteboard: [MemShrink]
b2g process has a *ton* of queued IPC messages (although the count is 0, so maybe that's not correct), snippet:

0 (100.0%) -- queued-ipc-messages
├──0 (100.0%) ── content-parent((Preallocated), pid=18761, open channel, 0xa241f1f4, refcnt=16)
├──0 (100.0%) ── content-parent(Browser, pid=-1, closed channel, 0x9e44b5f4, refcnt=1)
├──0 (100.0%) ── content-parent(Browser, pid=-1, closed channel, 0x9fd019f4, refcnt=1)
├──0 (100.0%) ── content-parent(Browser, pid=-1, closed channel, 0x9fd01df4, refcnt=1)
├──0 (100.0%) ── content-parent(Browser, pid=-1, closed channel, 0x9fd021f4, refcnt=1)
├──0 (100.0%) ── content-parent(Browser, pid=-1, closed channel, 0x9fd025f4, refcnt=1)
├──0 (100.0%) ── content-parent(Browser, pid=-1, closed channel, 0x9fd029f4, refcnt=1)
├──0 (100.0%) ── content-parent(Browser, pid=-1, closed channel, 0x9fd02df4, refcnt=1)


Majority if memory is heap-unclassified, which might make sense if we're not reporting memory used for queued messages:

94.86 MB (100.0%) -- explicit
├──30.05 MB (31.68%) ── heap-unclassified

The keyboard process is using a ridiculous amount of memory, the majority in one zone:

110.43 MB (100.0%) -- explicit
├───78.67 MB (71.24%) -- js-non-window
│   ├──75.52 MB (68.39%) -- zones
│   │  ├──74.85 MB (67.78%) -- zone(0xb3d39400)
│   │  │  ├──70.62 MB (63.95%) -- compartment([System Principal], outOfProcessTabChildGlobal)
│   │  │  │  ├──64.44 MB (58.35%) -- classes
│   │  │  │  │  ├──24.72 MB (22.38%) -- class(Function)
│   │  │  │  │  │  ├──13.42 MB (12.15%) -- objects
│   │  │  │  │  │  │  ├───9.31 MB (08.43%) ── gc-heap
│   │  │  │  │  │  │  └───4.11 MB (03.72%) ── malloc-heap/slots
│   │  │  │  │  │  └──11.30 MB (10.23%) -- shapes
│   │  │  │  │  │     ├───8.22 MB (07.44%) -- gc-heap
│   │  │  │  │  │     │   ├──5.14 MB (04.65%) ── base
│   │  │  │  │  │     │   ├──1.54 MB (01.40%) ── tree
│   │  │  │  │  │     │   └──1.54 MB (01.39%) ── dict
│   │  │  │  │  │     └───3.08 MB (02.79%) -- malloc-heap
│   │  │  │  │  │         ├──3.08 MB (02.79%) ── dict-tables
│   │  │  │  │  │         └──0.00 MB (00.00%) ++ (2 tiny)
│   │  │  │  │  ├──21.16 MB (19.16%) -- class(Object)
│   │  │  │  │  │  ├──11.96 MB (10.83%) -- objects
│   │  │  │  │  │  │  ├───7.85 MB (07.11%) ── gc-heap
│   │  │  │  │  │  │  └───4.11 MB (03.72%) ── malloc-heap/slots
│   │  │  │  │  │  └───9.20 MB (08.33%) -- shapes
│   │  │  │  │  │      ├──7.20 MB (06.52%) -- gc-heap
│   │  │  │  │  │      │  ├──4.63 MB (04.19%) ── tree
│   │  │  │  │  │      │  ├──2.57 MB (02.32%) ── base
│   │  │  │  │  │      │  └──0.00 MB (00.00%) ── dict
│   │  │  │  │  │      └──2.00 MB (01.82%) -- malloc-heap
│   │  │  │  │  │         ├──2.00 MB (01.81%) ── tree-kids
│   │  │  │  │  │         └──0.00 MB (00.00%) ++ (2 tiny)
│   │  │  │  │  ├──10.27 MB (09.30%) -- class(Array)
│   │  │  │  │  │  ├──10.27 MB (09.30%) -- objects
│   │  │  │  │  │  │  ├──10.26 MB (09.29%) ── gc-heap
│   │  │  │  │  │  │  └───0.00 MB (00.00%) ++ malloc-heap
│   │  │  │  │  │  └───0.00 MB (00.00%) ++ shapes
│   │  │  │  │  ├───3.09 MB (02.80%) -- class(Block)
│   │  │  │  │  │   ├──3.09 MB (02.80%) -- objects
│   │  │  │  │  │   │  ├──3.09 MB (02.80%) ── gc-heap
│   │  │  │  │  │   │  └──0.00 MB (00.00%) ── malloc-heap/slots
│   │  │  │  │  │   └──0.01 MB (00.00%) ++ shapes
│   │  │  │  │  ├───3.08 MB (02.79%) -- class(Proxy)
│   │  │  │  │  │   ├──3.08 MB (02.79%) ── objects/gc-heap
│   │  │  │  │  │   └──0.00 MB (00.00%) ++ shapes/gc-heap
│   │  │  │  │  ├───2.05 MB (01.86%) -- class(XPCWrappedNative_NoHelper)
│   │  │  │  │  │   ├──2.05 MB (01.86%) -- objects
│   │  │  │  │  │   │  ├──2.05 MB (01.86%) ── gc-heap
│   │  │  │  │  │   │  └──0.00 MB (00.00%) ── malloc-heap/slots
│   │  │  │  │  │   └──0.00 MB (00.00%) ++ shapes
│   │  │  │  │  └───0.07 MB (00.06%) ++ class(<non-notable classes>)
│   │  │  │  ├───6.00 MB (05.43%) ── compartment-tables
│   │  │  │  └───0.18 MB (00.16%) ++ (2 tiny)
│   │  │  ├───3.04 MB (02.75%) ── unused-gc-things
│   │  │  └───1.19 MB (01.08%) ++ (10 tiny)

And there's a ton of suspect observers on inner-window-destroyed:
67,435 (100.0%) -- observer-service
└──67,435 (100.0%) -- referent
   ├──67,361 (99.89%) ── strong
   └──────74 (00.11%) ++ weak

67,249 (100.0%) -- observer-service-suspect
└──67,249 (100.0%) ── referent(topic=inner-window-destroyed)
Do you have GC/CC logs?  That memory report indicates that we're leaking a lot of iframes.
Flags: needinfo?(khuey) → needinfo?(tkundu)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #3)
> Do you have GC/CC logs?  That memory report indicates that we're leaking a
> lot of iframes.

Yes. 

GC CC logs : https://drive.google.com/file/d/0B1cSMS8_GuAESEFheUdVVDdoYlE/view?usp=sharing
Flags: needinfo?(tkundu) → needinfo?(khuey)
Thanks for that report.  mccr8 and I identified two issues.

1. The observer at [0] is registered but never unregistered.  The SearchWindow entrains the <iframe mozbrowser> producing the symptoms visible in the memory report.
2. Bug 1048833 is back on 2.1 somehow in the keyboard process.  Perhaps that fix does not work in the keyboard process for some reason (does it create a new window every time)?

Let's morph this to cover #1, and I'll clone bug 1048833 for #2.

[0] http://mxr.mozilla.org/gaia/source/apps/system/js/search_window.js#13
Component: Stability → Gaia::System::Browser Chrome
Flags: needinfo?(khuey) → needinfo?(kgrandon)
Summary: browser stability runs leaks memory → SearchWindow registers but does not unregister settings observers
Well this is a bad one. We really need memory tests for gaia =(
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
Flags: needinfo?(kgrandon)
Whiteboard: [MemShrink] → [systemsfe][MemShrink]
Target Milestone: --- → 2.1 S7 (24Oct)
Comment on attachment 8519705 [details] [review]
[PullReq] KevinGrandon:bug_1095714_search_window_listener to mozilla-b2g:master

Hey Alive - this seems like the best fix to me right now. What do you think?
Attachment #8519705 - Flags: review?(alive)
Comment on attachment 8519705 [details] [review]
[PullReq] KevinGrandon:bug_1095714_search_window_listener to mozilla-b2g:master

I am not sure if it's me r+ the settings listener in searchWindow;
but to me the correct way to implement is move that out to rocketbar(searchWindowManager) and watch settings there.

*Window is not designed to observe settings right now.
Attachment #8519705 - Flags: review?(alive) → review+
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #9)
> Comment on attachment 8519705 [details] [review]
> [PullReq] KevinGrandon:bug_1095714_search_window_listener to
> mozilla-b2g:master
> 
> I am not sure if it's me r+ the settings listener in searchWindow;
> but to me the correct way to implement is move that out to
> rocketbar(searchWindowManager) and watch settings there.
> 
> *Window is not designed to observe settings right now.

Thanks, I've commented in bug 1075701 and we should track that work there.

It also looks like this might be present on v2.0, but it is less impactful there as the only entrypoint to this is from the home screen, and users would probably use that less. Updating the tracking flags to reflect this.
In master: https://github.com/mozilla-b2g/gaia/commit/59013e78cbd999ea85ac6258fe2945e050dbd1ab
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Memory and CAF blocker
blocking-b2g: 2.1? → 2.1+
Unable to verify issue due to STR being automation steps for a back-end issue
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-] → [QAnalyst-Triage+][QAnalyst-verify-]
Flags: needinfo?(ktucker)
Please request Gaia v2.1 approval on this when you get a chance.
Flags: needinfo?(kgrandon)
Target Milestone: 2.1 S7 (24Oct) → 2.1 S9 (21Nov)
Comment on attachment 8519705 [details] [review]
[PullReq] KevinGrandon:bug_1095714_search_window_listener to mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Feature implementation.
[User impact] if declined: Memory leak eventually resulting in a device crash.
[Testing completed]: Manual and simple unit test added.
[Risk to taking this patch] (and alternatives if risky): Fairly simple patch which is only around search app closing - should be low risk.
[String changes made]: None.
Flags: needinfo?(kgrandon)
Attachment #8519705 - Flags: approval-gaia-v2.1?
Attachment #8519705 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1+
You need to log in before you can comment on or make changes to this bug.