Closed Bug 970303 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86_64
Windows 7
defect
Not set

Tracking

(blocking-b2g:1.4+, firefox29 wontfix, firefox30 fixed, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

VERIFIED FIXED
1.4 S5 (11apr)
blocking-b2g 1.4+
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: rafael.marquez, Assigned: tedders1)

References

Details

(Whiteboard: [systemsfe][p=3])

Attachments

(2 files, 6 obsolete files)

*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
blocking-b2g: --- → 1.4?
Whiteboard: [systemsfe]
blocking-b2g: 1.4? → 1.4+
Assignee: nobody → tclancy
Status: NEW → ASSIGNED
Target Milestone: --- → 1.4 S3 (14mar)
Whiteboard: [systemsfe] → [systemsfe][p=3]
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
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.
Resolution: WORKSFORME → FIXED
Rafael, can you attempt to reproduce the issue using the Steps to Reproduce in Comment 1? Thanks.
Attached file another_log.rtf
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.
Should we reopen?
Oh, we can re-open things? Cool. I think that's a good idea.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attached patch bug-970303-fix-part-1.patch (obsolete) — Splinter Review
Attachment #8388921 - Flags: review?(bent.mozilla)
Attached file Bug 970303 fix - Part 2 (obsolete) —
Attachment #8388923 - Flags: review?(21)
Comment on attachment 8388923 [details] [review]
Bug 970303 fix - Part 2

Cristian is likely a better fit for this part of the code :)
Attachment #8388923 - Flags: review?(21) → review?(crdlc)
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?
Attachment #8388921 - Flags: review?(bent.mozilla) → review+
I suggest that Borja will review it as well because he implemented it
Flags: needinfo?(borja.bugzilla)
Attachment #8388923 - Flags: review?(crdlc) → review?(borja.bugzilla)
PM triage this bug.  This should stay 1.4+.
Review ping
Target Milestone: 1.4 S3 (14mar) → 1.4 S4 (28mar)
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.
Attachment #8388923 - Attachment is obsolete: true
Attachment #8388923 - Flags: review?(borja.bugzilla)
Flags: needinfo?(borja.bugzilla)
Depends on: 981938
Can we land this patch and close this bug?
Attached patch bug-970303-fix-part1.patch (obsolete) — Splinter Review
Attachment #8388921 - Attachment is obsolete: true
Attachment #8396618 - Flags: review?(bent.mozilla)
Attachment #8396620 - Flags: review?(borja.bugzilla)
No longer depends on: 981938
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 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.
Attachment #8396620 - Flags: review?(borja.bugzilla) → review-
Blocks: 982844
Okay, forget the gaia patch. I'll mark this bug as depending on Bug 981938 again.

The gecko patch still needs a review.
Depends on: 981938
Comment on attachment 8396620 [details] [review]
Bug 970303 - part 2: Remove undefined downloads from downloads list.

>https://github.com/mozilla-b2g/gaia/pull/17597
Attachment #8396620 - Attachment is obsolete: true
Comment on attachment 8396618 [details] [diff] [review]
bug-970303-fix-part1.patch

As discussed yesterday this isn't right.
Attachment #8396618 - Flags: review?(bent.mozilla) → review-
Attached patch bug-970303-fix-part1.patch (obsolete) — Splinter Review
Attachment #8396618 - Attachment is obsolete: true
Attachment #8398846 - Flags: review?(bent.mozilla)
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.
Attachment #8398846 - Flags: review?(bent.mozilla) → review+
Target Milestone: 1.4 S4 (28mar) → 1.4 S5 (11apr)
Attached patch bug-970303-fix.patch (obsolete) — Splinter Review
Attachment #8398846 - Attachment is obsolete: true
Attachment #8401643 - Flags: review?(bent.mozilla)
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.
Attachment #8401643 - Flags: review?(bent.mozilla) → review+
Addressing Ben's nit.
Attachment #8401643 - Attachment is obsolete: true
Attachment #8403026 - Flags: review?(bent.mozilla)
Comment on attachment 8403026 [details] [diff] [review]
bug-970303-fix.patch

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

Great! Thanks.
Attachment #8403026 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/c8388e3bac74
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Bug verified in master(2.0) branch
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.