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

RESOLVED FIXED in mozilla22

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: djf, Assigned: reuben)

Tracking

Trunk
mozilla22
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:-)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
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.
blocking-basecamp: --- → ?
(Reporter)

Updated

6 years ago
Blocks: 820940
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.
(Reporter)

Comment 3

6 years ago
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.
(Reporter)

Comment 4

6 years ago
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.
The object wrapping there is quite fragile, yes. We should try to reuse ObjectWrapper.jsm instead.
(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.
Assignee: nobody → anygregor
We discussed this during triage and with the small savings of bug 820940, this isn't a blocker.
blocking-basecamp: ? → -
(Assignee)

Comment 8

5 years ago
Yoink.
Assignee: anygregor → reuben.bmo
(Assignee)

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
Version: 18 Branch → Trunk
(Assignee)

Comment 9

5 years ago
Created attachment 723189 [details] [diff] [review]
1 - Tests

This tests get() and addObserver() when blobs are stored. Feel free to suggest more tests.
Attachment #723189 - Flags: review?(anygregor)
(Assignee)

Comment 10

5 years ago
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?
Attachment #723190 - Flags: review?(anygregor)
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.
(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.
(Assignee)

Comment 13

5 years ago
(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).
(Assignee)

Comment 14

5 years ago
Created attachment 723546 [details] [diff] [review]
1 - Tests
Attachment #723189 - Attachment is obsolete: true
Attachment #723189 - Flags: review?(anygregor)
Attachment #723546 - Flags: review?(anygregor)
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.
Attachment #723546 - Flags: review?(anygregor) → review+
(Assignee)

Comment 16

5 years ago
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.
Attachment #723190 - Flags: review?(anygregor) → review+
(Assignee)

Comment 18

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

Updated

5 years ago
Depends on: 852726
(Assignee)

Comment 19

5 years ago
Created attachment 727006 [details] [diff] [review]
2 - Only serialize actual objects (store Blobs, Dates and Files as is)
Attachment #723190 - Attachment is obsolete: true
Attachment #727006 - Flags: review?(anygregor)
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;
>     }
>   },
>
Attachment #727006 - Flags: review?(anygregor)
(Assignee)

Comment 21

5 years ago
Created attachment 727420 [details] [diff] [review]
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.
Attachment #727006 - Attachment is obsolete: true
Attachment #727420 - Flags: review?(anygregor)
(Assignee)

Updated

5 years ago
Attachment #723546 - Attachment is obsolete: true
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 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.
Attachment #727420 - Flags: review?(anygregor)
(Assignee)

Comment 24

5 years ago
Created attachment 727888 [details] [diff] [review]
2 - Only serialize actual objects (store Blobs, Dates and Files as is)
Attachment #727420 - Attachment is obsolete: true
Attachment #727888 - Flags: review?(anygregor)
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!
Attachment #727888 - Flags: review?(anygregor) → review+
https://hg.mozilla.org/mozilla-central/rev/5408bf5cf363
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Blocks: 854514
(Reporter)

Updated

5 years ago
Blocks: 857472
You need to log in before you can comment on or make changes to this bug.