Writing quota manager tests using IndexedDB or DOM cache can be sometimes hard, due to limitations in the APIs or due to transparent compression, etc. It would be better to have a simple quota client for this purpose. The client can be also helpful as a template for creating new quota clients in future.

Tom, here's the patch, you can start rewriting some of those tests.

(In reply to Jan Varga [:janv] from comment #3) > Tom, here's the patch, you can start rewriting some of those tests. Sure, thanks!

I will be submitting patches for this soon.

This stream is rarely used in the codebase so we didn't notice.

Comment on attachment 8904340 [details] [diff] [review] Part 2: Allow creation and initialization of quota streams on separate threads Review of attachment 8904340 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure this is the patch you meant to upload, it just makes File{,Input,Output}Stream constructors public. And this seems odd given that there are public static Create factory methods. So I'm going to clear the review request for now. If this really is all this patch wants to be, I feel like the commit message should elaborate on what's going on, if not in the header file itself.

(In reply to Andrew Sutherland [:asuth] from comment #10) > Comment on attachment 8904340 [details] [diff] [review] > Part 2: Allow creation and initialization of quota streams on separate > threads > > Review of attachment 8904340 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm not sure this is the patch you meant to upload, it just makes > File{,Input,Output}Stream constructors public. And this seems odd given Yes, I need a public constructor for main simpledb patch. > that there are public static Create factory methods. So I'm going to clear > the review request for now. If this really is all this patch wants to be, I > feel like the commit message should elaborate on what's going on, if not in > the header file itself. You are right, it looks a bit odd, I'll rework it.

I want to take one more look at the main patch before submitting it.

Comment on attachment 8904341 [details] [diff] [review] Part 3: Move MemoryOutputStream to a generic place Review of attachment 8904341 [details] [diff] [review]: ----------------------------------------------------------------- r+'ing on the basis that this is a straight-up refactoring, but it would be great to have add a block comment either here or in a follow-up that explains how it fits in with all the other streams in existence. For example, the following comment is probably better than nothing (but I won't be nominating it for comment of the year): An output stream so you can read your potentially-async input stream into a contiguous buffer in the form of an nsCString using NS_AsyncCopy. Back when streams were more synchronous and people didn't know blocking I/O was bad, if you wanted to read a stream into a flat buffer, you could use NS_ReadInputStreamToString/NS_ReadInputStreamToBuffer. But those don't work with async streams. This can be used to replace hand-rolled Read/AsyncWait() loops. Because you specify the expected size up front, the nsCString buffer is pre-allocated so wasteful reallocations can be avoided. However, nsCString currently may over-allocate and this can be problematic on 32-bit windows until we're rid of that build configuration.

Comment on attachment 8904339 [details] [diff] [review] Part 1: Fix default permissions for r/w streams Review of attachment 8904339 [details] [diff] [review]: ----------------------------------------------------------------- Doh. I find it slightly amusing that you wrote in this behavior in bug 757507, though. :) I would feel slightly more comfortable about this if we chose a non-world-readable mode like 0640, possibly even a non-group readable mode like 0600. I think the only client of this code is NS_NewLocalFileStream, for which one caller passes 0640, and the other two don't seem to care. It's not clear to me whether the latter two (in dom/quota/ and dom/filehandle/) care deeply about world-readability, but I don't think it's a good idea to simply change the default so drastically. r=me conditionally; we should discuss what the Right Thing here is before this patch lands. Perhaps asuth has opinions, too?

Oh, and the comment in netwerk/base/public/nsIFileStreams.idl describing this function needs to be updated to describe the changed default! ni? just to make sure this comment doesn't get overlooked.

(In reply to Jan Varga [:janv] from comment #13) > I want to take one more look at the main patch before submitting it. Just in case you're waiting on my review of part 2 before posting it, I wanted to do the final sign-off after seeing the main patch because what make sense depends on how the changes are used. (In particular, I was thinking I might contribute comment suggestions or minor changes). I give provisional r=asuth for part 2 if that helps. (I comment now because the simple client might be nice for tests for the data clearing enhancement bugs.)

No, I just had to work on something else. I'll see how review for baku in bug 1333050 goes, I'll get back to this after that.

It seems that simpledb may also be used for stuff like service worker registration. Originally simpledb supported just one file per origin, I'm going to change it to support multiple files. It's an easy change.

Ok, here it is.

Apologies about the delay; I hope to finish the core of this review tomorrow, and was hoping there'd be good news about downgrade concerns so we wouldn't have to worry about that. I know SimpleDB is currently test-only and doesn't currently rev the schema version, but if we're planning to use it for bug 1183245, then the issue that QuotaManager::InitializeOrigin() in a prior release (57) will freak out and break if it sees the unknown client type is relevant. I think our simplest option might be to land this in 58, possibly with other downgrade-mitigation-enhancements, then land bug 1183245 and others in 59 (or later). That way 58 would be able to handle seeing the SDB client when confronted with a downgrade from 59 where SDB would actually be used by content for the first time.

Ok, sounds good to me.

bikeshedding: I think dom/quota/simpledb might be less confusing to people browsing dom/ than dom/simpledb/ who might otherwise think we're creating a new web storage API. I have a few high level questions: - Why does simpledb expose such low level file operations instead of just being something like put(name, string/blob/stream, useSnappy)? I would expect most consumers to want to read and write a file in its entirety, and these primitives make that hard to do. And any consumer attempting to use the lower-level abstractions seems likely to have problems with corruption/truncation under crashes/power failures/etc. in a way that they would not if they used SQLite or something built on SQLite. - Why do we have PBackgroundSDBConnection and the ability to directly access this storage from the content process at all? If it's just for testing, then we have SpecialPowers.loadPrivilegedScript to let (e10s) mochitests run script in the parent. Alternately, tests can just be written as "browser" (chrome) mochitests which already run in the parent but can also easily manipulate content, etc. If it's not just for testing, then I think there's a risk of coordination problems between multiple processes which the current API is not suited for. - What's the migration strategy for changes to this client? Will its versions be be tracked as part of the Quota Manager's schema version? - One strategy might be to do something like prepend a header to the contents of each file that contains [simple db version number, is this file snappy compressed, length of data on disk ignoring the header, CRC/integrity-hash over the contents of the file]. - Since it feeds in to the design of this, what's the broad strokes of your plan for moving ServiceWorkerRegistrar over to this implementation? Also... is LocalStorage going to use this? I ask because I'm interested for BackgroundSync whose current patches use both a global SQLite database plus a per-origin database. - Ignoring the potential of a generic ability for APIs like backgroundsync and webpayments to annotate their storage onto specific registrations (which could be better), these needs could both easily be met by the SimpleDB if: - BackgroundSync could efficiently say "tell me all the origins that have a 'backgroundsync'" file on disk. These are the origins that have any registrations. - BackgroundSync could then load the contents of those files atomically. - I would definitely be interested in talking about generic store-on-registration persistence. I know that's a lot of high-level questions. It might be better to do this in a vidyo call; let me know if you'd prefer to do that.

(In reply to Andrew Sutherland [:asuth] from comment #24) > - Why do we have PBackgroundSDBConnection and the ability to directly access > this storage from the content process at all? I phrased this poorly. A PBackground-managed IPDL protocol is obviously a nice way to get from a parent-process main-thread to the parent-process PBackground where QuotaManager lives. While we would expect potential consumers of SimpleDB to already be PBackground themselves, such as BackgroundSync, test JS code is obviously not going to have that advantage. It's also the case that it's very hard to get a reference to the PBackground ("IPDL Background") thread to bounce runnables across, and arguably using IPDL is a cleaner way to get across. So I think my real question is: - Can we add guards that the protocol is only used same-process? (And maybe we could discuss on dev-platform about adding explicit IPDL annotations for protocols that are really intended for chromeonly communication to help convey this and make it easier for sandboxing in the future, plus maybe reduce code/binary size?) - As a very brief thought exercise, if we are going to limit things to same process, would just directly using MozPromise-style runnables maybe cut down on boilerplate? The existing IPDL is very boiler-platey.

(In reply to Andrew Sutherland [:asuth] from comment #25) > (In reply to Andrew Sutherland [:asuth] from comment #24) > > - Why do we have PBackgroundSDBConnection and the ability to directly access > > this storage from the content process at all? > > I phrased this poorly. A PBackground-managed IPDL protocol is obviously a > nice way to get from a parent-process main-thread to the parent-process > PBackground where QuotaManager lives. While we would expect potential > consumers of SimpleDB to already be PBackground themselves, such as > BackgroundSync, test JS code is obviously not going to have that advantage. > It's also the case that it's very hard to get a reference to the PBackground > ("IPDL Background") thread to bounce runnables across, and arguably using > IPDL is a cleaner way to get across. > > So I think my real question is: > - Can we add guards that the protocol is only used same-process? (And maybe > we could discuss on dev-platform about adding explicit IPDL annotations for > protocols that are really intended for chromeonly communication to help > convey this and make it easier for sandboxing in the future, plus maybe > reduce code/binary size?) Originally, I didn't want to use IPC at all, but what if we decide to use that from a content process in future ? We would have to rewrite it heavily. I think I'm ok with limiting it to same process. > - As a very brief thought exercise, if we are going to limit things to same > process, would just directly using MozPromise-style runnables maybe cut down > on boilerplate? The existing IPDL is very boiler-platey. Do you mean to replace the state machine with MozPromise chaining ? Hm, we can consider that.

> I know that's a lot of high-level questions. It might be better to do this > in a vidyo call; let me know if you'd prefer to do that. Yeah, a vidyo call would be better.

Comment on attachment 8912709 [details] [diff] [review] Part 6: Automatic tests for simpledb Review of attachment 8912709 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/test/head.js @@ +135,5 @@ > .getService(Components.interfaces.nsIPermissionManager) > .testPermissionFromPrincipal(principal, permission); > } > + > +function getConnection() nit: consider renaming this getSDBConnection() to improve test clarity a little and make naive dxr/searchfoxing/grepping more useful ::: dom/quota/test/helpers.js @@ +311,5 @@ > }; > > self.postMessage({ op: "ready" }); > } > + I'd suggest adding a block comment like the following to help make the SpecialPowers wrapping/unwrapping more understandable, if only for future me, because I find this stuff confusing: """ SDBConnections and SpecialPowers wrapping: SpecialPowers provides a SpecialPowersHandler Proxy mechanism that lets our content-privileged code borrow its chrome-privileged principal to access things we shouldn't be able to access. The proxies wrap their returned values, so once we have something wrapped we can rely on returned objects being wrapped as well. The proxy will also automatically unwrap wrapped arguments we pass in. However, we need to invoke wrapCallback on callback functions so that the arguments they receive will be wrapped because the proxy does not automatically wrap content-privileged functions. Our use of (wrapped) SpecialPowers.Cc results in getSDBConnection() producing a wrapped nsISDBConnection instance. The nsISDBResults instances exposed on the (wrapped) nsISDBRequest are also wrapped, so our requestFinished helper wraps the results in SDBResultWrapper instances that behave the same as the result, automatically unwrapping the wrapped array/arraybuffer results. """ @@ +312,5 @@ > > self.postMessage({ op: "ready" }); > } > + > +function getConnection() (renaming suggested here too) @@ +342,5 @@ > + return SpecialPowers.unwrap(this._result.getAsArrayBuffer()); > + } > +} > + > +function* requestFinished(request) { (I look forward to when we refactor everything to async functions so callers don't have to use yield*.)

Comment on attachment 8912691 [details] [diff] [review] Part 5: Core implementation Review of attachment 8912691 [details] [diff] [review]: ----------------------------------------------------------------- In our vidyo call, we reached consensus that: - SimpleDB and its low level ops are great for testing, but is not appropriate for production use. So if/when we move ServiceWorkerRegistrar to a reusable quota client, SimpleDB won't be that client. (Although its code could form the basis for such a thing.) This is sad but means SimpleDB can largely land as-is. - The SimpleDB will entirely be gated behind a preference. Ideally, it would be gated behind an ifdef/conditional-build thing, but since the advent of packaged tests, I don't think this is feasible. That is, we create a number of "target.*.tests.zip" files that contain tests as part of our builds so that the tests can be run against any build. This means that a preference needs to be our guard. Given that this has become a testing-only client and my backlog's a little deep right now, my review is a bit more cursory than I'd like, but I think is sufficient. The most important check I made was that nsIFile::Append is used so that should the pref get flipped by nefarious code, it is limited to manipulating quota-controlled files beneath PROFILE/storage/default/origin/sdb/. I'd like to also review the preference guard logic. In particular, that mozilla::dom::AllocPBackgroundSDBConnectionParent refuses to allocate if the preference isn't enabled.

I'm going to resurrect this.

Addressed review comments.

(In reply to Andrew Sutherland [:asuth] from comment #29) > I'd like to also review the preference guard logic. In particular, that > mozilla::dom::AllocPBackgroundSDBConnectionParent refuses to allocate if the > preference isn't enabled. I submitted patch 7 for this. There is one pref check on the child side to prevent creation of SDBConnection objects. Another one is on the parent side when connection tries to open database.

(In reply to Nathan Froyd [:froydnj] from comment #15) > I would feel slightly more comfortable about this if we chose a > non-world-readable mode like 0640, possibly even a non-group readable mode > like 0600. I think the only client of this code is NS_NewLocalFileStream, > for which one caller passes 0640, and the other two don't seem to care. > It's not clear to me whether the latter two (in dom/quota/ and > dom/filehandle/) care deeply about world-readability, but I don't think it's > a good idea to simply change the default so drastically. > > r=me conditionally; we should discuss what the Right Thing here is before > this patch lands. Perhaps asuth has opinions, too? The other two (dom/quota/ and dom/filehandle/) create the file (using 0644 mode) prior to creating the stream. So the stream never needs to use the default access mode value. I think it would be better if simpledb just passed in its own access mode and didn't use the default value at all.

Ok, I'll do one more try push and then we can land this.

Pushed by jvarga@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/03dfadb39d50 Part 1: Make quota stream constructors public and move the Create() helpers outside of the stream classes; r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef89713c643 Part 2: Move MemoryOutputStream to a generic place; r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/bf1a68dee63b Part 3: Use fully qualified name when declaring ref counting for IDB quota client; r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/b23989158daf Part 4: Core implementation; r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/481c13a4221b Part 5: Automatic tests for simpledb; r=asuth https://hg.mozilla.org/integration/mozilla-inbound/rev/dee7aef26593 Part 6: Gate SimpleDB behind a preference; r=asuth

https://hg.mozilla.org/mozilla-central/rev/03dfadb39d50 https://hg.mozilla.org/mozilla-central/rev/3ef89713c643 https://hg.mozilla.org/mozilla-central/rev/bf1a68dee63b https://hg.mozilla.org/mozilla-central/rev/b23989158daf https://hg.mozilla.org/mozilla-central/rev/481c13a4221b https://hg.mozilla.org/mozilla-central/rev/dee7aef26593