806374 - Don't store phone's wallpaper or the dialer ringtone or the notification ringtone as a data: URI in the settings DB

Status: RESOLVED FIXED

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

Description

[Justin Lebar (not reading bugmail)]

Filed based on bug 802446 comment 16.

I don't know how much memory we're wasting here, but it seems like we may not want to store the phone's wallpaper as a data: URI in the settings DB because

* data: URIs are 2.7x larger than blobs
* all access to the settings DB is proxied through the main process (apparently?), so the main process uses memory (at least temporarily) to support this image.

Can we so something smarter here?

Comment 1

[[:fabrice] Fabrice Desré]

Since the wallpaper is managed by the system app, we can probably just move the image file into the system app resources and reference it.

Comment 2

[Justin Lebar (not reading bugmail)]

(In reply to Fabrice Desré [:fabrice] from comment #1)
> Since the wallpaper is managed by the system app, we can probably just move
> the image file into the system app resources and reference it.

The homescreen app is the one which actually displays the wallpaper, right?  Under that scheme, how would we transfer the wallpaper from the system app to the homescreen app?

Comment 3

[[:fabrice] Fabrice Desré]

No, the system app displays the wallpaper afaict. Just try by killing the homescreen ;)

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> No, the system app displays the wallpaper afaict. Just try by killing the
> homescreen ;)

You are right, but the image files must be shared between: system app/settings app/wallpaper app.
And what you said in comment 2 is what system app had done early before.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

Sorry is comment 1.
(In reply to Fabrice Desré [:fabrice] from comment #1)
> Since the wallpaper is managed by the system app, we can probably just move
> the image file into the system app resources and reference it.

If we do so, we couldn't set a photo captured by the camera but only the static images in the system app.

Comment 6

[Andrew McCreight [:mccr8]]

Data URIs are also being used to store ringtones in settings.

Beyond the single copy stored directly, you can end up with additional copies of these giant strings getting copied around.  For instance, in this B2G log I have, it looks like we have 4 copies of the wallpaper floating around, one inside the CSP cache...

Comment 7

[[:fabrice] Fabrice Desré]

Attachment: gecko patch

This patch converts all data: uris that are using when calling set() to blobs. This means that when doing a get() of the setting, we need to check if its a blob and get the blob uri.

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: gaia patch v1

The gaia side of the conversion. I still have to do the ringtones, but this patch should take care of all the wallpaper use.

Comment 9

[[:fabrice] Fabrice Desré]

Attachment: gaia patch v2

Hopefully with the notification and dialer ringtones also fixed.

Comment 10

[Justin Lebar (not reading bugmail)]

Surely you need to revoke this object URL at some point?

Comment 11

[[:fabrice] Fabrice Desré]

(In reply to Justin Lebar [:jlebar] from comment #10)
> Surely you need to revoke this object URL at some point?

Ideally yes (if not they are cleared when the document disappears), but I don't know how to do that for css background images.

Comment 12

[Justin Lebar (not reading bugmail)]

Otherwise every time you change the background, we'll leak the old blob permanently, in the main process, right?

Comment 13

[[:fabrice] Fabrice Desré]

Attachment: gaia patch v3

Now trying to revoke URL.

Comment 14

[[:fabrice] Fabrice Desré]

Attachment: gecko patch v2

Update of the previous patch to convert data: uris in all object properties, and to also do it in the SettingsService.

Comment 15

[[:fabrice] Fabrice Desré]

Attachment: gaia patch v4

This patch let us use either urls or blobs for the wallpaper and ringtones settings. I also changed the wallpaper saving in the settings app to not do a blob -> data uri anymore.

Tim & Vivien, feel free to redirect the review. I just don't know who to ask.

Comment 16

[Gregor Wagner [:gwagner]]

Comment on attachment 782372 [details] [diff] [review]
gecko patch v2

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

Looks good to me. The only thing missing is a testcase :)

::: dom/settings/SettingsDB.jsm
@@ +166,5 @@
> +        let start = aValue.indexOf(",") + 1;
> +        let isBase64 = ((aValue.indexOf("base64") + 7) == start);
> +        let payload = aValue.substring(start);
> +
> +        return new Blob([isBase64 ?base64DecToArr(payload) : payload],

nit: space after ?

@@ +175,5 @@
> +    }
> +    return aValue
> +  },
> +
> +  // Makes sure any propery that is a data: uri gets converted to a Blob.

nit: property

@@ +189,5 @@
> +      return aObject
> +    } else if (kind == "blob") {
> +      return aObject
> +    } else if (kind == "date") {
> +      return aObject

maybe combine file, blob, date and add ';' after aObject

@@ +194,5 @@
> +    } else if (kind == "primitive") {
> +      return this.convertDataURIToBlob(aObject);
> +    }
> +
> +    // Fall-through, we now have a dictionnary object.

nit: dictionary

Comment 17

[[:fabrice] Fabrice Desré]

Try build with nits addressed and a test added (based on the blobs tests, but storing a data: uri instead of a blob) :
https://tbpl.mozilla.org/?tree=Try&rev=77de3bb69ddd

Comment 18

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

Comment on attachment 782374 [details] [diff] [review]
gaia patch v4

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

To be honest I'm a bit concerned about memory leaks.

Also the code seems really similar in so many places. What about a little shared/js/ file to centralize that?

Comment 19

[[:fabrice] Fabrice Desré]

Attachment: gaia patch v5

Everything looks fine in my testing. We'll need to land that before the gecko patch as it supports both blobs and plain text uris.

Comment 20

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

Comment on attachment 783536 [details] [diff] [review]
gaia patch v5

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

Thanks.

Comment 21

[[:fabrice] Fabrice Desré]

https://github.com/mozilla-b2g/gaia/commit/996244ec35d2d151a1ae5ac4e17ea0ffcdde8aa0

Keeping the bug open until I push the gecko part.

Comment 22

[James Lal [:lightsofapollo]]

backed out of gaia for now:
https://github.com/mozilla-b2g/gaia/commit/8ae9100dd47e13a655423191cb3b3e5a02c5388a

I don't have any context here but this was breaking the travis build somehow.

Comment 23

[Rick Waldron [:rwaldron]]

https://github.com/mozilla-b2g/gaia/pull/11431/files#r5667602

Additionally, the SettingsURL constructor needs unit tests before it can be used in the Messages app.

Comment 24

[Kevin Grandon :kgrandon]

Looks like I am taking over and getting this landed. Rick - I'll try to address your concerns before we land. Thanks.

Comment 25

[Kevin Grandon :kgrandon]

Gaia part has landed in master: https://github.com/mozilla-b2g/gaia/commit/845abbb2ae4802c1fe50a50336dac922536a329d

Comment 26

[Kevin Grandon :kgrandon]

Attachment: gaia patch v6

The landed patch had a few minor differences than v5. Obsoleting the old patch in case someone needs this.

Comment 27

[Julien Wajsberg [:julienw]]

please see https://github.com/mozilla-b2g/gaia/commit/90245e0e244063545b83f6b1f21279a04bb077d4#commitcomment-3828736 as I added some recommendation to do the SettingsURL abstraction right. Thanks !

Comment 28

[Kevin Grandon :kgrandon]

Fabrice - Would you have any comment on Julien's comment above?

Comment 29

[[:fabrice] Fabrice Desré]

I commented on github.

Comment 30

[Julien Wajsberg [:julienw]]

Filed bug 903760 for further work.

Comment 31

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/c521aae575b5

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c521aae575b5