Closed
Bug 821630
Opened 12 years ago
Closed 11 years ago
[Settings]: can't retrieve blobs from settings database with get()
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
People
(Reporter: djf, Assigned: reuben)
References
Details
Attachments
(1 file, 5 obsolete files)
11.16 KB,
patch
|
gwagner
:
review+
|
Details | Diff | Splinter Review |
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•12 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: --- → ?
Comment 2•12 years ago
|
||
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•12 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•12 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.
Comment 5•12 years ago
|
||
The object wrapping there is quite fragile, yes. We should try to reuse ObjectWrapper.jsm instead.
Comment 6•12 years ago
|
||
(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.
Updated•12 years ago
|
Assignee: nobody → anygregor
Comment 7•12 years ago
|
||
We discussed this during triage and with the small savings of bug 820940, this isn't a blocker.
blocking-basecamp: ? → -
Assignee | ||
Updated•11 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Version: 18 Branch → Trunk
Assignee | ||
Comment 9•11 years ago
|
||
This tests get() and addObserver() when blobs are stored. Feel free to suggest more tests.
Attachment #723189 -
Flags: review?(anygregor)
Assignee | ||
Comment 10•11 years ago
|
||
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 11•11 years ago
|
||
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•11 years ago
|
||
(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•11 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•11 years ago
|
||
Attachment #723189 -
Attachment is obsolete: true
Attachment #723189 -
Flags: review?(anygregor)
Attachment #723546 -
Flags: review?(anygregor)
Comment 15•11 years ago
|
||
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•11 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.
Updated•11 years ago
|
Attachment #723190 -
Flags: review?(anygregor) → review+
Assignee | ||
Comment 17•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e56f3e0f00d9
Assignee | ||
Comment 18•11 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 | ||
Comment 19•11 years ago
|
||
Attachment #723190 -
Attachment is obsolete: true
Attachment #727006 -
Flags: review?(anygregor)
Comment 20•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
Attachment #723546 -
Attachment is obsolete: true
Comment 22•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
Attachment #727420 -
Attachment is obsolete: true
Attachment #727888 -
Flags: review?(anygregor)
Comment 25•11 years ago
|
||
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+
Assignee | ||
Comment 26•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5408bf5cf363
Status: NEW → ASSIGNED
Comment 27•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5408bf5cf363
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in
before you can comment on or make changes to this bug.
Description
•