1389660 - Create an attachDatabase API, use it when cloning, and propagate all schema-relative pragmas for attached databases on clone

Status: NEW

(Toolkit :: Storage, enhancement, P3)

Description

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

Thanks to excellent analysis by :adw in bug 1388584, we believe Places is breaking because its ATTACH DATABASE command is not binding the database path, but instead doing its own string concatenation without escaping the database path.  This will definitely break on directories like "Alice's profile-breaking directory" that include a single "'" inside.

While the root cause of that bug is in Places[1], it's also the case that Connection::initializeClone does the exact same thing.  Since Places likes to clone its connections, this is likely to be a breaking problem in mozStorage as well.  This seems like an excellent justification for adding an "attachDatabase" API call to our connections and having Places use it.

Given that Place's existing attempt at a generic AttachDatabase also (non-generically) sets the journal_size_limit, I'll throw that on as the third argument.  We may need to save that off depending on whether initializeClone can query that for the attached database.  I'll look into it and other similarly attached-database-varying pragmas.

1: https://dxr.mozilla.org/mozilla-release/rev/cace0357d40e875ea45b9ccad99f8785fc2cdb50/toolkit/components/places/Database.cpp#383

Comment 1

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

Attachment: pulled a string yak shaving, fanciness to be split up

Just stashing my WIP.  This would need to be broken out into a number of
patches.  The thought train that went into this was:
- We want to propagate schema-relative pragmas per-schema and separate from
  the global schemas.
- That results in a lot of copied and pasted boilerplate.
- Hey, manually building strings for things that could be bound is what got
  us into this mess in the first place.
- Pragmas can actually be numeric/string/bool.
- mozIStorageRow (async API return rows) exposes variant-returning functions,
  this method could be moved up to mozIStorageValueArray enabling us to add
  getPragma and setPragma convenience methods that can be largely
  type-agnostic.
- We can similarly use the variant type to easily enable a simple execution
  method that binds a single argument by index.  This would be a gateway to
  doing fancier template magic to reduce boilerplate.
- Unfortunately, having to new the specific variant type and store it in an
  nsCOMPtr<nsIVariant> adds back some of the boilerplate.  The main win is
  we can skip an rv check.

If continuing in this direction, I think it's worth trying some deeper
template magic that doesn't involve Variants in the cases where the type
information is known to us.  The getPragma/setPragma propagation might as
well stick with using Variants, but C++ code that knows its types need not.

Comment 2

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

Since bug 1388584 has been addressed on all branches by binding the path in the statement, the urgency of addressing this is reduced, un-assigning.

In terms of the yak shaving, I'll review my WIP in the future to see if any of it looks reasonable, but it seems like existing boilerplate may be the way to go.  Ideally we won't be doing so much new C++ development going forward, but instead will be using rust and can use an existing crate for dealing with SQLite (or use mentat ;), so a deeper investment in templates here doesn't seem justified.

Comment 3

[Marco Bonardo [:mak]]

Thank you for working with Drew on the blocking issue with ATTACH. I indeed avoided adding a general API for attaching, because it was a lot of work for a small benefit. I was partially wrong, considered I overlooked bug 1388584.
It would still need 2 implementations (sync and async) and lots of PRAGMA copies, when maybe the consumer doesn't really care (Places uses default page_size and synchronous, for example).
This is probably something we want if other consumers start using ATTACH, I agree it's not a priority for now.