1361330 - Implement a simple quota client (SimpleDB)

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, enhancement, P2)

Description

[Jan Varga [:janv]]

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.

Comment 1

[Jan Varga [:janv]]

Attachment: WIP

Comment 2

[Jan Varga [:janv]]

Attachment: patch

Comment 3

[Jan Varga [:janv]]

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

Comment 4

[Tom Tung [:tt, :ttung]]

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

Sure, thanks!

Comment 5

[Jan Varga [:janv]]

I will be submitting patches for this soon.

Comment 6

[Jan Varga [:janv]]

Attachment: Part 1: Fix default permissions for r/w streams

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

Comment 7

[Jan Varga [:janv]]

Attachment: Part 2: Allow creation and initialization of quota streams on separate threads

Comment 8

[Jan Varga [:janv]]

Attachment: Part 3: Move MemoryOutputStream to a generic place

Comment 9

[Jan Varga [:janv]]

Attachment: Part 4: Use fully qualified name when declaring ref counting for IDB quota client

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 11

[Jan Varga [:janv]]

(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.

Comment 12

[Jan Varga [:janv]]

Attachment: Part 2: Make quota stream constructors public and move the Create() helpers outside of the stream classes

Comment 13

[Jan Varga [:janv]]

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

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

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 15

[Nathan Froyd [:froydnj]]

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?

Comment 16

[Nathan Froyd [:froydnj]]

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.

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

(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.)

Comment 18

[Jan Varga [:janv]]

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.

Comment 19

[Jan Varga [:janv]]

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.

Comment 20

[Jan Varga [:janv]]

Attachment: Part 5: Core implementation

Ok, here it is.

Comment 21

[Jan Varga [:janv]]

Attachment: Part 6: Automatic tests for simpledb

Comment 22

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 23

[Jan Varga [:janv]]

Ok, sounds good to me.

Comment 24

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 25

[Andrew Sutherland [:asuth] (he/him)]

(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.

Comment 26

[Jan Varga [:janv]]

(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.

Comment 27

[Jan Varga [:janv]]

> 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 28

[Andrew Sutherland [:asuth] (he/him)]

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 29

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 30

[Jan Varga [:janv]]

I'm going to resurrect this.

Comment 31

[Jan Varga [:janv]]

Attachment: Part 6: Automatic tests for simpledb

Addressed review comments.

Comment 32

[Jan Varga [:janv]]

Attachment: Part 7: Gate SimpleDB behind a preference

Comment 33

[Jan Varga [:janv]]

(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.

Comment 34

[Jan Varga [:janv]]

(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.

Comment 35

[Jan Varga [:janv]]

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

Comment 36

[Jan Varga [:janv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=13b0c875aa76becf8225da10260741c3569fa416&selectedJob=194827804

Comment 37

[Pulsebot]

Pushed by [e-mail]:
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

Comment 38

[Stefan Hindli [:stefan_hindli]]

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