820940 - Gaia uses a data: URI for background of homescreen + lockscreen, wasting at least 100 of KB of main-process memory, and hiding another ~400KB of memory

Status: RESOLVED DUPLICATE of bug 806374

(Firefox OS Graveyard :: Gaia, defect, P3)

Description

[Justin Lebar (not reading bugmail)]

Gaia is apparently using data: URIs for the lockscreen and homescreen backgrounds.

These strings are ~89kb each.  They live in the main process and are never freed.  Moreover it appears that the lockscreen and homescreen backgrounds are identical strings.

Comment 1

[Justin Lebar (not reading bugmail)]

See bug 820134 -- there may be a larger-than-100kb gain lurking here.

Comment 2

[Justin Lebar (not reading bugmail)]

Guys, how hard would it be to use file refs here instead of data URIs?  We're wasting an embarrassing amount of memory.

Comment 3

[David Flanagan [:djf]]

Justin:

We can't use file refs. Wallpaper can come from the gallery or from built-in wallpapers (stored in the hidden wallpaper app).  But the system has to make a copy: we can't have wallpaper break if the original photo is deleted or sdcard is removed.

The data URI is an abomination, however, and we ought to be storing blobs in the settings database.

The data URI implementation is there I think because originally the settings database didn't support blobs (IIRC).  And also because we use a text file to initialize the default settings values.

I would love to fix this.  I think I may even have filed a bug about it a couple of months ago. But couldn't get blocking+ on it.

With this new information about the strings never being freed, maybe we can get blocking plus. If so, you can assign this to me, because I know just what to do to fix it.

Nominating for blocking

Comment 4

[Justin Lebar (not reading bugmail)]

I don't think this is a blocker, but I think it's an obvious soft-blocker, which means "we'll take a fix".

I don't understand this part, though:

> We can't use file refs. Wallpaper can come from the gallery or from built-in wallpapers 
> (stored in the hidden wallpaper app).  But the system has to make a copy: we can't have 
> wallpaper break if the original photo is deleted or sdcard is removed.

Does the root Gecko process not have anywhere on the non-removable flash storage that it could store a copy of the image being used as background?

Indeed, that can't be true, because the settings db must save itself somewhere non-removable.

I don't know if a file ref would be better or worse than a blob, but a file ref shouldn't be impossible.

Comment 5

[David Flanagan [:djf]]

(In reply to Justin Lebar [:jlebar] from comment #4)
> I don't think this is a blocker, but I think it's an obvious soft-blocker,
> which means "we'll take a fix".

Right, but we're also basically forbidden to work on non-blockers, so if we care about the memory implications, we should get this made blocking.

> 
> I don't understand this part, though:
> 
> > We can't use file refs. Wallpaper can come from the gallery or from built-in wallpapers 
> > (stored in the hidden wallpaper app).  But the system has to make a copy: we can't have 
> > wallpaper break if the original photo is deleted or sdcard is removed.
> 
> Does the root Gecko process not have anywhere on the non-removable flash
> storage that it could store a copy of the image being used as background?
> 
> Indeed, that can't be true, because the settings db must save itself
> somewhere non-removable.
> 
> I don't know if a file ref would be better or worse than a blob, but a file
> ref shouldn't be impossible.

Presumably such a storage space exists, but there are no content JS APIs  for saving files to it or reading files from it. The settings database is based on indexeddb, which handles blobs efficiently as files, I think, so I think a blob-based solution would be ideal.

Comment 6

[Philipp von Weitershausen [:philikon]]

Blob in settings DB (

Comment 7

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #6)
> Blob in settings DB (

Hmm, I didn't quite finish that sentence, did I... I wanted to say: Blob in settings DB would be the most optimal indeed. Any reason that wouldn't work out of the box?

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

We didn't use blob for settings database because the original |make settingsdb| script doesn't support that, and I don't think the current |build/settings.py| do that either.

djf, if you want to fix this. you would have to fix the build script too.

Comment 9

[Philipp von Weitershausen [:philikon]]

If y'all are busy with blockers, I'd be happy to take a stab at this. Please assign to me if necessary.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

If you have free cycles, please focus on startup wins.  We're much further away from targets on the startup side than 100KB will buy us on the memory side.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

blocking-basecamp? I'm unsure 100kb is enough to block basecamp.

Comment 12

[Justin Lebar (not reading bugmail)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #10)
> If you have free cycles, please focus on startup wins.  We're much further
> away from targets on the startup side than 100KB will buy us on the memory
> side.

FWIW I believe the combined unreported memory from all data: URIs is closer to 500kb (bug 820134).  I haven't been able to track them all down because of the noise from this one.

Note also that fixing this may be a pre-requisite for fixing bug 812948, which is a 2.5mb win, plus that bug may similarly be hiding other bugs with additional wins.

Comment 13

[David Scravaglieri [:scravag]]

We block based on Justin comment. Feel free to discuss about that and ask for minusing if needed.

Comment 14

[David Flanagan [:djf]]

If I understand the bug correctly, the issue is that when you use a data: URI, the long, long string is never freed.

We use data: URIs for convenience in the settings database, in large part because that's the only format we can store for the default value in build/settings.py.  So a full-blown fix for this would involve taking the default value out and including the default image in the wallpaper-setting apps that they would use if no value was returned from the settings DB.

It occurs to me, though, that a quicker and simpler fix would be to keep storing the image as a data uri, but when that image is used (in homescreen and lockscreen, I think) convert it to a blob uri before using it.  Then the string would presumably be freed.

A hybrid approach is to allow the wallpaper setting property to hold either a string or a blob. This would require the homescreen and lockscreen to test the value they get. If they get a string, they convert to a blob and overwrite the string with that.  And if we do that, then we should also modify the wallpaper setting code (in settings, wallpaper and homescreen) to save the blob instead of taking the extra steps to convert to a data uri first.

Comment 15

[Justin Lebar (not reading bugmail)]

> We use data: URIs for convenience in the settings database, in large part because that's 
> the only format we can store for the default value in build/settings.py.

How about the following approach to work around this?

* Store the default wallpaper filename in the settings database.  (Or don't; the default resources/images/backgrounds/default.png is already hardcoded in a few places.)

* On first run, we'll call Wallpaper.loadCurrentWallpaper().  We observe that there's no wallpaper in the settings db, so we open default.png as a blob and shove it in there.

Maybe you can't open default.png.  If so, here's another option:

Store the blob as a data URI in settings.json, but set a special flag which says "convert this setting to a blob."  Then on first run, the settings code opens settings.json, converts the setting to a blob, and we're done.

Comment 16

[David Flanagan [:djf]]

(In reply to Justin Lebar [:jlebar] from comment #15)
> > We use data: URIs for convenience in the settings database, in large part because that's 
> > the only format we can store for the default value in build/settings.py.
> 
> How about the following approach to work around this?
> 
> * Store the default wallpaper filename in the settings database.  (Or don't;
> the default resources/images/backgrounds/default.png is already hardcoded in
> a few places.)

Its a URL, not a filename, but yeah.  I think I'd just leave it hardcoded.

> 
> * On first run, we'll call Wallpaper.loadCurrentWallpaper().  We observe
> that there's no wallpaper in the settings db, so we open default.png as a
> blob and shove it in there.

Not necessary given the above, but a nice refinement.

> Maybe you can't open default.png.  If so, here's another option:
> 
> Store the blob as a data URI in settings.json, but set a special flag which
> says "convert this setting to a blob."  Then on first run, the settings code
> opens settings.json, converts the setting to a blob, and we're done.

I don't even think there is a need for a special setting.  Just doing a typeof on the settings value will tell us if it is a string that needs conversion.

If we're going to keep the default.png file in the system app anyway, then I'd say let's just do that and remove the giant data url from settings.py.

If philikon doesn't take this bug soon, I will.

Comment 17

[David Flanagan [:djf]]

The string 'wallpaper.image' appears in the following files, all of which will have to be modified for this bug:

apps/communications/dialer/js/oncall.js
apps/homescreen/js/wallpaper.js
apps/settings/js/wallpaper.js
apps/system/js/bootstrap.js
apps/system/js/lockscreen.js
apps/wallpaper/js/share.js
build/settings.py

Files that set wallpaper.image will have to be modified to save a blob instead of saving a data url.  This will actually be the easiest part of the patch.

Files that read the wallpaper setting will have to be modified so that they can take a blob, create a blob: url, and use that. And, they'll have to remember to use revokeURL on the old blob url, if there is one. And it may be hard to write a shared utility to help with this because some places we use img elements and other places we use background-image.

Furthermore, the system app (at least) will still have to be able to deal with strings instead of blobs. (Either a default data: URI from the settings database or a regular URL refering to a default image in the system app). In this case, they'll have to create an offscreen image, load the URL, copy to a canvas and convert to a blob, which they should then store back into the settings database. I think we could do this just in the system app, knowing that it will run before anything else queries the wallpaper, so it will always be able to convert a wallpaper string to a blob.

Two different parts of the system app read and listen for changes to wallpaper.image, so I think maybe I should create a new Wallpaper module with a addWallpaperListener() function that handles the setting and passes a blob url to registered listeners.

Finally, build/settings.py should probably change so that it just doesn't do anything for wallpaper.  Let the system app load a default wallpaper image from its own file. The build/wallpaper.jpg file is a low-quality image, and I think we want a high-quality png from the system app instead.  (It looks like the two images are not the same, so I should modify the system app to have a png version of the default image in build/wallpaper.jpg. The wallpaper app should have this, I think.)

Comment 18

[David Flanagan [:djf]]

I have a work in progress at: 
https://github.com/davidflanagan/gaia/compare/wallpaperblob

Unfortunately, I'm stuck because the settings database does not actually support blob values.

I can store blobs in the database, and when I do observers listening via addObserver() seem to get the new value just fine.

But when querying the setting value via get(), I get some object that is not a blob. When I try to pass this non-blob object to URL.createObjectURL() it throws a type error.  I suspect object wrapper issues. Filed 821630.

Comment 19

[David Flanagan [:djf]]

I just realized there's a problem with my current work-in-progress: it relies on a make-reset to change the initial wallpaper.image setting to null.  But we can't make everyone reset, so the code has to handle the case where there is a data uri in the settings db and should convert it to a blob.  That means changing the new system/js/wallpaper.js module in my current patch.

Comment 20

[Justin Lebar (not reading bugmail)]

It's not consistent to block on this and not block on bug 812948.  I'd like these bugs fixed, but I'd like more for us to be internally consistent here.

Comment 21

[David Flanagan [:djf]]

You took my +! 812948 says you can use your magical uplift powers, so maybe you can do that for my patch if 812630 gets fixed.

Comment 22

[Justin Lebar (not reading bugmail)]

I'm very happy to use my memory-fix a+ powers for this bug, yes.

Comment 23

[David Flanagan [:djf]]

But I'm not allowed to work on it anymore without the blocking+ :-(

Comment 24

[Andreas Gal :gal]

We don't have the bandwidth to fix all these bugs for January 15, but we definitely want to fix them. We have to focus on whats absolutely critical to make the product happen.

Comment 25

[Justin Lebar (not reading bugmail)]

I understand that to mean that you're happy with the average-case and worst-case memory usage on the device?

It's very difficult to proceed improving the memory usage in the average case and investigating the memory usage in the worst case when we don't fix bugs like this, because they obfuscate other bugs.

So if you're happy with how things are, that's fantastic; I'm really glad to hear that.

Comment 26

[Andreas Gal :gal]

Thats not what I said above. What I said above is that I would not stop the v1 product if this bug isn't fixed. That doesn't mean its any less important to fix it. I would simply rather fix it on January 16 than put the whole product at risk.

Comment 27

[Justin Lebar (not reading bugmail)]

I'd rather not block the product on this either.  I'm definitely not an apologist for the way we've set up our tracking flags, where a bug either "stops ship" or is understood to be "not something you may work on".

I'm just saying that if you're blocked on "memory usage" of any sort, it's difficult for us to make progress if we don't fix bugs like this.  I just want to make explicit the consequences here, is all.

Comment 28

[Andreas Gal :gal]

Thanks. I appreciate where you are coming from. I wanted to do the same: explain the consequences of not everyone focusing on the most critical show-stoppers at this point.

Comment 29

[David Flanagan [:djf]]

We've got a fix in hand for this one.  But its blocked on 821630, so that would be the bug to argue about anyway.

Comment 30

[Florian Bender]

Is this fixed by Bug 806374?

Comment 31

[David Flanagan [:djf]]

Yes I think it is.