903760 - But 806374 follow-up: Fix the SettingsURL abstraction

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia, defect)

Description

[Julien Wajsberg [:julienw]]

Bug 806374 introduced a SettingsURL abstraction where the object URL creation and revocation are not correctly done.

Normally, you create an object URL only at the very moment you need it, and you revoke it once the resource is loaded. With the current SettingsURL abstraction, we create an objectURL as soon as we set the value, whereas we revoke it only when we set a new value, which happens rarely. 

I don't exactly know what really happens behind the scene, and what are the real consequences, so we should ask someone who knows better. My guess is that as soon as we create an object URL, we're telling the browser engine that we'll need this blob soon, and that it should be ready, but I may be plain wrong.

If we really want to keep the abstraction, we need to create the object url when we call "get" (and maybe rename it to "create"), and add a "revoke" method that would revoke the object url if it's a blob, and do nothing if it's a data url.

Fabrice said on github:

  I agree about the create/revoke lifetime issues, but I don't see any way to know eg. when a CSS background has been loaded and can be revoked.

AFAIK we can't, and that's why we need to use real img node when we want to use blobs as src. But someone can know a trick :)

Comment 1

[Julien Wajsberg [:julienw]]

Would Ben Turner knows something about the real consequences of this ?

(see https://github.com/mozilla-b2g/gaia/blob/master/shared/js/settings_url.js for the SettingsURL implementation)

Comment 2

[David Flanagan [:djf]]

My understanding of this (but I'd still really like to see Ben's answer) is that the issue here is pretty minor. If you don't call URL.releaseObjectURL(), the mapping between the UUID in the blob URL and the blob has to be maintained. Since the blob url is a string that can be passed around, gecko can't do reference counting on it itself, and we have to manage that with the release call.

But as far as I know, keeping the URL around isn't going to force an on-disk blob to be read into memory, for example.  In the case of ringtones from the settings db, the blob is already in memory, and has to stay there ready to play on incoming calls, right? So I don't see the harm in retaining a blob url forever.

For wallpaper, I suppose it would be nice if there was a way to allow the blob to be freed once the image was decoded.  And, that, I gather, is the tricky bit here.

Comment 3

[[:fabrice] Fabrice Desré]

Indeed the disk based Blobs are pretty cheap to keep around. So even with the current patch, we're in a better situation than keeping around huge data: uris.

Comment 4

[Julien Wajsberg [:julienw]]

I have absolutely no problem with keeping blobs around. My concern is with keeping blob url around. Especially, what exactly happens if we have an url pointing to a disk-based blob ?

If there is no difference between keeping a blob and keeping a blob url referencing it, then ok.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> If there is no difference between keeping a blob and keeping a blob url
> referencing it, then ok.

I don't think there's any problem here. Holding a blob and holding a blob url are equivalent in JS (assuming that the document sticks around).

Comment 6

[Julien Wajsberg [:julienw]]

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #5)

> I don't think there's any problem here. Holding a blob and holding a blob
> url are equivalent in JS (assuming that the document sticks around).

oh right, we need to revoke the blog url at least before the app is closed, right ?

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Julien Wajsberg [:julienw] from comment #6)
> oh right, we need to revoke the blog url at least before the app is closed,
> right ?

Well, that happens automatically when the document goes away in normal web pages... I don't know how you guys are using this though.

Comment 8

[Julien Wajsberg [:julienw]]

Should be the same then.

Comment 9

[BMO Automation]

Firefox OS is not being worked on