821630 - [Settings]: can't retrieve blobs from settings database with get()

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[David Flanagan [:djf]]

I'm trying to change the way Gaia stores wallpaper images, to use blobs instead of data uris.

I've found that I can save blobs to the settings database. If I do this, listeners registered with addObserver() get the new blob value just fine. But if I try to query the value with get(), I successfully retrieve an object, but the object is not a valid blob.

I'd guess that something is going wrong wrapping the blob.

I don't have a formal test case, but you can see the code I'm working on here: https://github.com/davidflanagan/gaia/compare/wallpaperblob

Comment 1

[David Flanagan [:djf]]

Cc'ing Gregor because he works on settings.  Cc'ing Justin because he really wants the wallpaper patches to work.  And nominating blocking because this blocks #820940 which is blocking.

Comment 2

[Justin Lebar (not reading bugmail)]

FWIW the word "blob" doesn't appear in dom/settings/SettingsManager.js.  I think we may have been mistaken in the belief that blobs are supported.

Comment 3

[David Flanagan [:djf]]

Justin, 

I've been told that because the Settings API is based on IndexedDB, blobs should "just work".  Given that they work partially, I'm guessing there is an easy fix for this.

Comment 4

[David Flanagan [:djf]]

Like maybe SettingsManager.js:112 should include an "&& value is not a blob" clause so we don't try to stick an __exposedProps__ object on the blob.

I don't know what parts of the settings stuff is running in which process, so I don't know what needs to be exposed or wrapped to make this work right, but I'm hoping it is easy.

Comment 5

[[:fabrice] Fabrice Desré]

The object wrapping there is quite fragile, yes. We should try to reuse ObjectWrapper.jsm instead.

Comment 6

[Gregor Wagner [:gwagner]]

(In reply to David Flanagan [:djf] from comment #3)
> Justin, 
> 
> I've been told that because the Settings API is based on IndexedDB, blobs
> should "just work".  Given that they work partially, I'm guessing there is
> an easy fix for this.

Yeah they should work and we have to fix this.

Comment 7

[Andrew Overholt [:overholt]]

We discussed this during triage and with the small savings of bug 820940, this isn't a blocker.

Comment 8

[Reuben Morais [:reuben]]

Yoink.

Comment 9

[Reuben Morais [:reuben]]

Attachment: 1 - Tests

This tests get() and addObserver() when blobs are stored. Feel free to suggest more tests.

Comment 10

[Reuben Morais [:reuben]]

Attachment: 2 - Don't serialize settings

So the problem here is that SettingsManager is doing things like |setting = JSON.parse(JSON.stringify(setting))|, which breaks blobs.

There is a comment mentioning cloning issues, but dom/settings/tests/ is green with this patch, so I'm assuming that's no longer necessary. If it is, why, and why do we not have a test for that?

Comment 11

[Gregor Wagner [:gwagner]]

Comment on attachment 723189 [details] [diff] [review]
1 - Tests

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

::: dom/settings/tests/test_settings_blobs.html
@@ +89,5 @@
> +  },
> +  function() {
> +    let lock = mozSettings.createLock();
> +    req = lock.set({"test1": storedBlob});
> +    req.onsuccess = next;

You are calling next twice here. Once in req.onsuccess and once in the observer.

Comment 12

[Gregor Wagner [:gwagner]]

(In reply to Reuben Morais [:reuben] from comment #10)
> Created attachment 723190 [details] [diff] [review]
> 2 - Don't serialize settings
> 
> So the problem here is that SettingsManager is doing things like |setting =
> JSON.parse(JSON.stringify(setting))|, which breaks blobs.
> 
> There is a comment mentioning cloning issues, but dom/settings/tests/ is
> green with this patch, so I'm assuming that's no longer necessary. If it is,
> why, and why do we not have a test for that?

This code was needed to avoid security exceptions when dealing with objects. I think we have tests that deal with objects but please also test this on the real device and put/get an object in a child process.

Comment 13

[Reuben Morais [:reuben]]

(In reply to Gregor Wagner [:gwagner] from comment #12) 
> This code was needed to avoid security exceptions when dealing with objects.
> I think we have tests that deal with objects but please also test this on
> the real device and put/get an object in a child process.

The settings app works normally with this patch applied. Anyway, I'll change the patch so it only does that if it's an object (but not a File/Blob/Array/etc).

Comment 14

[Reuben Morais [:reuben]]

Attachment: 1 - Tests

Comment 15

[Gregor Wagner [:gwagner]]

Comment on attachment 723546 [details] [diff] [review]
1 - Tests

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

Thanks!

::: dom/settings/tests/test_settings_blobs.html
@@ +88,5 @@
> +    next();
> +  },
> +  function() {
> +    let lock = mozSettings.createLock();
> +    req = lock.set({"test1": storedBlob});

Add a comment that next() is called by the observer.

@@ +102,5 @@
> +    req.onerror = onFailure("Getting blob");
> +  },
> +  function() {
> +    let lock = mozSettings.createLock();
> +    req = lock.clear();

Lets remove the observer function as well.

Comment 16

[Reuben Morais [:reuben]]

djf, I reapplied your changes on top of master and made ringtones be stored as blobs instead of data URIs in a branch to test this change: https://github.com/reubenmorais/gaia/compare/settings-blobs

Everything seems to be working fine. I'll create a PR after I land this.

Comment 17

[Reuben Morais [:reuben]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e56f3e0f00d9

Comment 18

[Reuben Morais [:reuben]]

Backed out for breaking mochitests on B2G. Too late to think, will debug later: https://hg.mozilla.org/integration/mozilla-inbound/rev/e40a3de37603

Comment 19

[Reuben Morais [:reuben]]

Attachment: 2 - Only serialize actual objects (store Blobs, Dates and Files as is)

Comment 20

[Gregor Wagner [:gwagner]]

Comment on attachment 727006 [details] [diff] [review]
2 - Only serialize actual objects (store Blobs, Dates and Files as is)

>     if (!this._open) {
>       dump("Settings lock not open!\n");
>       throw Components.results.NS_ERROR_ABORT;
>     }
>
>     if (this._settingsManager.hasWritePrivileges) {
>       let req = Services.DOMRequest.createRequest(this._settingsManager._window);
>       if (DEBUG) debug("send: " + JSON.stringify(aSettings));
>-      let settings = JSON.parse(JSON.stringify(aSettings));
>-      this._requests.enqueue({request: req, intent: "set", settings: settings});
>+      this._requests.enqueue({request: req, intent: "set", settings: aSettings});

I don't think we can do this. We have to clone the object because if the content side changes
the object we will store the changed object.


>       this.createTransactionAndProcess();
>       return req;
>     } else {
>       if (DEBUG) debug("set not allowed");
>       throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
>     }
>   },
>

Comment 21

[Reuben Morais [:reuben]]

Attachment: 2 - Only serialize actual objects (store Blobs, Dates and Files as is)

This uses stringify's replacer and parse's reviver parameters to preserve binaries.

Comment 22

[Gregor Wagner [:gwagner]]

Comment on attachment 727420 [details] [diff] [review]
2 - Only serialize actual objects (store Blobs, Dates and Files as is)

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

::: dom/settings/SettingsManager.js
@@ +204,5 @@
> +                                                      .generateUUID().toString();
> +        binaries[uuid] = value;
> +        return uuid;
> +      }
> +      if (kind == "array") {

you are handling array twice here.

Comment 23

[Gregor Wagner [:gwagner]]

Comment on attachment 727420 [details] [diff] [review]
2 - Only serialize actual objects (store Blobs, Dates and Files as is)

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

Let's do one final round.

::: dom/settings/SettingsManager.js
@@ +195,5 @@
> +    // the set() call and the enqueued request being processed. We can't simply
> +    // parse(stringify(obj)) because that breaks things like Blobs, Files and
> +    // Dates, so we use stringify's replacer and parse's reviver parameters to
> +    // preserve binaries.
> +    let binaries = {};

Lets do Object.create(null);

@@ +198,5 @@
> +    // preserve binaries.
> +    let binaries = {};
> +    let stringified = JSON.stringify(aObject, function(key, value) {
> +      let kind = ObjectWrapper.getObjectKind(value);
> +      if (kind == "file" || kind == "blob" || kind == "date" || kind == "array") {

remove array here.

@@ +217,5 @@
> +    return JSON.parse(stringified, function(key, value) {
> +      if (value in binaries) {
> +        return binaries[value];
> +      }
> +      if (Array.isArray(value)) {

Shouldn't be needed.

Comment 24

[Reuben Morais [:reuben]]

Attachment: 2 - Only serialize actual objects (store Blobs, Dates and Files as is)

Comment 25

[Gregor Wagner [:gwagner]]

Comment on attachment 727888 [details] [diff] [review]
2 - Only serialize actual objects (store Blobs, Dates and Files as is)

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

Thanks!

Comment 26

[Reuben Morais [:reuben]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5408bf5cf363

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5408bf5cf363