956215 - Homescreen still uses data urls (pngs), wasting memory

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Homescreen, defect, P1)

Description

[Andreas Gal :gal]

Reported by njn. This is wasting valuable space on the 128MB device.

From the main process:

│  │   │  ├────583,404 B (01.41%) -- strings
│  │   │  │    ├──364,816 B (00.88%) -- notable
│  │   │  │    │  ├──184,368 B (00.45%) -- string(length=28946, copies=3, "data:image/png;base64,iVBORw0KGgoAAAANSUh...
│  │   │  │    │  │  ├──184,320 B (00.44%) ── malloc-heap
│  │   │  │    │  │  └───────48 B (00.00%) ── gc-heap
│  │   │  │    │  ├──110,640 B (00.27%) -- string(length=17210, copies=3, "data:image/png;base64,iVBORw0KGgoAAAANSUhEU...
│  │   │  │    │  │  ├──110,592 B (00.27%) ── malloc-heap
│  │   │  │    │  │  └───────48 B (00.00%) ── gc-heap
│  │   │  │    │  ├───61,488 B (00.15%) -- string(length=9114, copies=3, "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAA...
│  │   │  │    │  │   ├──61,440 B (00.15%) ── malloc-heap
│  │   │  │    │  │   └──────48 B (00.00%) ── gc-heap

From the homescreen process:

│   │  │  ├────198,632 B (01.75%) -- strings
│   │  │  │    ├──118,832 B (01.05%) -- notable
│   │  │  │    │  ├───61,456 B (00.54%) -- string(length=28946, copies=1, "data:image/png;base64,iVBORw0KGgoAA...
│   │  │  │    │  │   ├──61,440 B (00.54%) ── malloc-heap
│   │  │  │    │  │   └──────16 B (00.00%) ── gc-heap
│   │  │  │    │  ├───36,880 B (00.32%) -- string(length=17210, copies=1, "data:image/png;base64,iVBORw0KGgoAAAA...
│   │  │  │    │  │   ├──36,864 B (00.32%) ── malloc-heap
│   │  │  │    │  │   └──────16 B (00.00%) ── gc-heap
│   │  │  │    │  └───20,496 B (00.18%) -- string(length=9114, copies=1, "data:image/png;base64,iVBORw0KGgoAAAANSU...
│   │  │  │    │      ├──20,480 B (00.18%) ── malloc-heap
│   │  │  │    │      └──────16 B (00.00%) ── gc-heap

Comment 1

[Andreas Gal :gal]

I really meant tarako? here but we don't have a flag for that.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We really should do that CSP that blocks the data scheme idea that jlebar had.

Comment 3

[Andreas Gal :gal]

Thats a great idea. ++jlebar. Can we do this selectively for our own apps only?

Comment 4

[[:fabrice] Fabrice Desré]

I'm almost 100% sure that one of these is the marketplace icon.

I also filed bug 956192 to get rid of data urls in building blocks (they will show up in the email app for instance).

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Probably.  I think we need a CSP extension though, IIRC it doesn't support blacklisting a set of schemes, only whitelisting a set.

Comment 6

[[:fabrice] Fabrice Desré]

fwiw, certified apps have a hardcoded csp in c++. I can add a "no data: url" rule there easily.

Comment 7

[Andreas Gal :gal]

We shouldn't do it for all certified apps, just the ones Mozilla controls.

Comment 8

[Andreas Gal :gal]

Nick, any chance that you can dump the entire images and attach them here? That would help quickly identifying whats up.

Comment 9

[James Zhang (Spreadtrum)]

Jesse, can you provide a patch for reducing data urls memory and it's better for v1.3.

Comment 10

[Andreas Gal :gal]

James, if you guys have a patch please assign your engineer to this. I will grant you permissions to fully triage bugs (let me know if anyone else on your team should have the ability to change bug assignees). Thanks!

Comment 11

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3+ to benefit tarako.
to be conservative, estimated ~300 KB saving given we have 11 preloaded apps on home screen

Comment 12

[Andreas Gal :gal]

1.3+ seems wrong. Shouldn't we have a separate flag for tarako? We can try to uplift to 1.3 but I wouldn't delay 1.3 for this.

Comment 13

[Jason Smith [:jsmith]]

(In reply to Andreas Gal :gal from comment #12)
> 1.3+ seems wrong. Shouldn't we have a separate flag for tarako? We can try
> to uplift to 1.3 but I wouldn't delay 1.3 for this.

I'll bring up this concern at the drivers' meeting on Monday.

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: Don't truncate long strings in about:memory

> Nick, any chance that you can dump the entire images and attach them here?
> That would help quickly identifying whats up.

I don't have a development B2G device.  But the attached patch will allow somebody else to do it -- currently we truncate long strings shown in about:memory at 4096 chars.  This patch increases that, which means the entire data URI will be shown.  From that I assume it's possible to somehow get the original image back?

Comment 15

[Andrew McCreight [:mccr8]]

> From that I assume it's possible to somehow get the original image back?
IIRC, you can paste it into a browser and it will show you the image.

Also, given a JS heap dump you can figure out what is holding the big strings alive, though that may or may not be helpful.

Comment 16

[[:fabrice] Fabrice Desré]

(In reply to Joe Cheng [:jcheng] from comment #11)
> triage: 1.3+ to benefit tarako.
> to be conservative, estimated ~300 KB saving given we have 11 preloaded apps
> on home screen

Not all preloaded apps are using data urls, only the marketplace in the standard set.

Comment 17

[[:fabrice] Fabrice Desré]

On a fresh production gaia install, I only get one data: uri in the parent process and one in the homescreen. They are both the marketplace icon. What was the configuration used that led to the memory report from comment #0 ?

Comment 18

[Keven Kuo [:kkuo]]

It looks same case as 944224.

--
Keven

Comment 19

[James Zhang (Spreadtrum)]

Attachment: remove_data_urls.patch

Who can review jesse's patch?

Comment 20

[[:fabrice] Fabrice Desré]

Comment on attachment 8356037 [details] [diff] [review]
remove_data_urls.patch

Review of attachment 8356037 [details] [diff] [review]:
-----------------------------------------------------------------

We can't have this kind of dependencies in the homescreen. What needs to be done is simply fix the marketplace app so that it includes the icon in its package.

Comment 21

[[:fabrice] Fabrice Desré]

cvan, can you take that?

Comment 22

[Jesse]

Well, in my opinion, what we can do is not only simply fixing the marketplace app icon, but also providing a mechanism to ensure there is no data url any more or replacing a data-url with a local path in some special cases.

(In reply to Fabrice Desré [:fabrice] from comment #20)
> Comment on attachment 8356037 [details] [diff] [review]
> remove_data_urls.patch
> 
> Review of attachment 8356037 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We can't have this kind of dependencies in the homescreen. What needs to be
> done is simply fix the marketplace app so that it includes the icon in its
> package.

Comment 23

[Andreas Gal :gal]

Jesse, I agree with you but for that we need a patch that transparently converts the data uri into a blob and back as needed. Thats the ideal solution. We can probably get by with the cache purging since we do that whenever the homescreen goes into the background.

Comment 24

[Jason Smith [:jsmith]]

Is there anything left to do here? We've fixed the icons issue on the marketplace side in a different bug.

Comment 25

[Keven Kuo [:kkuo]]

Eve

Comment 26

[Keven Kuo [:kkuo]]

Evelyn will file other bug for each app that uses data url.

--
Keven

Comment 27

[Evelyn Hung]

(In reply to Keven Kuo [:kkuo] from comment #26)
> Evelyn will file other bug for each app that uses data url.
> 
> --
> Keven

For tarako, there is only browser app using data url, and I filed bug 958433 for it. However, after analysis and discussed with Ben on the bug, the benefit of memory saving  (40k) doesn't justify the risk of a making this change in 1.3.

Comment 28

[Jason Smith [:jsmith]]

This is a blocker for tarako, but not 1.3.

Comment 29

[Preeti Raghunath(:Preeti)]

Moving to 1.3T+ per bug comments

Comment 30

[Joe Cheng [:jcheng] (please needinfo)]

1.3T means tarako, remove [tarako] in whiteboard

Comment 31

[Joe Cheng [:jcheng] (please needinfo)]

hi Evelyn, is this something you can work on for Tarako? Thanks

Comment 32

[Evelyn Hung]

I thought the bug has been fixed after bug 944224 (see comment 18). Is there anything we want to do here?

Comment 33

[Joe Cheng [:jcheng] (please needinfo)]

let this goto tarako triage. 1.3T?

Comment 34

[bhavana bajaj [:bajaj]]

NO on :fabrice to check if this is still needed for 1.3T given the marketplace fix.

Comment 35

[[:fabrice] Fabrice Desré]

The marketplace fix is good, nothing to do here anymore.

Comment 36

[Nicholas Nethercote [inactive]]

I'm still seeing lots of images encoded as data: URIs on a trunk build on my Buri. The marketplace icon is the main offender. I identified the images using the techniques in comment 14 and comment 15 -- increase the notable string max length for about:memory and then paste the URI into the browser address bar, and it displays the image.

--------
Ones showing up as JS strings
--------

Main Process, within the system zone:
- 430,192 B -- length=28946, copies=7 -- marketplace icon, 120x120
- 258,160 B -- length=17210, copies=7 -- marketplace icon, 90x90
- 143,472 B -- length=9114,  copies=7 -- marketplace icon, 60x60

Main Process, within the app://system.gaiamobile.org/index.html zone:
- 61,456 B -- length=28946, copies=1 -- marketplace icon, 120x120
- 36,880 B -- length=17210, copies=1 -- marketplace icon, 90x90
- 20,496 B -- length=9114,  copies=1 -- marketplace icon, 60x60

Homescreen process, within the system zone:
- 184,368 B -- length=28946, copies=3 -- marketplace icon, 120x120
- 110,640 B -- length=17210, copies=3 -- marketplace icon, 90x90
-  61,488 B -- length=9114,  copies=3 -- marketplace icon, 60x60

Homescreen process, with the app://verticalhome.gaiamobile.org/index.html#root zone:
- 77,840 B -- length=38551, copies=1 -- games wallpaper (hooded thief), 320x480
- 61,456 B -- length=28946, copies=1 -- marketplace icon, 120x120
- 36,880 B -- length=17210, copies=1 -- marketplace icon, 90x90
- 20,496 B -- length=9114,  copies=1 -- marketplace icon, 60x60

--------
Ones showing up as images (less important)
--------
Smart Collections process:
- 647,248 B (614,400 B uncompressed, 32768 B raw) -- games wallpaper
- 36,944 B (20,480 B uncompressed, 16,384 B raw) -- "Games Games" dragon icon
- 36,944 B (20,480 B uncompressed, 16,384 B raw) -- "Blackjack" icon
  - plus 6 more just like that 
- 32,848 B (20,480 B uncompressed, 12,288 B raw) -- "Sudoku Sam..." icon
  - plus 10 more just like that 
- 28,752 B (20,480 B uncompressed, 8192 B raw) - "Chess" icon
  - plus 4 more just like that


Not much has changed since comment 0, except we have even more copies in the main process.

Comment 37

[Kevin Grandon :kgrandon]

(In reply to Nicholas Nethercote [:njn] from comment #36)
> --------
> Ones showing up as JS strings
> --------
> 
> Main Process, within the system zone:
> - 430,192 B -- length=28946, copies=7 -- marketplace icon, 120x120
> - 258,160 B -- length=17210, copies=7 -- marketplace icon, 90x90
> - 143,472 B -- length=9114,  copies=7 -- marketplace icon, 60x60

I suppose you're using an engineering build? It looks like these may be coming from a dev app, which we should probably remove/update: https://github.com/mozilla-b2g/gaia/blob/master/dev_apps/marketplace.allizom.org/manifest.webapp

(I'm guessing we should not see these in production)

Comment 38

[Nicholas Nethercote [inactive]]

> I suppose you're using an engineering build?

I built it myself. Can I specify flags that make it more like a production build?

Comment 39

[Wil Clouser [:clouserw]]

(In reply to Kevin Grandon :kgrandon from comment #37)
> (In reply to Nicholas Nethercote [:njn] from comment #36)
> I suppose you're using an engineering build? It looks like these may be
> coming from a dev app, which we should probably remove/update:
> https://github.com/mozilla-b2g/gaia/blob/master/dev_apps/marketplace.allizom.
> org/manifest.webapp
> 
> (I'm guessing we should not see these in production)

That is the hosted version of the staging Marketplace.  Technically it should be replaced with our current packaged version but we are testing a fully packaged version right now (just a couple bugs left) so it'll probably be easiest just to wait until that is finished and replace it then.  Bug 897156 is tracking.

Comment 40

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Nicholas Nethercote [:njn] from comment #38)
> I built it myself. Can I specify flags that make it more like a production
> build?

VARIANT=user should get you a build without all the dev and testing stuff.

Comment 41

[Emanuel Hoogeveen [:ehoogeveen]]

I should clarify (although I'm not 100% on it myself): I believe this environment variable is used during the configuration step, so |VARIANT=user ./config.sh| should work, but it can't hurt adding it everywhere.