Closed Bug 1044125 Opened 6 years ago Closed 6 years ago

After using cards view, app screenshots are decoded in-memory whenever the screen is on

Categories

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect, P1)

x86_64
Windows 7
defect

Tracking

(blocking-b2g:2.1+, b2g-v2.0 unaffected, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S5 (26sep)
blocking-b2g 2.1+
Tracking Status
b2g-v2.0 --- unaffected
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: khuey, Assigned: aus)

References

Details

(Keywords: memory-footprint, perf, regression, Whiteboard: [MemShrink:P1][systemsfe][c=memory u= p= s=])

Attachments

(2 files)

STR:

Boot up the phone and unlock it.
Take a memory report with the -m flag.  Call this report #1.
Fire up two apps, then press the home button long enough to bring up the cards view.
Press home again to exit cards view.
Turn off the screen.
Take a memory report with the -m flag.  Call this report #2.
Turn on the screen.  You should be at the lockscreen now.  Do not unlock the phone.
Take a memory report with the -m flag.  Call this report #3.

Diff reports #2 and #3.

What happens:

I see two large images each consuming 7 MB of uncompressed-nonheap memory in the system app.  These are presumably the screenshots from the cards view, because they are not in report #1.  These should not be uncompressed in memory when the cards view is not in active use.
qawanted to see if this reproduces on 2.0.
Keywords: qawanted
I wasn't able to reproduce this on my 2.0 flame device. I see a lot of small images, but not the 2 large images uncompressed images:

├──10.38 MB (19.26%) -- images/content/raster
│  ├──10.30 MB (19.10%) -- used
│  │  ├───2.10 MB (03.89%) ++ (15 tiny)
│  │  ├───1.55 MB (02.88%) -- image(blob:36e6c36b-e7f0-433c-95e9-f55ddaa45eb3)
│  │  │   ├──1.50 MB (02.78%) ── uncompressed-nonheap
│  │  │   └──0.05 MB (00.10%) ++ (2 tiny)
│  │  ├───1.54 MB (02.86%) -- image(blob:51270eb6-de9b-46c7-9a90-40b17d02fd76)
│  │  │   ├──1.50 MB (02.78%) ── uncompressed-nonheap
│  │  │   └──0.04 MB (00.08%) ++ (2 tiny)
│  │  ├───1.53 MB (02.84%) -- image(blob:9dd35880-2a09-4a67-952a-220f10f6aa27)
│  │  │   ├──1.50 MB (02.78%) ── uncompressed-nonheap
│  │  │   └──0.03 MB (00.06%) ++ (2 tiny)
│  │  ├───1.51 MB (02.80%) -- image(blob:f304fdd2-0fe2-4225-b455-1e6e2b9fd09a)
│  │  │   ├──1.46 MB (02.72%) ── uncompressed-nonheap
│  │  │   └──0.04 MB (00.08%) ++ (2 tiny)
│  │  ├───1.47 MB (02.73%) -- image(app://system.gaiamobile.org/shared/style/value_selector/images/ui/gradient.png)
│  │  │   ├──1.46 MB (02.72%) ── uncompressed-nonheap
│  │  │   └──0.01 MB (00.01%) ++ (2 tiny)
│  │  └───0.59 MB (01.09%) -- image(app://system.gaiamobile.org/shared/style/confirm/images/ui/gradient.png)
│  │      ├──0.59 MB (01.09%) ── uncompressed-nonheap
│  │      └──0.00 MB (00.01%) ++ (2 tiny)
│  └───0.09 MB (00.16%) ++ unused
[Blocking Requested - why for this release]:

Ok, that's good.
blocking-b2g: --- → 2.1?
Keywords: qawanted
Whiteboard: [MemShrink] → [MemShrink][systemsfe]
Whiteboard: [MemShrink][systemsfe] → [MemShrink:P1][systemsfe]
Alive, are you the right person to look into this?  I suspect the cards view is still in the DOM hidden somewhere.
Flags: needinfo?(alive)
Sam, could you take a look?

Kyle, the original use case is wanna to keep the screenshot blob once we call getScreenshot() so we don't need to fetch screenshot every time we get into cardview. I think it's a tradeoff.
Flags: needinfo?(alive) → needinfo?(sfoster)
It's not the blobs I'm concerned about.  The *uncompressed* image data is in memory too, as if the blobs were being actively displayed on screen via <img> or background-image or something.
I can take a look into this, but the plan is to retire the old cards view for 2.1 and finish implementing the Haida-style task switcher. Still, we should understand this issue in case that doesn't make it and to be sure we don't carry the problem over.
Keywords: perf
Whiteboard: [MemShrink:P1][systemsfe] → [MemShrink:P1][systemsfe][c=memory u= p= s=]
Priority: -- → P1
When the card view is being shown, it currently requests a background-image url via appWindow.requestScreenshotURL. But the #cards-list element is emptied completely when the card view is hidden, so we should have nothing hanging around there. However, it does seem that the blob url remains assigned to the backgroundImage on the .browser-container > .screenshot-overlay when the cards view hides. Perhaps we could keep a reference to the blob but clear the value here, and assign it again when that screenshot overlay is needed? 
I've not succeeded in watching any effect this might have on memory usage (2 b2g processes detected error when I run ./tools/get_about_memory.py) but intuitively this seems plausible.
Flags: needinfo?(sfoster) → needinfo?(alive)
> 2 b2g processes detected error when I run ./tools/get_about_memory.py

This is bug 1048024 and/or bug 1050026.
(In reply to Sam Foster [:sfoster] from comment #8)
> When the card view is being shown, it currently requests a background-image
> url via appWindow.requestScreenshotURL. But the #cards-list element is
> emptied completely when the card view is hidden, so we should have nothing
> hanging around there. However, it does seem that the blob url remains
> assigned to the backgroundImage on the .browser-container >
> .screenshot-overlay when the cards view hides. Perhaps we could keep a
> reference to the blob but clear the value here, and assign it again when
> that screenshot overlay is needed? 
> I've not succeeded in watching any effect this might have on memory usage (2
> b2g processes detected error when I run ./tools/get_about_memory.py) but
> intuitively this seems plausible.

So this sounds a dupe of another bug I saw, the screenshot layer is for the swipe navigation.
What we could try is: 
1. ONLY get the screenshot while app is closing. Don't use setVisible(false, true).
2. Render the screenshot overlay while swipe navigation starts or enter task manager.
Flags: needinfo?(alive)
Aus lemme know if you have time to take this one. I will dispatch to someone else if you are not available.
Flags: needinfo?(aus)
Yep, I can look into this.
Assignee: nobody → aus
Flags: needinfo?(aus)
Target Milestone: --- → 2.1 S2 (15aug)
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Blocker justification: big memory regression
blocking-b2g: 2.1? → 2.1+
Keywords: qawanted
Target Milestone: 2.1 S3 (29aug) → 2.1 S4 (12sep)
From what I can see in the debugger, while cards view is active we have the screenshot image on the blob on the .screenshot-overlay element in the AppWindow (e.g. "blob:app://system.gaiamobile.org/c6a4dadc-420b-48bc-977a-91b9394c6db4"), as well as the .screenshotView element on the <li> in #cards-list ("blob:app://system.gaiamobile.org/63169d31-8d1b-476a-aa8a-78bb8524a0d8"). Also, on the AppWindow instance there is a ._screenshotBlob reference with the Blob object. 

When cards-view is dismissed, the card's .screenshotView element is destroyed so that reference goes away. Also, the background-image is removed from the AppWindow's .screenshot-overlay element. The _screenshotBlob remains. 

So, while cards view is up we seem to have 2 separate image blob assigned, one of which isnt currently in use or visible. That is a bug we should fix but not the issue described here. When cards view is inactive, I dont see where this reported decoded in-memory screenshot is or is coming from? To answer the question in comment #4, no, the card elements from card view are destroyed when card view is hidden.
The screenshotBlob reference is used for edge gesture / sheets swiping btw, we do need to hold on to that while the app is inactive.
I don't think this necessarily involves the cards view.  Consider the following STR:

1. Reboot the phone.
2. Unlock the device and take a memory report (#1).
3. Launch an app.
4. Press the home button.
5. Take another memory report (#2)
6. Repeat steps 3-5 twice (producing memory reports #3 and #4).
7. Turn off the screen and take a memory report (#5).
8. Turn on the screen (now at the lock screen) and take a memory report (#6).

Reports 2, 3, and 4 will each grow another several MB blob image over the previous one.  The blob is uncompressed (see images/content/raster/used uncompressed-nonheap).  Report 5 will not have any of this uncompressed data, but 6 will have all of it.
(In reply to Candice Serran (:cserran) from comment #13)
> Blocker justification: big memory regression

What's the QA request for?
Flags: needinfo?(cserran)
(In reply to Jason Smith [:jsmith] from comment #17)
> (In reply to Candice Serran (:cserran) from comment #13)
> > Blocker justification: big memory regression
> 
> What's the QA request for?

In triage today team mentioned a patch that landed may have already fixed this. Need QA to retest. (sorry for the late explanation)
Flags: needinfo?(cserran)
I was able to reproduce this on a build from Monday.
Any news on this Aus?
Flags: needinfo?(aus)
I've started implementing the fix that Alive mentioned and should have something ready for Monday -- as long as what we're trying to do actually fixes the problem :)
Status: NEW → ASSIGNED
Flags: needinfo?(aus)
With the fix I'm almost done with this is what I'm seeing from the memory report after follow STRs in comment #16.

Initial Report: 2.70 MB (05.84%) -- images/content/raster/used
After Launching 3 apps: 3.66 MB (07.23%) -- images/content/raster/used
While the screen is off: 3.66 MB (07.47%) -- images/content/raster/used
When I turned the screen back on: 3.66 MB (07.26%) -- images/content/raster/used

Looks like it works!
Hi Kyle, I know you're probably pretty busy, but if you had a moment to also confirm that this patch works for you that'd be appreciated. :) No worries if you don't have time.
Flags: needinfo?(khuey)
I'm out of the office, so the earliest I could possibly test this is monday.
Comment on attachment 8486831 [details] [review]
Pull Request - Only render screenshot overlay when required by edge gestures, capture screenshot on app close only.

Left some comments also hope etienne to take a look.
Attachment #8486831 - Flags: review?(alive) → review?(etienne)
Comment on attachment 8486831 [details] [review]
Pull Request - Only render screenshot overlay when required by edge gestures, capture screenshot on app close only.

Sorry if I completely misunderstood the patch, but looks like we're *never* calling setVisible(false) on frames with this patch. Keeping all apps alive and fully running all the time :/

The screenshotOvelay is never displayed (since the frame is visible and on top), thus hiding the fact that we never get the sheetstransitionstart/end events like Alive pointed out.

The AppWindow#setVisible is responsible to take/hide the screenshots and actually setVisible(false/true) the frame at the correct moment. Your change completely bypasses this so we never get to actually calling setVisible(false) on the frame...
Attachment #8486831 - Flags: review?(etienne) → review-
(In reply to Etienne Segonzac (:etienne) from comment #27)
> Comment on attachment 8486831 [details] [review]
> Pull Request - Only render screenshot overlay when required by edge
> gestures, capture screenshot on app close only.
> 
> Sorry if I completely misunderstood the patch, but looks like we're *never*
> calling setVisible(false) on frames with this patch. Keeping all apps alive
> and fully running all the time :/
> 
> The screenshotOvelay is never displayed (since the frame is visible and on
> top), thus hiding the fact that we never get the sheetstransitionstart/end
> events like Alive pointed out.
> 
> The AppWindow#setVisible is responsible to take/hide the screenshots and
> actually setVisible(false/true) the frame at the correct moment. Your change
> completely bypasses this so we never get to actually calling
> setVisible(false) on the frame...

Gotcha, I think I know the changes I need to make to take care of this.

I'm going to have the AppWindowManager handle broadcasting 'sheetstransitionstart' and 'sheetstransitionend' to the AppWindow instances instead. This should ensure the events make it to the right handler. I'll also ensure the frame gets hidden in setVisible.
Comment on attachment 8486831 [details] [review]
Pull Request - Only render screenshot overlay when required by edge gestures, capture screenshot on app close only.

* Only take screenshot during closed transition handler.
* AppWindowManager now handles broadcasting 'sheetstransitionstart' and 'sheetstransitionend' to AppWindow instances.
* Active AppWindow instances do not need to do anything during sheetstransitionstart (updated handler to reflect that in AppWindow). They will have their visibility set on '_swipeout'.
* Only broadcast '_sheetstransitionend' to inactive applications. Newly active AppWindow instances will have their visibility set on '_swipein'.
* Addressed other comments by refactoring _showScreenshotOverlay and _hideScreenshotOverlay.
Attachment #8486831 - Flags: review?(etienne)
Attachment #8486831 - Flags: review?(alive)
Attachment #8486831 - Flags: review-
If PR looks good, the next step is to add unit-tests for 'sheetstransitionend' handling in AppWindowManager and AppWindow (the internal version of said event) as well as getScreenshot being called in the AppTransitionController _closed handler.
Comment on attachment 8486831 [details] [review]
Pull Request - Only render screenshot overlay when required by edge gestures, capture screenshot on app close only.

LGTM with nits
* Remove the screenshotstate if unnecessary.
* Correct the AppWindow.prototype.setVisible jsdoc comments and arguments
* Tests

Hope :etienne to double check it.
Attachment #8486831 - Flags: review?(alive) → feedback+
Comment on attachment 8486831 [details] [review]
Pull Request - Only render screenshot overlay when required by edge gestures, capture screenshot on app close only.

Comments on github, let me know if anything is unclear.
I think it's safe to start adding tests once those are addressed.
Attachment #8486831 - Flags: review?(etienne)
Phew... all review comments and concerns addressed. Starting on tests!
Comment on attachment 8486831 [details] [review]
Pull Request - Only render screenshot overlay when required by edge gestures, capture screenshot on app close only.

Now with added and updated tests!
Attachment #8486831 - Flags: review?(etienne)
Comment on attachment 8486831 [details] [review]
Pull Request - Only render screenshot overlay when required by edge gestures, capture screenshot on app close only.

Comments are on github, one of them is responding to a previous comment so make sure to start here [1] not to miss it.

This patch will need a last round of review because of the event changes, sorry about that, but it should be the last one!

[1] https://github.com/mozilla-b2g/gaia/pull/23874#discussion_r17537646
Attachment #8486831 - Flags: review?(etienne)
I don't think I'll have a chance to test this before you land it.
Flags: needinfo?(khuey)
Attachment #8486831 - Flags: review?(etienne)
Comment on attachment 8486831 [details] [review]
Pull Request - Only render screenshot overlay when required by edge gestures, capture screenshot on app close only.

r=me with a few last nits.

I'm not able to give this a last spin on device and check the memory report, but please do so before landing. Thanks!
Attachment #8486831 - Flags: review?(etienne) → review+
Target Milestone: 2.1 S4 (12sep) → 2.1 S5 (26sep)
Latest try run is almost done running and looking good. Should be able to land this very soon!

https://tbpl.mozilla.org/?rev=186d44117b54c425bd0bac6a879e41e4a87f6ca1&tree=Gaia-Try
Commit (master): https://github.com/mozilla-b2g/gaia/commit/1503e1f9f7846a3813c057ce95c35ac22031027f

Fixed on master. Will be leaving open for now as we will want to land this on 2.1 and we may need a slightly different version. News on this tomorrow.
removing qa-wanted tag - between comment 19 and this being on the verge of fixed with new patches I do not think it applies anymore. Please re-add if I am wrong.
Keywords: qawanted
[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Core Application Window Class
[User impact] if declined: Memory usage is 1.8M higher than it has to be (per application), this includes suspended and killed apps!!!
[Testing completed]: On Flame (319M)
[Risk to taking this patch] (and alternatives if risky): It's well tested on device and has unit tests to back it up. I would consider this low risk.
[String changes made]: None.
Attachment #8491056 - Flags: review+
Attachment #8491056 - Flags: approval-gaia-v2.1?(bbajaj)
Blocks: 1062136
I guess I have to mark this fixed so that it gets considered for landing on 2.1. So yeah, it's fixed on master. When uplifting, *PLEASE* use the v2.1 PR.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(In reply to Ghislain Aus Lacroix [:aus] from comment #43)
> Try run for 2.1 PR:
> https://tbpl.mozilla.org/
> ?rev=46010a6aa80983494a2fb754b72f6eddc78096a1&tree=Gaia-Try

Try run is green!
Keywords: verifyme
Hi Ghislain -

Although it is quite late in 2.0 cycle, still partner is wondering can we uplift the patch to 2.0 branch. Do you think it is feasible?

Thanks

Vance
Flags: needinfo?(aus)
This bug is not present in 2.0.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #46)
> This bug is not present in 2.0.

Hi Kyle -

Sorry about the confusion, I asked because in the following comment it mentions that the patch of this bug can also fix Bug#1062136

https://bugzilla.mozilla.org/show_bug.cgi?id=1062136#c22

Since our partner is concerned about Bug#1062136, I wonder if we can uplift the patch here to 2.0, or we need a new solution for Bug#1062136

Thanks
Flags: needinfo?(khuey)
Ah, so the patch fixes multiple issues then.  Carry on.

(I think we should do the uplift request in the other bug, fwiw)
Flags: needinfo?(khuey)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #48)
> Ah, so the patch fixes multiple issues then.  Carry on.
> 
> (I think we should do the uplift request in the other bug, fwiw)

Done so in 1062136, thanks for the reminding
Attachment #8491056 - Flags: approval-gaia-v2.1?(bbajaj) → approval-gaia-v2.1+
Blocks: 1071295
Can we get some QA attention on verifying this?  This doesn't appear to be fixed on my master build.
Flags: needinfo?(tchung)
QA-Wanted to check this in the latest 2.2 and 2.1
Flags: needinfo?(tchung)
Keywords: qawanted
Thanks Joshua
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #0)
> STR:
> 
> Boot up the phone and unlock it.
> Take a memory report with the -m flag.  Call this report #1.
> Fire up two apps, then press the home button long enough to bring up the
> cards view.
> Press home again to exit cards view.
> Turn off the screen.
> Take a memory report with the -m flag.  Call this report #2.
> Turn on the screen.  You should be at the lockscreen now.  Do not unlock the
> phone.
> Take a memory report with the -m flag.  Call this report #3.
> 
> Diff reports #2 and #3.
> 
> What happens:
> 
> I see two large images each consuming 7 MB of uncompressed-nonheap memory in
> the system app.  These are presumably the screenshots from the cards view,
> because they are not in report #1.  These should not be uncompressed in
> memory when the cards view is not in active use.

Kyle - maybe I'm not using the correct "memory report" but I'm not seeing anything "image related" in it. 

I used the command "adb shell top -m 25" and here are the results from step 2) Two apps open and in card view
User 1%, System 1%, IOW 0%, IRQ 0%
User 11 + Nice 0 + Sys 10 + Idle 590 + IOW 0 + IRQ 0 + SIRQ 0 = 611

  PID PR CPU% S  #THR     VSS     RSS PCY UID      Name
  212  0   1% S    58 232496K  97808K     root     /system/b2g/b2g
 1654  0   1% R     1   1308K    528K     root     top
   98  1   0% S     1      0K      0K     root     ksmd
    6  0   0% D     1      0K      0K     root     kworker/u:0
    7  0   0% D     1      0K      0K     root     kworker/u:0H
    8  0   0% S     1      0K      0K     root     migration/0
    9  1   0% S     1      0K      0K     root     migration/1
   12  1   0% S     1      0K      0K     root     ksoftirqd/1
   13  1   0% S     1      0K      0K     root     khelper
   14  1   0% S     1      0K      0K     root     netns
   15  0   0% S     1      0K      0K     root     kworker/0:1
   17  0   0% S     1      0K      0K     root     kworker/0:1H
   18  1   0% S     1      0K      0K     root     modem_notifier
   19  1   0% S     1      0K      0K     root     smd_channel_clo
   20  1   0% S     1      0K      0K     root     smsm_cb_wq
   22  1   0% S     1      0K      0K     root     rpm-smd
   23  1   0% S     1      0K      0K     root     kworker/u:1H
   24  1   0% S     1      0K      0K     root     mpm
   25  1   0% S     1      0K      0K     root     irq/47-cpr
   26  0   0% S     1      0K      0K     root     kworker/u:2
   61  1   0% S     1      0K      0K     root     sync_supers
   62  1   0% S     1      0K      0K     root     bdi-default
   63  1   0% S     1      0K      0K     root     kblockd
   64  1   0% S     1      0K      0K     root     system
   65  1   0% S     1      0K      0K     root     khubd



tested on the latest KK Master

Device: Flame Master
Build ID: 20140929070359
Gaia: 4d663b1f7d63e4d3d69c181a58f21b38145044b2
Gecko: 66ab05e2a667
Version: 35.0a1 (Master)
Firmware Version: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
Flags: needinfo?(khuey)
"memory report" here means the results of running ./tools/get_about_memory.py.  The report will have a section with images in them.  Search for "images/content/raster/used".
Flags: needinfo?(khuey)
ahhhh- I was able to run it (once) - and then I started running into bug 1060690 - 

Sorry Tony - I tried to intercept this task but I think maybe it is better off being ran by someone over there - 

I'm blocked by Bug 1060690 - which prevents me from running the required report


Attempted by:
1) Pushed v180 KK base image
2) Full-Flashed latest 2.2 Nightly build on top of it
Flags: needinfo?(tchung)
Keywords: qawanted
(In reply to Joshua Mitchell [:Joshua_M] from comment #56)
> I'm blocked by Bug 1060690 - which prevents me from running the required
> report

That bug has been closed (and the one it duplicates is closed as well), if you're having new issues can you file a new bug? My guess is you just need to update your |get_about_memory.py| script. Try running |git pull| from your B2G checkout.
Flags: needinfo?(jmitchell)
(In reply to Eric Rahm [:erahm] from comment #57)
> (In reply to Joshua Mitchell [:Joshua_M] from comment #56)
> > I'm blocked by Bug 1060690 - which prevents me from running the required
> > report
> 
> That bug has been closed (and the one it duplicates is closed as well), if
> you're having new issues can you file a new bug? My guess is you just need
> to update your |get_about_memory.py| script. Try running |git pull| from
> your B2G checkout.

Joshua, try again per eric's tip?
Flags: needinfo?(tchung)
okay - updated and re-ran - No large image files were seen in steps 2 and 3 and only a small amount of mem usage (.79MB) so this is verified fixed  (both 2.1 and 2.2)

Step 1: ─4.38 MB (22.10%) ++ images/content/raster/used
Step 2: ├───0.79 MB (05.68%) -- images/content/raster/used
│   ├──0.45 MB (03.23%) ++ (14 tiny)
│   └──0.34 MB (02.46%) -- image(<non-notable images>)
│      ├──0.33 MB (02.37%) ── raw
│      └──0.01 MB (00.08%) ++ (2 tiny)
Step 3: ├───0.79 MB (05.54%) -- images/content/raster/used
│   ├──0.45 MB (03.15%) ++ (14 tiny)
│   └──0.34 MB (02.39%) -- image(<non-notable images>)
│      ├──0.33 MB (02.31%) ── raw
│      └──0.01 MB (00.08%) ++ (2 tiny)


Environmental Variables:
Device: Flame 2.1
Build ID: 20141006062615
Gaia: 778ebac47554e1c4b7e9a952d73e850f58123914
Gecko: bc87917b3b95
Version: 34.0a2 
Firmware Version: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame Master
Build ID: 20141006092353
Gaia: 470826d13ae130a5c3d572d1029e595105485fb0
Gecko: 4534f97c4633
Version: 35.0a1 (Master)
Firmware Version: V180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0
Status: RESOLVED → VERIFIED
Flags: needinfo?(jmitchell)
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.