423273 - Fix storage API to not break old behavior

Status: RESOLVED FIXED

(Toolkit :: Storage, defect, P1)

Description

[Shawn Wilsher :sdwilsh]

More fallout from Bug 416173.  This is actually causing the crash in bug 422687 too...

We need to be testing mozIStorageConnection::connectionReady instead of looking for a throw.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

nsFormHistory::OpenDatabase and nsDOMStorageDB::Init have essentially the same code. Do they need to be fixed as well?

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

My understanding, almost completely unaided by this bug, is that this is a moderate dataloss scenario?  Without more detail, though, it's hard to understand why this might or might not be a blocker, so hopefully enlightenment will come before the window closes.

Are we currently exposing an API to extensions that is trivial crash/dataloss bait, especially if they copied the pattern from the existing code in the tree?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Attachment: not the right patch

Is there still benefit to checking for NS_ERROR_FILE_CORRUPTED, or would that be redundant?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: potential patch

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #1)
> nsFormHistory::OpenDatabase and nsDOMStorageDB::Init have essentially the same
> code. Do they need to be fixed as well?
Yeah - that's two more consumers I didn't know about...

(In reply to comment #2)
> My understanding, almost completely unaided by this bug, is that this is a
> moderate dataloss scenario?  Without more detail, though, it's hard to
> understand why this might or might not be a blocker, so hopefully enlightenment
> will come before the window closes.
dataloss if we can't open the database, which usually means it got corrupted somehow.  However, the database will never be removed if it got into that state, so it'll always fail (before).

> Are we currently exposing an API to extensions that is trivial crash/dataloss
> bait, especially if they copied the pattern from the existing code in the tree?
We changed the API - it was documented on devmo.

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Documenting it on MDC was clearly not enough to catch even the in-tree callers, so I think my question remains: does trivial misuse of the API here result in crashes or dataloss, and why didn't we just make the API safe if so, given that we were changing it already?

I'm not asking if we can say "told you so" when some extension hits this problem, since that is typically not satisfying for users.  I'm asking what we've done to mitigate extensions hitting this problem, if they copied the code from ours in a well-meaning way, perhaps right up until beta 5.  (When did we change the API?  Was it late-compat?)

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Ah yes, was late compat -- I misread comment 0 initially.

In the case in which the connection is not ready, could we just throw?  It seems like an exceptional error case, and therefore well-suited to an exception.  Callers can then check the connection state, or inspect the exception, to decide if they want to back up and start over.

Comment 8

[Shawn Wilsher :sdwilsh]

We can't throw actually.  We need the connection object to hang around so methods that operate on the file, regardless of the database being open or not, can still work.  Throwing means we don't get a mozIStorageConnection object back.

People will find that their code doesn't work pretty quickly if the database gets corrupted anyway.  The crash that found this bug happened simply because we weren't erroring out if the connection wasn't opened.  I had thought all those instances had been fixed, but apparently one was missed.

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I think it's pretty weird API to get an object back from open if it can't be opened.  I also think it's pretty weird to use the object associated with an unopened and corrupt database to manipulate that database for backup.

I would have expected open to throw, and then the unusual case to take more code, possibly mStorageService.getConnectionFor("filename") (where .openDatabase is really sugar for .getConnectionFor("filename").open(), then).  It sure seems like connections and databases are different objects here, given the different constraints on their use.

(I mean throw when the connection is used in a non-ready state, so that "corrupt db" defaults to being handled like other failures, and authors can add special handling for the specific error if they care to.  Or you could put backupDB on the database service, rather than on the individual database object.)

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Assuming this is P1 because of the API compat story ... if not, please retarget to P2/Firefox 3 so we can get it out of the blocking path on Beta 5.

Comment 11

[Mike Connor [:mconnor]]

I think the old codepath needs to just not work if its not stable/reliable.  And if we're doing that, we should do it for b5...

Comment 12

[Shawn Wilsher :sdwilsh]

We just don't throw anymore - that's the only thing that has changed.  gavin's patch is correct, although I wouldn't bother backing up since we never did that before either.

I won't have cycles for this for at least a week at best, so if this is to make the beta, someone else should pick it up (and we ought to fix those other instances in comment 1

(In reply to comment #3)
> Is there still benefit to checking for NS_ERROR_FILE_CORRUPTED, or would that
> be redundant?
It won't throw that anymore.

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
First off - sorry, I misread this earlier so it wasn't properly addressed.

> I would have expected open to throw, and then the unusual case to take more
> code, possibly mStorageService.getConnectionFor("filename") (where
> .openDatabase is really sugar for .getConnectionFor("filename").open(), then). 
> It sure seems like connections and databases are different objects here, given
> the different constraints on their use.
I like this idea of using getConnection instead of openDatabase.  This does go along with what we actually return now, and it *will* break add-on's from doing this the old way.  I'm playing catch-up in school right now and don't have time for patches (I'm trying to keep up with bugmail and reviews though).  I don't expect this situation to improve for at least a week, if not longer, so if we want to do this for the Firefox 3, which means this beta, we'll need to get someone else to do it.  Maybe Edward (I told you I'd get you doing storage patches sooner or later)?

> (I mean throw when the connection is used in a non-ready state, so that
> "corrupt db" defaults to being handled like other failures, and authors can add
> special handling for the specific error if they care to.  Or you could put
> backupDB on the database service, rather than on the individual database
> object.)
So, the idea for backupDB was for corrupt and non-corrupt database connections.  Consumers store the connection object, but rarely store the file of the database, so the best place, at the time at least, was on the connection object.

Comment 14

[Mike Schroepfer]

Ondrej can you assist here?

Comment 15

[Ondrej Brablc]

All my other blocking bugs are for Firefox 3 and P2, so I can take this. 

So this bug is about moving backupDB from mozIStorageConnection to mozIStorageService and about throwing again when creation of database connection fails (that is rollback of bug 416173 and related)?

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> All my other blocking bugs are for Firefox 3 and P2, so I can take this. 
> 
> So this bug is about moving backupDB from mozIStorageConnection to
> mozIStorageService and about throwing again when creation of database
> connection fails (that is rollback of bug 416173 and related)?
No, that's not the impression I have from comment 9 - I tried to better clarify it in comment 13.  s/openDatabase/getConnection/, with a call to open then, if I understood shaver correctly.

Comment 17

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

IMO, we should do:
- openDatabase should remain, and throw ERROR_CORRUPT if the DB is corrupt.

And one of:
- there should be a getConnection which returns a connection which can be used to open or back up
- backupDB should go onto the database service, taking source-and-dest filenames

I think the latter is the most expedient here, rather than introducing new API objects in a rush.  It can be deprecated or redefined in terms of a connection object later if we find another need for a distinct object.

Comment 18

[Shawn Wilsher :sdwilsh]

That kinda makes backupDB useless for it's original, intended use case of taking an existing good database connection and backing it up to a unique file...

using it for the corrupted case was an afterthought.

just adding a getConnection (and depreciating openDatabase) isn't hard.  All you need to do is check the return variable of the Initialize call.

Comment 19

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

openDatabase is a useful cliche, I think we should preserve it as sugar rather than deprecating it.  If it's too awkward to get the filename from the database to feed it to databaseService.backupDB (one would think that we'd want it hanging around there for use in diagnostic messages and so forth) and it's easy to add another object into the mix, roll on!  (Do keep openDatabase, though; heed well the lesson of Java.)

Comment 20

[Shawn Wilsher :sdwilsh]

The problem is that we cannot return an object if we throw.  That's why we have no access to the filename.

Comment 21

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

The caller has access to the filename -- they just passed it into openDatabase.  I'm talking about the "original use case" for doing backup of an existing database connection. By definition, that means we didn't throw, amirite?

Comment 22

[Shawn Wilsher :sdwilsh]

aye, that is correct.  Also, care to elaborate on your Java comment - I think it was a bit before my time...

Comment 23

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Java APIs tend to be Architecturally Pure and totally devoid of useful or incremental sugar.  A lot of Mozilla APIs tend to be that way too: because someone _could_ need to tweak any number of things, every caller _has_ to supply values for them, even though they might not know or care what they do.  If instead you have a simple API that points people towards the most common and correct uses, with optional APIs to tweak behaviours (set attributes on the connection, add a listener instead of it needing to be provided/stubbed all the time, etc.), then people encounter complexity as they need complex things rather than having to swallow all the combination-space of the previous N years of use cases in one go.

https://mollyrocket.com/forums/viewtopic.php?t=215 is a great presentation about API design, and has explicit mention of this concept of incremental complexity and sane defaults.

Comment 24

[Ondrej Brablc]

I still think the same as in my comment #15, that is to:

1) Rollback following patches:

- Attachment #270770 [details] [diff]: v1.2 for bug #386394
- Attachment #301978 [details] [diff]: v1.1 for bug #416173
- Attachment #301997 [details] [diff]: v1.1 for bug #386768
- Attachment #309777 [details] [diff]: combined for bug #422687
- Attachment #301995 [details] [diff]: v1.1 for bug #416205
- Attachment #305969 [details] [diff]: like so for bug #416207
- Attachment #305415 [details] [diff]: patch v2: comments -> javadoc comments for bug #416208
- Attachment #304697 [details] [diff]: The patch for bug #418804
- Attachment #304840 [details] [diff]: fix for bug #418897

2) Mark invalid bug 418819 and bug 423273 (this bug).

3) Create a new backup methods in mozIStorageService:

  nsIFile generateCorruptedDatabaseFileName(in nsIFile aDatabaseFile);
  void backupDatabase(in nsIFile aDatabaseFile, in nsIFile aBackupFile);
 
A new umbrella bug should be filed to fix the storage API and individual bugs should be submitted for other components.

I would like to see some common agreement before I rush into doing all this.

Comment 25

[Shawn Wilsher :sdwilsh]

Sorry, but I disagree.  Point 3 missed the while original idea of the backupDB method.

What shaver suggested, and I agree with is add the following methods to mozIStorageService:
mozIStorageConnection getConnection(in nsIFile aDatabaseFile);
mozIStorageConnection getSharedConnection(in nsIFile aDatabaseFile);

and to mozIStorageConnection:
boolean open(); // returns what connectionReady would return


The code in mozStorageService.cpp would have to be changed to check the value of Initialize again, and call Open.  The code in mozStorageConnection::Initialize should be factored out to not do the opening stuff of the database, and that placed in Open.

This is the least compatibility breaking solution, and I think it's best given how late we are in the cycle.

Comment 26

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #25)
> What shaver suggested, and I agree with is add the following methods to
> mozIStorageService:
> mozIStorageConnection getConnection(in nsIFile aDatabaseFile);

getUnsharedConnection?  getPrivateConnection?

> mozIStorageConnection getSharedConnection(in nsIFile aDatabaseFile);
> 
> and to mozIStorageConnection:
> boolean open(); // returns what connectionReady would return

attribute readonly boolean open;

Comment 27

[Shawn Wilsher :sdwilsh]

(In reply to comment #26)
> getUnsharedConnection?  getPrivateConnection?
Why, exactly?

> > and to mozIStorageConnection:
> > boolean open(); // returns what connectionReady would return
> 
> attribute readonly boolean open;
but why - it's actually doing the work of opening the database at this point.

Comment 28

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Given "getSharedConnection", "getConnection" is different how?  How do you know you want to call getConnection -- when you've looked at all the more specialized get*Connection methods and none of their specialness is applicable?  If we need a way to make a connection unshared, perhaps |conn = conn.makePrivate()| or |conn = conn.makeUnshared()| is better API.  (I haven't read the docs, I confess, so you can RTFM me if that's appropriate, but I don't think there's a common notion of "shared" in database app development, so our uniqueness here probably needs some care and explicitness.)

What is the boolean for on open?  Shouldn't failing to open a database be exceptional, as it was before, since operating on an unready connection causes crashing or corruption or other badness, as evidenced by this whole bug?

And I want to reiterate that storageService.openDatabase() should remain, to avoid pointless compatibility and complexity churn!

Comment 29

[Ondrej Brablc]

Sharing is just about sharing memory buffers, it is never used in Mozilla and I would suggest to delete it. A better name would be openDatabaseWithExclusiveMemoryCache.

There is a missing answer what would happen when you call backupDB on active connection: https://bugzilla.mozilla.org/show_bug.cgi?id=386394#c7

We should really revert back to the old API we used to have before. There is no real justification why we should so late change the basics of how one opens a connection (see comment #21).

Comment 30

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Sharing memory buffers with which?  All other connections that are opened with "shared"?  A subset that can be specified?  If we don't use it, I agree that we should remove it.

Having the old API back, plus backupDB entry points on storageService for use in the case of a failed open, would seem indeed to suffice.

Comment 31

[Ondrej Brablc]

(In reply to comment #30)
> Sharing memory buffers with which?  All other connections that are opened with
> "shared"?  A subset that can be specified?  If we don't use it, I agree that we
> should remove it.

When we init the storage we call sqlite3_enable_shared_cache(1). Connections opened to the same file (from the same process) will share single memory cache. When we call openUnsharedDatabase, it will clear this flag, open database and raise the flag again. So this connection will have its own memory cache.

http://www.sqlite.org/sharedcache.html

You cannot combine sharing of memory cache with usage of virtual tables. Virtual table can only be used with registered module and we do not have API to register such module.

http://www.sqlite.org/lang_createvtab.html

Comment 32

[Shawn Wilsher :sdwilsh]

First of all, I'm sorry - I meant getUnsharedConnection to mirror the openUnsharedDatabase call.

(In reply to comment #28)
> Given "getSharedConnection", "getConnection" is different how?  How do you know
> you want to call getConnection -- when you've looked at all the more
> specialized get*Connection methods and none of their specialness is applicable?
>  If we need a way to make a connection unshared, perhaps |conn =
> conn.makePrivate()| or |conn = conn.makeUnshared()| is better API.  (I haven't
> read the docs, I confess, so you can RTFM me if that's appropriate, but I don't
> think there's a common notion of "shared" in database app development, so our
> uniqueness here probably needs some care and explicitness.)
Due to how we have to enable and disabled the shared sqlite cache, we can't really do it that way (unless we want to use a more complicated locking strategy.  It's doable, but I'm not sure I'm comfortable doing that this late in the game).

> What is the boolean for on open?  Shouldn't failing to open a database be
> exceptional, as it was before, since operating on an unready connection causes
> crashing or corruption or other badness, as evidenced by this whole bug?
That's a good point - throwing on open would be more ideal.  let's go with |void open()| then.

> And I want to reiterate that storageService.openDatabase() should remain, to
> avoid pointless compatibility and complexity churn!
Aye, I'm agreeing with you here.

(In reply to comment #29)
> Sharing is just about sharing memory buffers, it is never used in Mozilla and I
> would suggest to delete it. A better name would be
> openDatabaseWithExclusiveMemoryCache.
Absolutely not.  This was specifically added for add-on authors in bug 413589 and I'll be quick to r- and patch that does this without a *very* good reason (imho, this bug is *not* a very good reason).

> There is a missing answer what would happen when you call backupDB on active
> connection: https://bugzilla.mozilla.org/show_bug.cgi?id=386394#c7
Sorry - I must have missed that comment.  I've addressed it now.

> We should really revert back to the old API we used to have before. There is no
> real justification why we should so late change the basics of how one opens a
> connection (see comment #21).
And the solution that shaver proposed and I agree with doesn't change the basics - in fact old code will work just the same.  It's just providing another way to get a connection object such that backupDB will work on connections that are corrupted as well.

Comment 33

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

.open should return a database, no?  And therefore be named that way?

stgSvc.getConnection("filename").openDatabase() =~ stgSvc.openDatabase("filename")

Can the flag-twiddling race with other opens that don't expect a shared connection?  Seems like sqlite doesn't really expect to be dealing with both shared and unshared connections at the same time, given the API, but I could be wrong.

Comment 34

[Shawn Wilsher :sdwilsh]

(In reply to comment #33)
> .open should return a database, no?  And therefore be named that way?
> 
> stgSvc.getConnection("filename").openDatabase() =~
> stgSvc.openDatabase("filename")
I was thinking stg.Svc.getConnection(nsIFileObject).open(), but either one works for me, and yours is arguably more descriptive.

Our locking has to be done a bit differently now too.  We'll have to make the lock a static variable on the mozStorageService class (from a private member that it currently is), and either make accessors for it so we can lock on the openDatabase call.  There's no need for getUnsharedConnection, but we'll need to do a openUnsharedDatabase() on mozIStorageConnection.

> Can the flag-twiddling race with other opens that don't expect a shared
> connection?  Seems like sqlite doesn't really expect to be dealing with both
> shared and unshared connections at the same time, given the API, but I could be
> wrong.
Addressed over im, but for others I'll quote the sqlite docs:
Each call sqlite3_enable_shared_cache() effects subsequent database connections created using sqlite3_open(), sqlite3_open16(), or sqlite3_open_v2(). Database connections that already exist are uneffected. Each call to sqlite3_enable_shared_cache() overrides all previous calls within the same process.

I can't wait until we can really attack this API after this release...

Comment 35

[Shawn Wilsher :sdwilsh]

I'm just going to take this since I know exactly what needs to be done...

Comment 36

[Shawn Wilsher :sdwilsh]

renoming for blocking - patch coming soonish

Comment 37

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Implements the proposed API of shaver, and reverts many changes that were done from the previous change.

I'm not sure if shaver's review will work on all modules, but I'm sure he'll tell me if it's not.

This presently fails on two places unit tests because the tests throw NS_ERROR_FILE_LOCKED.  I'm not really sure why/how that's happening, so if someone more familiar with places code could take a look, it'd be greatly appreciated.

While the mozIStorageConnectionOptions interface may seem like overkill now, this is something we discussed on doing in the disable async IO bug.  It can be used in the future to specify if async is on or off, and lots of other pragmas that have to be set before the database is used.  I'm adding it now so there is less api breakage down the line.

Comment 38

[Ondrej Brablc]

I'm happy that you took the bug, because I disagree with the implementation that you suggest. It seems wrong to me to do rush changes in API forth and back so late just in order to allow running backup on failed connection. Alone backupDB name is wrong, the API uses "database" always verbosely. openUnsharedDatabase is wrong too, the database can still be "shared" by multiple connections, it is just the memory cache what is exclusive.
 
Connection object is not the right place for backup, I may need to run backup on connection which is not opened (and I would really feel much safer to do so even if there would be a unit test that does backup of database with opened transaction). And again, comment #21 shows, that it is no problem for the caller to call backup on service object.

mozIStorageConnectionOptions is probably step in the right direction, alone this could make the API nice again even if you would keep the backup on connection object. I would suggest that you delete openUnsharedDatabase and replace it with openDatabaseWithOptions. Adding an additional option "nothrowOnFailure" or "allowBackupOnFailedDatabase" will make all the other changes in API superfluous.

Comment 39

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I've been thinking about this all weekend, and squinting at the patch and previous bugs, making a mockery of my plans to recharge.  I actually woke up in the middle of the night thinking about this.

That wrestling has led to a couple of conclusions:

1) I need some sort of therapy, clearly.

2) We should minimize net API changes (vs. b4 and vs. 1.8) 

(In reply to comment #38)
> I'm happy that you took the bug, because I disagree with the implementation
> that you suggest. It seems wrong to me to do rush changes in API forth and back
> so late just in order to allow running backup on failed connection. Alone
> backupDB name is wrong, the API uses "database" always verbosely.
> openUnsharedDatabase is wrong too, the database can still be "shared" by
> multiple connections, it is just the memory cache what is exclusive.

I agree with this: the backup API has a  (no doubt in part due to the amount of stuff loaded on sdwilsh for the FF3 endgame and his being largely abandoned by other storage peers, notably myself) and we should undo it.

> Connection object is not the right place for backup, I may need to run backup
> on connection which is not opened (and I would really feel much safer to do so
> even if there would be a unit test that does backup of database with opened
> transaction). And again, comment #21 shows, that it is no problem for the
> caller to call backup on service object.

Indeed, I don't think we need or want a connection object.  The only thing that we connect to is a database, and we have a database to represent that connection.  If we need to refer to the database without it opened, we can and should use the filename; it's sufficient to connect, obviously, since it's what's we use to actually open a database!

> mozIStorageConnectionOptions is probably step in the right direction, alone
> this could make the API nice again even if you would keep the backup on
> connection object. I would suggest that you delete openUnsharedDatabase and
> replace it with openDatabaseWithOptions. Adding an additional option
> "nothrowOnFailure" or "allowBackupOnFailedDatabase" will make all the other
> changes in API superfluous.

openDatabaseWithOptions would be OK, but I think mozIStorageConnectionOptions is the wrong way to go about it.  It'd be righteous for JavaScript callers:

  openDatabase("q.db", {shared: true, debug: true, readahead: 2000})

as long as it was really clear what the defaults were and NS_ERROR_NOT_IMPLEMENTED was handled gracefully in the openDatabase code.  But for C++ callers, it would be quite gross: you'd have to instantiate a stgConnectionOptions object and populate it, and then you would have zero ABI compatibility without incredibly gross IID tracking inside openDatabase.  AIUI the majority use case for the shared-cache control is for extensions to our database model that will require C++ code.  So as long as we have only boolean flags, I think we should reflect them as consts on the interface and bit-or them together.

So I think we should:
- roll back the changes to before .connectionReady
- add backupDatabaseFile(srcfilename, destfilename) to the storage service for the backup-corrupt case
- leave shared/unshared/cache as it was before the .connectionReady mutation

What say ye gathered here?  Time is not on our side.

Comment 40

[Shawn Wilsher :sdwilsh]

(In reply to comment #39)
> I've been thinking about this all weekend, and squinting at the patch and
> previous bugs, making a mockery of my plans to recharge.  I actually woke up in
> the middle of the night thinking about this.
and here I thought I was the only crazy one doing this with the storage api...

> I agree with this: the backup API has a  (no doubt in part due to the amount of
> stuff loaded on sdwilsh for the FF3 endgame and his being largely abandoned by
> other storage peers, notably myself) and we should undo it.
I disagree, but clearly don't have a convincing argument on this, so I'll relent.

> openDatabaseWithOptions would be OK, but I think mozIStorageConnectionOptions
> is the wrong way to go about it.  It'd be righteous for JavaScript callers:
> 
>   openDatabase("q.db", {shared: true, debug: true, readahead: 2000})
yes, this is something I want to do for moz2

> as long as it was really clear what the defaults were and
> NS_ERROR_NOT_IMPLEMENTED was handled gracefully in the openDatabase code.  But
not sure what you are referring to by "NS_ERROR_NOT_IMPLEMENTED was handled gracefully".  Care to elaborate?

> for C++ callers, it would be quite gross: you'd have to instantiate a
> stgConnectionOptions object and populate it, and then you would have zero ABI
> compatibility without incredibly gross IID tracking inside openDatabase.  AIUI
I was wondering if this was going to be brought up.  I've never had to worry about ABI compatibility before, but I'll take a stab at a solution.  Would something like nsINavHistoryService::getNewQueryOptions() solve this?  They would get an object that implements all the defaults, and call set the ones they need.  No more needing to instantiate it themselves.

The thing I really like about this object (not currently implemented on it however), is the ability to set things like the page_size that must be set before the database is created (before any statements are executed on it - I'm unsure if it's read and/or write statements.  If it's read, our current implementation doesn't even work right...).

> the majority use case for the shared-cache control is for extensions to our
> database model that will require C++ code.  So as long as we have only boolean
> flags, I think we should reflect them as consts on the interface and bit-or
> them together.
I'm not sure you have the right understanding here.  The shared cache is used to boost performance for multiple connections to the same database.  None of this is because of language choices.  Perhaps I'm misunderstanding your understanding though...

As for the bit flags, that doesn't do us much good for things like setting page_size, page_count, etc.

> - leave shared/unshared/cache as it was before the .connectionReady mutation
This was done well after that.  Are you saying we should remove the non-shared cache code?  Or are you saying we should remove the locking that was introduced?  Or are you saying something else entirely?

Comment 41

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #40)
> >   openDatabase("q.db", {shared: true, debug: true, readahead: 2000})
> yes, this is something I want to do for moz2

You don't need moz2 for it -- that code would work today, with the API you proposed (though my example added debug and readahead as examples.

> > as long as it was really clear what the defaults were and
> > NS_ERROR_NOT_IMPLEMENTED was handled gracefully in the openDatabase code.  But
> not sure what you are referring to by "NS_ERROR_NOT_IMPLEMENTED was handled
> gracefully".  Care to elaborate?

If the caller omits a value for a property expected to be present (in implied getter/setter form) then XPConnect will return NS_ERROR_NOT_IMPLEMENTED.  This means that rather than (void)ing it away, you would need to treat getter failure as though the value were the default.  (Which means I maybe think that .shared should be .privateCache, since generally the default of an optional boolean parameter should be false.)

> > for C++ callers, it would be quite gross: you'd have to instantiate a
> > stgConnectionOptions object and populate it, and then you would have zero ABI
> > compatibility without incredibly gross IID tracking inside openDatabase.  AIUI
> I was wondering if this was going to be brought up.  I've never had to worry
> about ABI compatibility before, but I'll take a stab at a solution.  Would
> something like nsINavHistoryService::getNewQueryOptions() solve this?  They
> would get an object that implements all the defaults, and call set the ones
> they need.  No more needing to instantiate it themselves.

Sure, that's "instantiate a new one and populate it"; I was assuming we wouldn't make then do the whole XPCOM dance themselves.  They still have to do a lot more work than changing OPEN_DEFAULT to OPEN_SHARED_CACHE.

> The thing I really like about this object (not currently implemented on it
> however), is the ability to set things like the page_size that must be set
> before the database is created (before any statements are executed on it - I'm
> unsure if it's read and/or write statements.  If it's read, our current
> implementation doesn't even work right...).

You can open without write statements being executed, IIRC, so I don't see how that requires arguments to be passed to the open code.  Setting parameters can fail with NS_ERROR_TOO_LATE_LOL if statements have already been executed, or the storage service can grow .getUnopenedConnection(dbFile) while connection grows .open().  But that's all future stuff, of yet-unknown import, and we shouldn't be imposing this API regression on F3 extension and app developers in service of that vague goal.

> > the majority use case for the shared-cache control is for extensions to our
> > database model that will require C++ code.  So as long as we have only boolean
> > flags, I think we should reflect them as consts on the interface and bit-or
> > them together.
> I'm not sure you have the right understanding here.  The shared cache is used
> to boost performance for multiple connections to the same database.  None of
> this is because of language choices.  Perhaps I'm misunderstanding your
> understanding though...

I misunderstood the rationale for adding the control, indeed -- re-reading bug 413589 after your gentle clue-batting set me straight.  Thanks.

> As for the bit flags, that doesn't do us much good for things like setting
> page_size, page_count, etc.

Not a problem we need to solve now.  We can add .pageSize=8000 or setParameter("page_size", 8000) when we next crack the API open; given that you plan to revamp it incompatibly anyway, futurism doesn't pay its way at this point in the F3 cycle.

> > - leave shared/unshared/cache as it was before the .connectionReady mutation
> This was done well after that.  Are you saying we should remove the non-shared
> cache code?  Or are you saying we should remove the locking that was
> introduced?  Or are you saying something else entirely?

I was saying that we should leave shared/non-shared API as it is, and rollback any changes that may have been introduced in service of connectionReady.  There are changes to 

I do think that the method should be openDatabaseUnshared or (better) openDatabaseWithPrivateCache, because "Unshared" describes how it's being opened, rather than modifying "Database", but I don't think that's worth the change cost at this point.  We'll be pickier about the language when designing the API for storage2 :)

Comment 42

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #41)
> I was saying that we should leave shared/non-shared API as it is, and rollback
> any changes that may have been introduced in service of connectionReady.  There
> are changes to 

Got a little excited with the [Commit] button!

There are changes to OpenUnsharedConnection in your patch, and I was just over-clarifying that I think that stuff should just return to its earlier state.  I don't think that's controversial, so you probably weren't expecting me to say it. :)

Comment 43

[Mike Beltzner [:beltzner, not reading bugmail]]

Soo ... is that r- or r+, then?

Comment 44

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 311096 [details] [diff] [review]
v1.0

It's not r+, or it would be marked as r+! :)  I can mark it r-, though, to help track status.  Sorry!

Comment 45

[Mike Schroepfer]

Shawn/Shaver do we need this or sure for b5?

Comment 46

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yes, we do.  Need to get the broken connectionReady API gone before the major extension update push around the final beta, or we're going to be in a huge mess when RC1 drops.  (And the current API is an attractive nuisance, as per the original impetus for this bug.)

Comment 47

[Shawn Wilsher :sdwilsh]

I'll have a patch up later today.

Comment 48

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #37)
> This presently fails on two places unit tests because the tests throw
> NS_ERROR_FILE_LOCKED.  I'm not really sure why/how that's happening, so if
> someone more familiar with places code could take a look, it'd be greatly
> appreciated.

Mardak/Ondrej/Dietrich: can we get someone looking at this ASAP?

Comment 49

[Ed Lee :Mardak]

Which tests are it failing on?

Comment 50

[Shawn Wilsher :sdwilsh]

Seems to be working in the patch I'm about to attach.  Don't know why since it should the code that was causing the failure is the same in the two patches...

Comment 51

[Shawn Wilsher :sdwilsh]

Attachment: v2.0

I do believe this addresses everything that's been brought up.

Comment 52

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 311416 [details] [diff] [review]
v2.0

>+  /**
>+   * Copies the specified database file to the specified parent directory with
>+   * the specified file name.  If the parent directory is not specified, it
>+   * places the backup in the same directory as the current file.  This function
>+   * ensures that the file being created is unique.
>+   *
>+   * @param aDBFile
>+   *        The database file that will be backed up.
>+   * @param aFileName
>+   *        The name of the new file to create.
>+   * @param [optional] aParentDirectory
>+   *        The directory you'd like the file to be placed.
>+   * @return The nsIFile representing the backup file.
>+   */
>+  nsIFile backupDatabaseFile(in nsIFile aDBFile, in AString aFileName,
>+                             [optional] in nsIFile aParentDirectory);

Nits, follow your heart:

aBackupName?  "aFileName" is sort of ambiguous.
aBackupDirectory?

It's a little wierd to mix nsIFile and string, but I think it works pretty well
after sketching some sample code in a buffer.

>+
>+      try {
>+        dbConnection.openConnection()
>+      }
>+      catch (e) {
>+        // If the connection isn't ready after we open the database, that means
>+        // the database has been corrupted, so we back it up and then recreate it.
>+        if (e.result == Cr.NS_ERROR_FILE_CORRUPTED)
>+          dbConnection = this._dbBackUpAndRecreate(dbService, dbFile,
>+                                                   dbConnection);
>+      }

This isn't right, even ignoring the lack of braces around the multi-line then (maybe
that's house style?):

If the exception is not NS_ERROR_FILE_CORRUPTED, you'll just swallow it and continue.

  } catch (e if e.result == Cr.NS_ERROR_FILE_CORRUPTED) {

(This is a very very common error, which is why we (unsuccessfully) lobbied for catchguards
at ECMA, and why they're in our engine.)

r+sr=shaver with the catch issue fixed; rename the parameters to backupDatabaseFile if you
so choose!  Move the review markers forward to a fixed patch, and I'll be standing by with
approvalry.

Thanks a ton, Shawn; this was great work.

Comment 53

[Shawn Wilsher :sdwilsh]

Attachment: v2.1

(In reply to comment #52)
> Nits, follow your heart:
> 
> aBackupName?  "aFileName" is sort of ambiguous.
> aBackupDirectory?
Changed to aBackupFilename and aBackupParentDirectory.  I didn't really think about changing the names when I move the function...

> It's a little wierd to mix nsIFile and string, but I think it works pretty well
> after sketching some sample code in a buffer.
It was really weird when the interface didn't originally return an nsIFile that pointed to where it actually went (found that issue while trying to test it).

>   } catch (e if e.result == Cr.NS_ERROR_FILE_CORRUPTED) {
That is so cool and I didn't even know about it! :(

also fixed a bug with the content-preferences stuff.  I must have not ran the unit tests on it last time.

Comment 54

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 311441 [details] [diff] [review]
v2.1

r=shaver, good thing we didn't have to move flags forward! :)

Comment 55

[Shawn Wilsher :sdwilsh]

Comment on attachment 311441 [details] [diff] [review]
v2.1

Drivers:  Passes unit tests for affected components locally, and reverts an api change from 1.8.1.  The change this reverts could have broken add-ons (as well as code in our own codebase that was missed).

Comment 56

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 311441 [details] [diff] [review]
v2.1

a=shaver

Comment 57

[Shawn Wilsher :sdwilsh]

Checking in netwerk/cookie/src/nsCookieService.cpp;
new revision: 1.93; previous revision: 1.92
Checking in storage/public/mozIStorageConnection.idl;
new revision: 1.16; previous revision: 1.15
Checking in storage/public/mozIStorageService.idl;
new revision: 1.9; previous revision: 1.8
Checking in storage/src/mozStorageConnection.cpp;
new revision: 1.33; previous revision: 1.32
Checking in storage/src/mozStorageService.cpp;
new revision: 1.17; previous revision: 1.16
Checking in storage/test/unit/head_storage.js;
new revision: 1.8; previous revision: 1.7
Removing storage/test/unit/test_bug-416173.js;
new revision: delete; previous revision: 1.1
Checking in storage/test/unit/test_storage_connection.js;
new revision: 1.9; previous revision: 1.8
Checking in storage/test/unit/test_storage_service.js;
new revision: 1.3; previous revision: 1.2
Checking in toolkit/components/contentprefs/src/nsContentPrefService.js;
new revision: 1.15; previous revision: 1.14
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.177; previous revision: 1.176
Checking in toolkit/components/places/src/nsNavHistory.cpp;
new revision: 1.280; previous revision: 1.279
Checking in toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp;
new revision: 1.71; previous revision: 1.70

Comment 58

[Eric Shepherd [:sheppy]]

Docs updated:

http://developer.mozilla.org/en/docs/mozIStorageService
http://developer.mozilla.org/en/docs/mozIStorageConnection