970303 - [Download Manager] The download list is emptied after closing the settings app from task switcher

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[rafael.marquez]

*Procedure
1. Download some files
2. Open the download list and verify that tue files are displayed
3. Close settings app if it is opens. You can verify and closed it from task switcher
4. Reopen download list after closing settings app

*Expected Result
Are download files are displayed in download list after closing and reopening settings app

*Actual Result
The download list is emptied after closing the settings app


A bug similar to this was opened and resolved 2 months ago. bug 949411

Comment 1

[Ted Clancy [:tedders1]]

I find that reproducing this issue requires more specific steps than given above.

The STR I used are:

1) Boot the phone.
2) Open the browser. Go to http://owd.tid.es/dm/ . Download a few files.
3) Open the settings app. Go to the download screen.
Result: Downloaded files are listed.
4) Use the task manager to kill the download screen.
5) Use the task manager to kill the browser.
6) Open the settings app. Go to the download screen.
7) Select all the listed files and delete them.
8) Use the task manager to kill the download screen.
9) Open the browser. Go to http://owd.tid.es/dm . Download a few files.
10) Open the settings app. Go to the download screen.
Expected result: Downloaded files from step 9 are listed.
Actual result: Download list is blank.

Comment 2

[Ted Clancy [:tedders1]]

Rafael, can you attempt to reproduce the issue using the Steps to Reproduce in Comment 1? Thanks.

Comment 3

[Ted Clancy [:tedders1]]

Attachment: another_log.rtf

Comment 4

[Ted Clancy [:tedders1]]

I've added a bunch of logging, and I can see what's going on. (Log file is attached, though it will be largely indecipherable to anyone but me. Ask me if you have questions.)

There's an indexedDB database called |download_store|. It contains two |ObjectStores|s called |DataStoreDB| and |revision|.

When files are downloaded, the B2G process adds a new row to the |revision| ObjectStore. Rows are identified by a key which is a monotonically increasing integer. The B2G process keeps track of the last used key in a field called |nextAutoIncrementId| in a structure of type |ObjectStoreInfo|.

When the Download screen of the Settings app is started, it opens the |download_store| indexedDB database, and it reads in the last written |nextAutoIncrementId|. The B2G process creates a new instance of |ObjectStoreInfo| to store it. (Actually, it creates two new instances of |ObjectStoreInfo|. One seems unused. I think something is going wrong there.) When files are deleted, the Settings app adds new rows to |revision| and the B2G process increments |nextAutoIncrementId|.

When the Browser is reopened and new files are downloaded, the B2G process tries to create new rows in |revision| using the |nextAutoIncrementId| from the original instance of |ObjectStoreInfo|. This key value is now out-of-date, and attempting to use it leads to a key collision. The database never gets updated with information about the new downloads.

I'm not sure what the intended behaviour of indexedDB is supposed to be in this case. I suspect that the B2G process is only supposed to keep one instance of ObjectStoreInfo (for a given indexedDB database) that is shared by all child processes, but I'm not certain if that's the intended design. I'm going to consult with Ben Turner.

Comment 5

[Gregor Wagner [:gwagner]]

Should we reopen?

Comment 6

[Ted Clancy [:tedders1]]

Oh, we can re-open things? Cool. I think that's a good idea.

Comment 7

[Ted Clancy [:tedders1]]

There are two issues causing this problem.

The first issue is due to the way DatabaseInfo objects are being handled in indexedDB. DatabaseInfo objects are stored in a global hashtable, and there should be one DatabaseInfo object per database. DatabaseInfo objects are reference counted, and when they are no longer referenced, they remove themselves from the global hashtable.

However, when the Settings app is killed in step 4 of the STR, the DatabaseInfo object which represents the |download_store| database is explicitly removed from the global hashtable, even though the System app still has a reference to the it. (The System app is responsible for writing to the |download_store| when files are downloaded.) When the Settings app is re-launched in step 6, it creates a new DatabaseInfo object for the |download_store| database because it can't find a matching one in the global hashtable. This means we now have two DatabaseInfo objects representing the same database, one used by the System app and one used by the Settings app. The two objects end up with inconsistent information, leading to the violation of a database constraint.

The attached patch prevents the DatabaseInfo object from being removed from the global hashtable when the Settings app is killed.

The second issue is that there is a race condition in download_store.js. This race condition can cause inconsistent data to be stored in the database when multiple files are deleted at once. This causes a undefined value to be dereferenced in our javascript code. This race condition is further explained in Bug 981938.

The attached pull request does not fix the race condition. It merely prevents our javascript code from aborting in response to the inconsistent data. I expect the race condition to be fixed as part of Bug 981938.

Comment 8

[Ted Clancy [:tedders1]]

Attachment: bug-970303-fix-part-1.patch

Comment 9

[Ted Clancy [:tedders1]]

Attachment: Bug 970303 fix - Part 2

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8388923 [details] [review]
Bug 970303 fix - Part 2

Cristian is likely a better fit for this part of the code :)

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8388921 [details] [diff] [review]
bug-970303-fix-part-1.patch

Review of attachment 8388921 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, thanks for digging!

I assume the try server was happy with this change?

Comment 12

[Cristian Rodriguez (:crdlc)]

I suggest that Borja will review it as well because he implemented it

Comment 13

[Jean Gong :jgong]

PM triage this bug.  This should stay 1.4+.

Comment 14

[Gregor Wagner [:gwagner]]

Review ping

Comment 15

[Ted Clancy [:tedders1]]

Comment on attachment 8388923 [details] [review]
Bug 970303 fix - Part 2

I'm removing the Gaia part of this patch and marking this bug as dependent on Bug 981938. Bug 981938 will fix the underlying race condition in the database.

Comment 16

[Gregor Wagner [:gwagner]]

Can we land this patch and close this bug?

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

iirc this broke tests

Comment 18

[Ted Clancy [:tedders1]]

Attachment: bug-970303-fix-part1.patch

Comment 19

[Ted Clancy [:tedders1]]

Attachment: Bug 970303 - part 2: Remove undefined downloads from downloads list.

Comment 20

[Ted Clancy [:tedders1]]

Okay, new patches are attached. 

The gecko patch passes tests: https://tbpl.mozilla.org/?tree=Try&rev=c84b03ff1244

The gaia patch is a work-around for Bug 981938.

Comment 21

[Borja Salguero [:borjasalguero]]

Comment on attachment 8396620 [details] [review]
Bug 970303 - part 2: Remove undefined downloads from downloads list.

Hi Ted,
This is a workaround for an issue that is not tied with the Gaia Code (at least not with the rendering part). First of all we need to identify why there are 'downloads' returning 'undefined'.

On the other hand, as a tip, you could use
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/filter
instead of the 'for' loop. 

However, as I said, I would like not to add a workaround in Gaia which does not let us to identify the real issue.

Comment 22

[Ted Clancy [:tedders1]]

Okay, forget the gaia patch. I'll mark this bug as depending on Bug 981938 again.

The gecko patch still needs a review.

Comment 23

[Ted Clancy [:tedders1]]

Comment on attachment 8396620 [details] [review]
Bug 970303 - part 2: Remove undefined downloads from downloads list.

>https://github.com/mozilla-b2g/gaia/pull/17597

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8396618 [details] [diff] [review]
bug-970303-fix-part1.patch

As discussed yesterday this isn't right.

Comment 25

[Ted Clancy [:tedders1]]

Attachment: bug-970303-fix-part1.patch

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8398846 [details] [diff] [review]
bug-970303-fix-part1.patch

Review of attachment 8398846 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome! Thanks for digging through all this. r=me with these comments addressed:

::: dom/indexedDB/IDBDatabase.cpp
@@ +275,5 @@
>  IDBDatabase::Invalidate()
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>  
> +  InvalidateInternal(false);

Nit: Please make this:

  InvalidateInternal(/* aIsDead */ false);

Same for the other call site. That way it's more obvious what the bool means.

@@ +301,5 @@
>      QuotaManager::CancelPromptsForWindow(owner);
>    }
>  
> +  if (!aIsDead) {
> +    DatabaseInfo::Remove(mDatabaseId);

Nit: Let's add a comment here explaining the reasoning. We want to forcefully remove in the child when the parent has invalidated us in IPC mode because the database might no longer exist. We don't want to forcefully remove in the parent when a child dies since other child processes may be using the referenced DatabaseInfo.

Comment 27

[Ted Clancy [:tedders1]]

Attachment: bug-970303-fix.patch

Comment 28

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8401643 [details] [diff] [review]
bug-970303-fix.patch

Review of attachment 8401643 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +566,5 @@
>  IndexedDBDatabaseParent::ActorDestroy(ActorDestroyReason aWhy)
>  {
>    if (mDatabase) {
>      mDatabase->SetActor(static_cast<IndexedDBDatabaseParent*>(nullptr));
> +    mDatabase->InvalidateInternal(true);

Nit: Add the /* aIsDead */ here too.

Comment 29

[Ted Clancy [:tedders1]]

Attachment: bug-970303-fix.patch

Addressing Ben's nit.

Comment 30

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8403026 [details] [diff] [review]
bug-970303-fix.patch

Review of attachment 8403026 [details] [diff] [review]:
-----------------------------------------------------------------

Great! Thanks.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c8388e3bac74

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c8388e3bac74

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fcb53386d49e

Comment 34

[rafael.marquez]

Bug verified in master(2.0) branch