457110 - Support in-memory DB for the downloads manager back-end

Status: RESOLVED FIXED

(Toolkit :: Downloads API, enhancement)

Description

[(no longer active)]

We need support to use an in-memory DB in the downloads manager back-end, to be able to use it in the private browsing mode.

The in-memory DB's schema should be compatible with the on-disk DB, so that the existing download manager code would be able to use it without any change.  Also, we need dynamic switching between the on-disk and in-memory DBs at runtime.

Copying the existing contents of the on-disk DB when switching to an in-memory DB and vice versa may be desirable for other purposes, but it's out of the scope for this bug.  The in-memory DB initialized in this bug is empty at the beginning, and its contents will be discarded when switching back to the on-disk DB.

A patch will soon follow.

Comment 1

[(no longer active)]

I'm not sure what to do with mCurrentDownloads.  nsDownloadManager::RemoveAllDownloads has a comment which explicitly states that it should only be called when quitting the application.  Shawn, do you have any suggestions?

Comment 2

[Shawn Wilsher :sdwilsh]

We are, effectively, quitting the application here.  What you want to do is a total teardown and then a new setup of the database.

Comment 3

[(no longer active)]

Attachment: Patch (v1)

OK, here's the patch together with complete unit tests.  Some explanation is in order to make the review process easier.

Here is how the patch works: I have added a boolean inMemoryDB attribute to nsIDownloadManager interface, which, when set to true, re-initializes the download manager's DB connection to an in-memory (and initially empty) database.  The original on-disk DB connection is closed at this point.  Download manager can switch to the on-disk DB at any time by setting this attribute to false.  I factored out the common DB init tasks (which are not too many) in InitDBCommon.  Since the in-memory DB is always empty, there is no need to handle schema versions, or upgrade from downloads.rdf (since the on-disk DB is the default DB connection which would be used if initializing from an old profile containing downloads.rdf by default).

Because all usual download manager operations should work when using an in-memory DB as well, I decided to create a new unit-test directory (called unit-inmemorydb) and duplicate all applicable tests there, using in-memory DB instead of the default on-disk DB.  This allows future download manager unit tests to target both the on-disk and in-memory DB implementations.  The unit tests which were not applicable (i.e., migration unit tests, or those which tried to initialize the DB from a .sqlite or .rdf file) were omitted.  The unit-inmemorydb/test_download_manager.js, in addition to testing usual download manager operations (like unit/test_download_manager.js), also makes sure the DB changes are robust (by first running the tests using the on-disk DB, then switching to the in-memory DB and running the tests once again, and finally switching to the on-disk DB again and run the tests once more).  In order to ease the process of reviewing the tests section, I'll attach the result of the following command as well.

diff -r -u8 -p -N toolkit/components/downloads/test/unit toolkit/components/downloads/test/unit-inmemorydb

Last, but not least, one thing which caused a lot of debugging hours was that I found out that the download manager does *not* send the dl-cancel notification, which was already used in unit/test_download_manager.js (http://mxr.mozilla.org/mozilla-central/search?string=dl-cancel) and was crucial to the unit-inmemorydb/test_download_manager.js test I was writing.  The fix was simple, and it's included in this patch as well.  I made a few changes to the do_test_(pending|finished) dance which was performed in unit/test_download_manager.js to make sure it passes now that the dl-cancel notification is actually being passed.

Comment 4

[(no longer active)]

Attachment: Unit test diff

As promised.

Comment 5

[(no longer active)]

I've pushed this patch to the try server <http://hg.mozilla.org/try/rev/0df8bf70f458> to make sure the unit tests pass on all three platforms.

Comment 6

[Shawn Wilsher :sdwilsh]

try server doesn't run unit tests. 

I should get to the reviews today or tomorrow.

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> Created an attachment (id=341153) [details]
> Unit test diff
Are these difference from what's in the patch?

(In reply to comment #3)
> Here is how the patch works: I have added a boolean inMemoryDB attribute to
> nsIDownloadManager interface, which, when set to true, re-initializes the
> download manager's DB connection to an in-memory (and initially empty)
> database.  The original on-disk DB connection is closed at this point. 
> Download manager can switch to the on-disk DB at any time by setting this
> attribute to false.  I factored out the common DB init tasks (which are not too
> many) in InitDBCommon.  Since the in-memory DB is always empty, there is no
> need to handle schema versions, or upgrade from downloads.rdf (since the
> on-disk DB is the default DB connection which would be used if initializing
> from an old profile containing downloads.rdf by default).
I don't like the idea of exposing the inMemory stuff as a public interface.  Why not use an observer topic like everything else?

> Because all usual download manager operations should work when using an
> in-memory DB as well, I decided to create a new unit-test directory (called
> unit-inmemorydb) and duplicate all applicable tests there, using in-memory DB
> instead of the default on-disk DB.  This allows future download manager unit
> tests to target both the on-disk and in-memory DB implementations.  The unit
> tests which were not applicable (i.e., migration unit tests, or those which
> tried to initialize the DB from a .sqlite or .rdf file) were omitted.  The
> unit-inmemorydb/test_download_manager.js, in addition to testing usual download
> manager operations (like unit/test_download_manager.js), also makes sure the DB
> changes are robust (by first running the tests using the on-disk DB, then
> switching to the in-memory DB and running the tests once again, and finally
> switching to the on-disk DB again and run the tests once more).  In order to
> ease the process of reviewing the tests section, I'll attach the result of the
> following command as well.
Instead of copying all the files, can we not blacklist files we don't want tested, and dymanically load the other test files and run them with an in-memory database?  If we need to make changes to the tests (which we've had to do in the past, and expect to have to do in the future), this will double the work (as well as double the number of tests that have to be added in general).

Comment 8

[(no longer active)]

(In reply to comment #7)
> Are these difference from what's in the patch?

Yes, exactly.  Of course the unit/test_download_manager.js has been modified a bit, and the diff in this attachment has been made on the modified version of that file.

> (In reply to comment #3)
> I don't like the idea of exposing the inMemory stuff as a public interface. 
> Why not use an observer topic like everything else?

The only reason I exposed inMemoryDB on the public interface was to be able to use it from js for the tests.  If there's another way to test this besides exposing that on the public interface, then I'm all for it.

> Instead of copying all the files, can we not blacklist files we don't want
> tested, and dymanically load the other test files and run them with an
> in-memory database?

It's a good idea, but I'm not sure how to implement it.  The problem is that do_import_script, for example, does not provide a way to "unload" the imported script, so that the next one can be loaded (so that for example, multiple definition errors do not occur).  In fact, I'm not sure if that's at all possible with the JS engine.  Can you think of another way which makes this possible?

> If we need to make changes to the tests (which we've had
> to do in the past, and expect to have to do in the future), this will double
> the work (as well as double the number of tests that have to be added in
> general).

Yes, and it doubles the chance of getting something wrong in one of the two tests...

BTW, did you get a look on the implementation?  It would be great to see your comments on the implementation as well.

Thanks!

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> The only reason I exposed inMemoryDB on the public interface was to be able to
> use it from js for the tests.  If there's another way to test this besides
> exposing that on the public interface, then I'm all for it.
I'd prefer to do this with an observer.  You can QI the download manager service to nsIObserver, and dispatch accordingly in the tests.

> It's a good idea, but I'm not sure how to implement it.  The problem is that
> do_import_script, for example, does not provide a way to "unload" the imported
> script, so that the next one can be loaded (so that for example, multiple
> definition errors do not occur).  In fact, I'm not sure if that's at all
> possible with the JS engine.  Can you think of another way which makes this
> possible?
You can use mozIJSSubScriptLoader and load each test file onto an object (the optional second argument).  You can either make a new object for each file, or delete the object and remake it for each file.

> BTW, did you get a look on the implementation?  It would be great to see your
> comments on the implementation as well.
I didn't see anything terribly wrong with it, but I'll take a closer look a bit later today.

Comment 10

[(no longer active)]

(In reply to comment #9)
> I'd prefer to do this with an observer.  You can QI the download manager
> service to nsIObserver, and dispatch accordingly in the tests.

OK, I'll do that.

> You can use mozIJSSubScriptLoader and load each test file onto an object (the
> optional second argument).  You can either make a new object for each file, or
> delete the object and remake it for each file.

Thanks for the tip.  That object would be considered the global object, right?  I'll try to work out a bit more on this, and I'll see what I can come up with...

> I didn't see anything terribly wrong with it, but I'll take a closer look a bit
> later today.

Cool, thanks!

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> Thanks for the tip.  That object would be considered the global object, right? 
> I'll try to work out a bit more on this, and I'll see what I can come up
> with...
I don't think it needs to be the global object, but I'm not sure.  You might need to ask this one on irc to the JS folks.

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 341152 [details] [diff] [review]
Patch (v1)

>+void
>+nsDownloadManager::PauseAndRemoveAllDownloads()
>+{
>+  // Try to pause all downloads and, if appropriate, mark them as auto-resume
>+  // unless user has specified that downloads should be canceled
>+  enum QuitBehavior behavior = GetQuitBehavior();
>+  if (behavior != QUIT_AND_CANCEL)
>+    (void)PauseAllDownloads(PRBool(behavior != QUIT_AND_PAUSE));
>+
>+  // Remove downloads to break cycles and cancel downloads
>+  (void)RemoveAllDownloads();
>+}
I think we want them to automatically resume when we exit private browsing mode.

> nsresult
>+nsDownloadManager::InitDBCommon(nsIFile *dbFile)
I would prefer to have two different methods here (each returning an already_AddRefd<mozIStorageConnection> to get the proper database.

>+nsDownloadManager::InitInMemoryDB()
InitMemoryDB please

> nsDownloadManager::InitDB(PRBool *aDoImport)
InitFileDB now please

>+      nsCOMPtr<mozIStorageService> storage =
>+        do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID, &rv);
>+      NS_ENSURE_SUCCESS(rv, rv);
just drop the &rv, and NS_ENSURE_TRUE it

sorry for the delay on this

Comment 13

[(no longer active)]

Attachment: Patch (v2)

(In reply to comment #12)
> I think we want them to automatically resume when we exit private browsing
> mode.

Yes, but I will handle it in my patch on bug 248970.  The reason that we have to handle pausing and removing the downloads when switching the DB is that otherwise, we would end up with running downloads which we can't track.

> > nsresult
> >+nsDownloadManager::InitDBCommon(nsIFile *dbFile)
> I would prefer to have two different methods here (each returning an
> already_AddRefd<mozIStorageConnection> to get the proper database.

Done.

> >+nsDownloadManager::InitInMemoryDB()
> InitMemoryDB please
> 
> > nsDownloadManager::InitDB(PRBool *aDoImport)
> InitFileDB now please

Done.

> >+      nsCOMPtr<mozIStorageService> storage =
> >+        do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID, &rv);
> >+      NS_ENSURE_SUCCESS(rv, rv);
> just drop the &rv, and NS_ENSURE_TRUE it

Done.

This patch also includes the fixes requested in comment 7.  I've yet to come up with a working mechanism in order to load all tests from a single unit test, as proposed in comment 7, so this patch doesn't have any tests yet, but the tests will certainly follow.

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> (In reply to comment #12)
> > I think we want them to automatically resume when we exit private browsing
> > mode.
> 
> Yes, but I will handle it in my patch on bug 248970.  The reason that we have
> to handle pausing and removing the downloads when switching the DB is that
> otherwise, we would end up with running downloads which we can't track.
My comment there was more about how you are calling PauseAllDownloads.  I think you just want to pass PR_TRUE to it.

Comment 15

[Shawn Wilsher :sdwilsh]

Comment on attachment 342040 [details] [diff] [review]
Patch (v2)

>+already_AddRefed<mozIStorageConnection>
>+nsDownloadManager::GetFileDBConnection(nsIFile *dbFile) const
>+{
>+  if (!dbFile)
>+    return nsnull;
This should just be an assertion if anything.

>+  mozIStorageConnection * conn = nsnull;
Please use an nsCOMPtr here

>+  nsresult rv = storage->OpenDatabase(dbFile, &conn);
>+  if (rv == NS_ERROR_FILE_CORRUPTED) {
>+    // delete and try again, since we don't care so much about losing a user's
>+    // download history
>+    rv = dbFile->Remove(PR_FALSE);
>+    NS_ENSURE_SUCCESS(rv, nsnull);
>+    storage->OpenDatabase(dbFile, &conn);
>+    NS_IF_ADDREF(conn);
don't add ref, sore rv, and then NS_ENSURE_SUCCESS(rv, nsnull) after if (other errors could be returned).

>+  return conn;
return conn.forget() (once you make it an nsCOMPtr)

>+already_AddRefed<mozIStorageConnection>
>+nsDownloadManager::GetMemoryDBConnection() const
>+{
>+  mozIStorageConnection * conn = nsnull;
nsCOMPtr

>+  storage->OpenSpecialDatabase("memory", &conn);
>+  NS_IF_ADDREF(conn);
don't add ref, sore rv, and then NS_ENSURE_SUCCESS(rv, nsnull) after if (other errors could be returned).

>+  return conn;
return conn.forget() (once you make it an nsCOMPtr)

Comment 16

[(no longer active)]

Attachment: Patch (v3)

Fixed all of the issues in comment 15.

Comment 17

[(no longer active)]

Attachment: Patch (v3.1)

(In reply to comment #14)
> My comment there was more about how you are calling PauseAllDownloads.  I think
> you just want to pass PR_TRUE to it.

Oops, sorry I didn't see this one before.  Fixed in this version.

Comment 18

[(no longer active)]

Attachment: Patch (v3.1)

grrr, the previous patch included all of the previous test stuff, plus what I've been working on.

Shawn, it would be great if you can take a look at the new test in that page (test_bug457110.js) and let me know what you think.  It's not working of course (it leads to a crash).  I'm nearly out of ideas on using the subscript loader...

Comment 19

[Shawn Wilsher :sdwilsh]

Comment on attachment 342300 [details] [diff] [review]
Patch (v3.1)

>+  mDBConn = dont_AddRef(GetMemoryDBConnection());
don't need dont_AddRef - nsCOMPtr's know what to do with an already_AddRefd object

>+  mDBConn = dont_AddRef(GetFileDBConnection(dbFile));
>+  if (!mDBConn)
>+    return NS_ERROR_NOT_AVAILABLE;
ditto, and please use NS_ENSURE_TRUE for both of these

>+nsresult
>+nsDownloadManager::SwitchToMemoryDB(PRBool aUseInMemoryDB)
>+{
>+  if ((aUseInMemoryDB && mUsingMemoryDB) ||
>+      (!aUseInMemoryDB && !mUsingMemoryDB))
>+    return NS_OK; // no-op
>+
>+  mUsingMemoryDB = aUseInMemoryDB;
>+
>+  (void)PauseAllDownloads(PR_TRUE);
>+  (void)RemoveAllDownloads();
>+
>+  return Init();
I'd much rather see this function take an ENUM to indicate what state we want to be in, and act accordingly.  The name should be something like SwitchDatabaseTypeTo.

> #if defined(XP_WIN) && !defined(__MINGW32__)
>-  nsDownloadManager() : mScanner(nsnull) { };
>+  nsDownloadManager() : mScanner(nsnull),
>+                        mUsingMemoryDB(PR_FALSE)
>+  { };
> private:
>   nsDownloadScanner *mScanner;
>+#else
>+  nsDownloadManager() : mUsingMemoryDB(PR_FALSE)
>+  { };
since you are touching this, please use this format:

  nsDownloadMangaer() :
      mScaller(nsnull)
    , mUsingMemoryDB(PR_FALSE)
  {
  }


r=sdwilsh with those comments addressed.

I thought about the tests, and I don't care if you run all the tests with both types of db.  It's still a mozIStorageConnection, so if it works one way, it'll work the other.  If it doesn't it's a bug in storage, and a unit test will be made there.

Therefore, the only thing I need from you is a test that makes sure we switch the db types accordingly when the observer topic is received.

Comment 20

[(no longer active)]

Attachment: Patch (v3.2)

(In reply to comment #19)

All requested fixes done.

> I thought about the tests, and I don't care if you run all the tests with both
> types of db.  It's still a mozIStorageConnection, so if it works one way, it'll
> work the other.  If it doesn't it's a bug in storage, and a unit test will be
> made there.

Agreed.

> Therefore, the only thing I need from you is a test that makes sure we switch
> the db types accordingly when the observer topic is received.

OK, I've written a test for that which is included in this patch.

Comment 21

[Shawn Wilsher :sdwilsh]

Comment on attachment 343092 [details] [diff] [review]
Patch (v3.2)

>+already_AddRefed<mozIStorageConnection>
>+nsDownloadManager::GetFileDBConnection(nsIFile *dbFile) const
>+{
>+  NS_ASSERTION(dbFile, "GetFileDBConnection called with an invalid nsIFile");
>+
>+  nsCOMPtr<mozIStorageConnection> conn;
nit: declare this just before you use it, not at the top

>+already_AddRefed<mozIStorageConnection>
>+nsDownloadManager::GetMemoryDBConnection() const
>+{
>+  nsCOMPtr<mozIStorageConnection> conn;
ditto

>       // if the statement fails, that means all the columns were not there.
>       // First we backup the database
>+      nsCOMPtr<mozIStorageService> storage =
>+        do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID);
>+      NS_ENSURE_TRUE(storage, NS_ERROR_NOT_AVAILABLE);
>       nsCOMPtr<nsIFile> backup;
>+
nit: lose the newline

>+  switch (mDBType) {
>+  case DATABASE_MEMORY:
>+    rv = InitMemoryDB();
>+    break;
>+
>+  case DATABASE_DISK:
>+    rv = InitFileDB(&doImport);
>+    break;
>+
>+  default:
>+    NS_ASSERTION(0, "Unexpected value encountered for nsDownloadManager::mDBType");
>+    break;
>+  }
Please follow the dominate style for switch statements in this file (indent case statements).

>+  nsDownloadManager()
>+    : mDBType(DATABASE_DISK)
>+  {
>+  }
: on function declaration line please (see comment 19)

>diff --git a/toolkit/components/downloads/test/unit/test_bug_457110.js b/toolkit/components/downloads/test/unit/test_bug_457110.js
This isn't a useful name.  Please do not use the bug number and try to describe what's being tested with the name.

>+ * Contributor(s):
>+ *   Ehsan Akhgari <[e-mail]> (Original Author)
Don't need this since you are the original author.

>+function run_test() {
>+  let observer = dm.QueryInterface(Ci.nsIObserver);
>+
>+  // make sure the initial disk-based DB is initialized correctly
>+  let connDisk = dm.DBConnection;
>+  do_check_true(connDisk.connectionReady);
>+  do_check_neq(connDisk.databaseFile, null);
>+
>+  // switch to a disk DB -- should be a no-op
>+  observer.observe(null, "dlmgr-switchdb", "disk");
>+
>+  // make sure that the database has not changed
>+  do_check_eq(connDisk, dm.DBConnection);
>+  do_check_true(connDisk.connectionReady);
>+  do_check_neq(connDisk.databaseFile, null);
>+
>+  // switch to a memory DB
>+  observer.observe(null, "dlmgr-switchdb", "memory");
>+
>+  // make sure the DB is has been switched correctly
>+  let connMemory = dm.DBConnection;
>+  do_check_neq(connDisk, connMemory);
>+  do_check_true(connMemory.connectionReady);
>+  do_check_eq(connMemory.databaseFile, null);
>+
>+  // switch to a memory DB -- should be a no-op
>+  observer.observe(null, "dlmgr-switchdb", "memory");
>+
>+  // make sure that the database has not changed
>+  do_check_eq(connMemory, dm.DBConnection);
>+  do_check_true(connMemory.connectionReady);
>+  do_check_eq(connMemory.databaseFile, null);
>+
>+  // switch back to the disk DB
>+  observer.observe(null, "dlmgr-switchdb", "disk");
>+
>+  // make sure that the disk database is initialized correctly
>+  do_check_neq(connMemory, dm.DBConnection);
>+  do_check_neq(connDisk, dm.DBConnection);
>+  connDisk = dm.DBConnection;
>+  do_check_true(connDisk.connectionReady);
>+  do_check_neq(connDisk.databaseFile, null);
>+}
Not sure I like the fact that you are comparing storage objects like that.  You should be checking the database paths for the file ones.  I'm not sure why you are checking that the connection is not being reused - that's not part of the spec and not something I want to guarantee.  Holding onto the database connection could lead to bugs if we decide to change some behaviors down the line.

>diff --git a/toolkit/components/downloads/test/unit/test_download_manager.js b/toolkit/components/downloads/test/unit/test_download_manager.js
I don't understand the need for the changes in this file

Comment 22

[(no longer active)]

Attachment: Patch (v3.3)

(In reply to comment #21)
> Not sure I like the fact that you are comparing storage objects like that.  You
> should be checking the database paths for the file ones.  I'm not sure why you
> are checking that the connection is not being reused - that's not part of the
> spec and not something I want to guarantee.  Holding onto the database
> connection could lead to bugs if we decide to change some behaviors down the
> line.

OK, I changed the checks to use nsIFile::Equals which is supposed to return true if the file objects point to the same file.  Also I eliminated the checks for making sure no-op switch operations actually don't change the DB connection.

> >diff --git a/toolkit/components/downloads/test/unit/test_download_manager.js b/toolkit/components/downloads/test/unit/test_download_manager.js
> I don't understand the need for the changes in this file

Sorry those were left-overs from previous attempts, and are no longer necessary.

All other comments and nits fixed.  I also spun off the dl-cancel notification part to bug 459988.

Comment 23

[Shawn Wilsher :sdwilsh]

Comment on attachment 343177 [details] [diff] [review]
Patch (v3.3)

r=sdwilsh

Comment 24

[(no longer active)]

Pushed to mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/d4e34c95cac3>