Closed Bug 981938 Opened 6 years ago Closed 6 years ago

[Download Manager] After downloading multiple items at once, not all items appear in Downloads list

Categories

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

x86
macOS
defect
Not set

Tracking

(blocking-b2g:1.4+, b2g-v1.4 fixed, b2g-v2.0 fixed)

VERIFIED FIXED
1.4 S6 (25apr)
blocking-b2g 1.4+
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: tedders1, Assigned: tedders1)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files, 13 obsolete files)

1.07 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
arthurcc
: review+
Details | Review
Steps to reproduce:

1) Open browser. Go to http://owd.tid.es/dm.
2) Tap several links in rapid succession. Wait for files to download.
3) Open Settings app, and go to Downloads screen.

Expected Result: All downloaded files are listed.
Actual Result: Only some of the downloaded files are listed.

This is due to a race condition in the Download Store (gaia/shared/js/download/download_store.js). 

When downloading a file, the Download Store fetches a list of downloads from the database, appends the newly downloaded file, and then writes the list of downloads back to the database. 

However, these database calls are performed asynchronously. The information in the database may have changed between our fetch and our write. Our write clobbers the previous change.
blocking-b2g: --- → 1.4?
Assignee: nobody → tclancy
Whiteboard: [systemsfe]
blocking-b2g: 1.4? → 1.4+
Target Milestone: --- → 1.4 S3 (14mar)
Attached patch bug-981938-fix-part1.patch (obsolete) — Splinter Review
Bug 981938 - Part 1: Adding getAllKeys method to DataStore. r=bz
Attachment #8391644 - Flags: review?(bzbarsky)
Bug 981938 - Part 2: No longer rely on the value of the index record. r=djf
Attached file Bug 981938 - Part 2 (obsolete) —
Bug 981938 - Part 2: No longer rely on the value of the index record. r=djf
Attachment #8391646 - Attachment is obsolete: true
Attachment #8391647 - Flags: review?(dflanagan)
Target Milestone: 1.4 S3 (14mar) → 1.4 S4 (28mar)
My patch relies on the getAllKeys method in IDBObjectStore. Because this API is currently hidden behind a pref, I added a synonym called mozGetAllKeys. 

I saw that this was the approach used for IDBObjectStore's mozGetAll, as well as IDBIndex's mozGetAllKeys and mozGetAll, so I decided to be consistent with that.
That pattern isn't a good one to follow, actually, unless we actively want mozGetAllKeys exposed to every single web page.  Looks like those other moz* things got added back when the non-prefixed alternatives simply didn't exist, so there were no other options...

It would make more sense to me to just unconditionally expose the API to the relevant consumers, assuming those aren't "all web pages".  Ben, thoughts?
Flags: needinfo?(bent.mozilla)
My understanding is that the non-prefixed alternatives aren't fully standardized yet, so we don't want to expose them yet in case their prescribed behaviour changes.
We don't want to expose them to the web at large, sure.  I was talking about exposing them to the consumer you have here, assuming that's not "every single web page".  Is it?

And if it is, then why is exposing prefixed stuff any better than just exposing?  Al lit means is that web pages start depending on the prefixed stuff...  This is why we're not creating new prefixed DOM features.
Just to be clear, I wouldn't usually suggest adding moz-prefixed functions. The only reason I suggested adding mozGetAllKeys in this case was because all the other getAll/getAllKeys functions had a moz-prefixed version except this one, and it seemed to me like an asymmetrical omission, as well as a potential gotcha for users.

If you're not keen on that, there are lots of other options. 

1) I could write code to expose the un-prefixed getAllKeys to certified apps, regardless of the pref settings. It's easily done, but slightly inelegant because I'd have to add a [Func=...] function. (Also, should I be using getAllKeys? Is it likely to change in a future revision after being standardized?)

2) I could add a mozGetAllKeys, but expose it only to certified apps.

3) I could avoid using getAllKeys/mozGetAllKeys completely, at the expense of making my code a bit more cumbersome.

I really have no preference. It sounds like you'd prefer number 1, is that correct?
Another option that has been suggested to me (by :sicking) is extending our webIDL syntax so that we can say "expose this function if a pref is set OR if we're a certified app". Right now, there's no way to do that in our syntax. 

:sicking suggested perhaps [ForceAvailableIn=CertifiedApps], which would override the [Pref=...] setting. If that was available, I could simply add [ForceAvailableIn=CertifiedApps] to getAllKeys, and use getAllKeys. 

(In case I didn't mention it, the app I'm working on is a certified app.)
I prefer #1, but I'd like to get Ben's feedback on it (and in particular, answers to your questions there).

We could certainly add a ForceAvailableIn, yes, if we decide to do #1.
Blocks: 970303
I'm not entirely sure what's being asked here, but here's my best guess:

Boris is right that we had moz* props on indexedDB because at the time that was supposed to be what one did when exposing non-standardized features. That thinking has since changed.

getAll/getAllKeys/openKeyCursor are planned for v2 of the spec but are not yet standardized so they're now hidden behind that experimental pref. Once they're enabled by default we'll have to deprecate the moz* versions and then eventually remove them.

So going forward I guess we should expose the unprefixed props to certified apps, in effect making all certified apps use experimental idb features. It's possible that the v2 spec will require slight changes to them but that's just the risk you take using them I guess. We should not add any further moz* things.
Flags: needinfo?(bent.mozilla)
Comment on attachment 8391644 [details] [diff] [review]
bug-981938-fix-part1.patch

Let's do what Ben says.
Attachment #8391644 - Flags: review?(bzbarsky) → review-
No longer blocks: 970303
Blocks: 970303
I tried exposing getAllKeys() to certified apps, but that doesn't work. As far as I can tell, the problem is that the Datastore operates in its own process which is not a certified app.

I'll talk to Ben about alternatives.
Attached file Bug 981938 - Part 2 (obsolete) —
(New pull request, because I accidentally clobbered my old Github fork.)
Attachment #8391647 - Attachment is obsolete: true
Attachment #8391647 - Flags: review?(dflanagan)
Attachment #8397222 - Flags: review?(arthur.chen)
Note that you can expose based on other things too (like the URL and whatnot, if it comes to that).
Attachment #8397222 - Attachment is obsolete: true
Attachment #8397222 - Flags: review?(arthur.chen)
Attachment #8391644 - Attachment is obsolete: true
Attached patch bug-981938-fix-part1.patch (obsolete) — Splinter Review
Decided in the end to just use a cursor. Has the disadvantage of being slow, unfortunately.
Attachment #8397524 - Flags: review?(bent.mozilla)
Attached file Bug 981938 - Part 2 (obsolete) —
Attachment #8397525 - Flags: review?(arthur.chen)
Did the lazy-import idea not pan out?
I tried it. It didn't work.
Attached patch Stab in the dark, v1 (obsolete) — Splinter Review
Hi Ted, please give this a try and see if it somehow fixes things? It applies on top of that patch you had to make the getAllKeys() function check the app permissions.
Attachment #8398219 - Flags: feedback?(tclancy)
Hi Ted, I am still working on it. I think I need more time as I am not familiar with the original code base. Will finish the review as soon as possible.
Target Milestone: 1.4 S4 (28mar) → 1.4 S5 (11apr)
Attached patch permissions-check.patch (obsolete) — Splinter Review
This patch is just for discussion purposes.
Hey Ben. Your stab in the dark doesn't seem to work for me. I see the following error at boot time:

E/GeckoConsole(  108): [JavaScript Error: "indexedDB is undefined" {file: "resource://gre/modules/IndexedDBHelper.jsm" line: 55}]

For reference purposes, I attached the patch that I used to make getAllKeys() check the app privileges. It's named permissions-check.patch.
Attachment #8398219 - Flags: feedback?(tclancy)
Comment on attachment 8397525 [details] [review]
Bug 981938 - Part 2

Hi ted, could you explain more about your patch? I can't understand how it works.
Comment on attachment 8397524 [details] [diff] [review]
bug-981938-fix-part1.patch

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

Canceling review for now, we've been discussing this offline and progress is being made towards the better fix.
Attachment #8397524 - Flags: review?(bent.mozilla)
Attached patch bug-981938-fix-part-1.patch (obsolete) — Splinter Review
Attachment #8397524 - Attachment is obsolete: true
Attachment #8401644 - Flags: review?(bent.mozilla)
Attachment #8401646 - Flags: review?(bent.mozilla)
Attached patch bug-981938-fix-part-4.patch (obsolete) — Splinter Review
Attachment #8401647 - Flags: review?(bobbyholley)
Attachment #8398219 - Attachment is obsolete: true
Attachment #8399520 - Attachment is obsolete: true
Arthur, I've revised the code in response to your review. I've factored the duplicated code. (And you're right, the argument to getAllKeys() was a mistake, although it didn't affect execution.) I'll put further comments in Github.
Comment on attachment 8401647 [details] [diff] [review]
bug-981938-fix-part-4.patch

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

Er, what? Can you explain what you're trying to do? I'm pretty sure that this is not the right solution.
Attachment #8401647 - Flags: review?(bobbyholley) → review-
Comment on attachment 8401647 [details] [diff] [review]
bug-981938-fix-part-4.patch

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

Hi Bobby.

Chrome modules (like Datastore) use a singleton of type nsSystemPrincipal as their principal.

In the Webidl files, some API functions are protected with "AvailableIn=CertifiedApps". That mechanism calls nsPrincipal::GetAppStatus() and checks for a return value of APP_STATUS_CERTIFIED.

My intention with this patch was to make those protected API functions available to Chrome modules, since I'd like those modules to have access to the protected functions.

Does that make sense?
Typically such APIs are tied to a particular window/global.

AvailableIn will make the API exposed on all objects that are in a global that is part of a certified app.

It sounds like in your case the global is actually chrome, not part of an app at all?  It may be that the right thing is to change the behavior of AvailableIn or to change the condition used on this interface...
Thanks for your feedback Boris. I'll change the condition used on this interface.
Attached patch bug-981938-fix-part-1.patch (obsolete) — Splinter Review
Attachment #8401644 - Attachment is obsolete: true
Attachment #8401647 - Attachment is obsolete: true
Attachment #8401644 - Flags: review?(bent.mozilla)
Attachment #8402094 - Flags: review?(bent.mozilla)
Ted, thanks for the explanation. I add a comment in github.
Attached patch bug-981938-fix-part-1.patch (obsolete) — Splinter Review
Oops, I accidentally a file.
Attachment #8402094 - Attachment is obsolete: true
Attachment #8402094 - Flags: review?(bent.mozilla)
Attachment #8402767 - Flags: review?(bent.mozilla)
Attachment #8401646 - Flags: review?(bent.mozilla) → review+
Ted, firstly, if you want to add a new method to DataStore, please first write a proposal including the use case you're trying to address and send it dev-webapi.  We're trying to standardize this API so we should not add random methods without any coordination with the people working on the API.

Also, reading comment 0, it seems like you're just asking for transactional semantics to avoid clobbering values.  I just r+ed a patch on bug 963038 which does that.  With that, do we need to do anything here?
Flags: needinfo?(tclancy)
Comment on attachment 8402767 [details] [diff] [review]
bug-981938-fix-part-1.patch

r- until we at least agree that this is a good idea.  :-)
Attachment #8402767 - Flags: review?(bent.mozilla) → review-
Attached patch bug-981938-fix-part-1.patch (obsolete) — Splinter Review
I'm attaching a new patch with TWO reviewers.

Ben's review is for correctness of the code.

Ehsan's review is for whether we want to add getAllKeys() to the DataStore (which is yet to be determined).
Attachment #8402767 - Attachment is obsolete: true
Attachment #8402983 - Flags: review?(ehsan)
Attachment #8402983 - Flags: review?(bent.mozilla)
Flags: needinfo?(tclancy)
Ted, so following up on our quick meeting today, did we conclude that this is the only way to fix the 1.4 blocker?  If yes, then I'd be OK with this as long as you file a 1.5 blocker bug to remove this API and fix the settings app in a way that makes us not need to maintain getAllKeys() here.  I'm still not convinced that we would want to support this API in DataStore.
Flags: needinfo?(tclancy)
Also note that this patch makes the IDB getAllKeys API available to certified apps on b2g.  I'm not really sure why that's needed in order to fix the issue at hand here.
Hi Ehsan. 

> so following up on our quick meeting today, did we conclude that this is the only way to fix the 1.4 blocker?

No. As you suggested, we're going to try to use the sync() method on the Datastore to set up a parallel list of downloads in the Settings app. Ben checked with Gregor, and he said that's okay.

In the meantime, I'm going to leave these patches here (with appropriate reviews) just in case we need to fall back on them.

> Also note that this patch makes the IDB getAllKeys API available to certified apps on b2g. I'm not really sure why that's needed in order to fix the issue at hand here.

That's a good point.
Flags: needinfo?(tclancy)
Comment on attachment 8402983 [details] [diff] [review]
bug-981938-fix-part-1.patch

(In reply to comment #42)
> Hi Ehsan. 
> 
> > so following up on our quick meeting today, did we conclude that this is the only way to fix the 1.4 blocker?
> 
> No. As you suggested, we're going to try to use the sync() method on the
> Datastore to set up a parallel list of downloads in the Settings app. Ben
> checked with Gregor, and he said that's okay.

That's great!

> In the meantime, I'm going to leave these patches here (with appropriate
> reviews) just in case we need to fall back on them.

OK.  So r=me with what I requested in comment 40, but I hope that we can avoid taking this patch.  Thanks!
Attachment #8402983 - Flags: review?(ehsan) → review+
Comment on attachment 8397525 [details] [review]
Bug 981938 - Part 2

r=me with the comment addressed, thanks.
Attachment #8397525 - Flags: review?(arthur.chen) → review+
Comment on attachment 8402983 [details] [diff] [review]
bug-981938-fix-part-1.patch

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

r=me with this fixed:

::: dom/indexedDB/IDBObjectStore.cpp
@@ +65,5 @@
> +bool
> +IndexedDBExperimentalEnabled(JSContext* aCx, JSObject* aObject)
> +{
> +  return mozilla::Preferences::GetBool("dom.indexedDB.experimental") ||
> +    IsInCertifiedApp(aCx, aObject) || UsesSystemPrincipal(aCx, aObject);

Nit:

  return a ||
         b ||
         c;
Attachment #8402983 - Flags: review?(bent.mozilla) → review+
Sorry to do this to you, Arthur, but I had to change the patch. This is the solution suggested by Ehsan.

Just to recap:

The Download Store is implemented using a DataStore. The issue is how to get a list of the Download Store's keys.

* The currently existing code stores a list of keys in a special record inside the Download Store (stored under INDEX_ID). That was prone to race conditions.

* The previous patch called getAllKeys() (a new method) on the DataStore. This was vetoed by Ehsan.

* The new patch calls sync() on the DataStore to fetch a history of changes, computes the list of keys, and then stores them in a new IndexedDB called 'download_list'.
Attachment #8397525 - Attachment is obsolete: true
Attachment #8402983 - Attachment is obsolete: true
Attachment #8404926 - Flags: review?(arthur.chen)
Target Milestone: 1.4 S5 (11apr) → 1.4 S6 (25apr)
Comment on attachment 8404926 [details] [review]
Removing the meta-entry from the download store. Using sync() to get list of keys.

I guess I need more clarifications. Please check my comments in github, thanks.
Attachment #8404926 - Flags: review?(arthur.chen)
Comment on attachment 8404926 [details] [review]
Removing the meta-entry from the download store. Using sync() to get list of keys.

Hi Arthur. I responded to your comments on GitHub, and adjusted the code as requested.
Attachment #8404926 - Flags: review?(arthur.chen)
Comment on attachment 8404926 [details] [review]
Removing the meta-entry from the download store. Using sync() to get list of keys.

r=me with the comment addressed, thanks!
Attachment #8404926 - Flags: review?(arthur.chen) → review+
Keywords: checkin-needed
master: 15b79c11c3e16d53b45d121ebb54e37a9e3d8f5b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Bug verified in master(2.0) branch
Status: RESOLVED → VERIFIED
No longer blocks: 1180250
Depends on: 1180250
You need to log in before you can comment on or make changes to this bug.