1216537 - Request Storage permission at runtime

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1216537 - Check and request storage permission if file download is started. r?nalexander,paolo

Review commit: https://reviewboard.mozilla.org/r/29399/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29399/

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8703568 [details]
MozReview Request: Bug 1216537 - Check and request storage permission if file download is started. r?nalexander,paolo

https://reviewboard.mozilla.org/r/29399/#review26301

nits.  It's fine by me, but flag a Desktop download peer just in case.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:1533
(Diff revision 1)
> +  if (aProperties.becauseBlockedByPermission) {

Why is this not exclusive, like the others?  I think I'd want it to be like `becauseBlockedByReputationCheck`.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:1582
(Diff revision 1)
> +   * download files was not granted.

Explain the use case, and how it doesn't apply to all systems.

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm:71
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "RuntimePermissions",

This would fail if invoked on non-Fennec builds, since the module is only included in Fennec builds.  (It's in `mobile/android/modules` IIRC.)

I'd either not use this pattern or comment that this is Fennec only.  In fact, I'd make this conditional on `MOZ_WIDGET_ANDROID`.

Also, consider using `AppConstants` rather than the preprocessor.  I know it's not the style of this file, but let's not make the situation worse while we're here.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8703568 [details]
MozReview Request: Bug 1216537 - Check and request storage permission if file download is started. r?nalexander,paolo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29399/diff/1-2/

Comment 4

[:Paolo Amadini]

Comment on attachment 8703568 [details]
MozReview Request: Bug 1216537 - Check and request storage permission if file download is started. r?nalexander,paolo

https://reviewboard.mozilla.org/r/29399/#review27029

This needs a simple test, it doesn't need to actually call into RuntimePermissions to work, just look at what the other DownloadIntegration.shouldBlockFor tests do.

Let's spell out "RuntimePermissions" everywhere to avoid confusion with permissions granted to the individual web page.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:464
(Diff revision 2)
> +        if (yield DownloadIntegration.shouldBlockForSystemPermissions()) {

shouldBlockForRuntimePermissions

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:465
(Diff revision 2)
> +          throw new DownloadError({ becauseBlockedByPermission: true });

becauseBlockedByRuntimePermissions

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:1583
(Diff revision 2)
> +   * This does not apply to all systems. On Android this is flag is set to true

"this flag is set"

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm:514
(Diff revision 2)
> +                             .then((permissionGranted) => !permissionGranted, Cu.reportError);

No need to catch the error if it's an exceptional situation and the module does not normally reject.

nit: parentheses not needed around permissionGranted.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8703568 [details]
MozReview Request: Bug 1216537 - Check and request storage permission if file download is started. r?nalexander,paolo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29399/diff/2-3/

Comment 6

[:Paolo Amadini]

Hi Sebastian, if you need help with the test just let me know, and I can post some more detailed information on how to write it tomorrow.

Comment 7

[:Paolo Amadini]

Comment on attachment 8703568 [details]
MozReview Request: Bug 1216537 - Check and request storage permission if file download is started. r?nalexander,paolo

Hm, looks like you need to update DownloadError.toSerializable and DownloadError.fromSerializable, sorry I missed those last time. Flagging this patch as r- as a reminder to fix those:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm#1584

For the tests, let's follow the pattern of the other similar methods for consistency. At some point we'll implement a way for tests to directly override DownloadIntegration methods, but for now we're adding a conditional at the beginning of the production code.

Add the flags dontCheckRuntimePermissions and shouldBlockInTestForRuntimePermissions to the DownloadIntegration object, and set the first one here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/test/unit/head.js#793

Use the two flags at the beginning of your DownloadIntegration method, for example:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#482

Finally, copy-and-paste "test_blocked_parental_controls" and use the two new flags instead:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/test/unit/common_test_Download.js#1573

Please place the new test after test_blocked_parental_controls_httpstatus450 so that the two existing related tests stay together.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2541f92372f2

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8703568 [details]
MozReview Request: Bug 1216537 - Check and request storage permission if file download is started. r?nalexander,paolo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29399/diff/3-4/

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #8)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2541f92372f2

Mh, test_DownloadLegacy.js timed out on Linux and MacOSX. I wonder if this is related.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

@paolo: How can I run common_test_Download.js locally? Do I need a full desktop build for that?

Comment 12

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1216537 - Check and request storage permission if file download is started. r?nalexander,paolo

Review commit: https://reviewboard.mozilla.org/r/31039/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31039/

Comment 13

[:Paolo Amadini]

Comment on attachment 8708402 [details]
MozReview Request: Bug 1216537 - Check and request storage permission if file download is started. r?nalexander,paolo

Adding one more asynchronous check at the beginning of the "start" method actually uncovered a pre-existing race condition with progress reports in DownloadLegacySaver. This was not related to the new check or its associated new test.

I've updated the patch to fix this problem and edited a few nits as well.

I've pulled your commit and run "hg amend" on it, but apparently this was pushed as a new review request rather than an update. You can try to pull my updated revision and integrate it in your patch queue, maybe "hg evolve" will work, or you can try to rebase:

hg pull -r db275274bc394107a083182afe3866b9f35d91e8 https://reviewboard-hg.mozilla.org/gecko

To run the tests locally, you can execute "test_DownloadCore.js" and "test_DownloadLegacy.js", both include "common_test_Downloads.js".

Comment 14

[:Paolo Amadini]

Comment on attachment 8703568 [details]
MozReview Request: Bug 1216537 - Check and request storage permission if file download is started. r?nalexander,paolo

And this bug is ready to land once you've cross-checked my changes.

Comment 15

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8708402 [details]
MozReview Request: Bug 1216537 - Check and request storage permission if file download is started. r?nalexander,paolo

https://reviewboard.mozilla.org/r/31039/#review27881

Consider this feedback -- I expect paolo to have the final word on this.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:2341
(Diff revision 1)
> +

If I read this correctly, you're changing the behaviour to persist the progress information, allowing for a progress function set part way through a download to be notified with up-to-date information.  That seems correct, but I'll comment to make sure paolo considers this more carefully.

I find the name "wasNotified" odd, now, since the actual progress notification may not have occurred: we may early return when `!setProgressBytesFn`.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:2461
(Diff revision 1)
> +    if (this.progressWasNotified) {

Why is this addition necessary?

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/7f2434e93076d5141d7761dcbada0d5378fce116
Bug 1216537 - Check and request storage permission if file download is started. r=nalexander,paolo

Comment 17

[:Paolo Amadini]

(In reply to Nick Alexander :nalexander from comment #15)
> If I read this correctly, you're changing the behaviour to persist the
> progress information, allowing for a progress function set part way through
> a download to be notified with up-to-date information.  That seems correct,
> but I'll comment to make sure paolo considers this more carefully.

I wrote that part, and you read it correctly. We need this code because we have to link two independently, asynchronously initialized entities, the download in the front-end and the nsITransfer notifications in the back-end. We wouldn't need this DownloadLegacy connector if we could initialize Downloads.jsm before we start receiving data and meta-data from the network channel, but unfortunately for legacy downloads this can't be done at present.

> I find the name "wasNotified" odd, now, since the actual progress
> notification may not have occurred: we may early return when
> `!setProgressBytesFn`.

onProgressBytesCalled would now be a better name, but I didn't think about this. Anyways, it's not a big deal even if we don't update it, now that the patch has landed.

> ::: toolkit/components/jsdownloads/src/DownloadCore.jsm:2461
> (Diff revision 1)
> > +    if (this.progressWasNotified) {
> 
> Why is this addition necessary?

There is some code in the tests that waits for the download to reach a certain progress (50%) before allowing more data to be sent as part of the simulated network response. If we lose the progress notification, the download won't finish.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f2434e93076