Closed Bug 1216537 Opened 4 years ago Closed 4 years ago

Request Storage permission at runtime

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

No description provided.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Attachment #8703568 - Flags: review?(nalexander)
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.
Depends on: 1237576
No longer depends on: 1237576
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/
Attachment #8703568 - Attachment description: MozReview Request: Bug 1216537 - Check and request storage permission if file download is started. r? → MozReview Request: Bug 1216537 - Check and request storage permission if file download is started. r?nalexander
Attachment #8703568 - Flags: review?(paolo.mozmail)
Attachment #8703568 - Flags: review?(paolo.mozmail)
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.
Attachment #8703568 - Attachment description: MozReview Request: Bug 1216537 - Check and request storage permission if file download is started. r?nalexander → MozReview Request: Bug 1216537 - Check and request storage permission if file download is started. r?nalexander,paolo
Attachment #8703568 - Flags: review?(paolo.mozmail)
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/
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 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.
Attachment #8703568 - Flags: review?(paolo.mozmail) → review-
Attachment #8703568 - Flags: review- → review?(paolo.mozmail)
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/
(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.
@paolo: How can I run common_test_Download.js locally? Do I need a full desktop build for that?
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".
Attachment #8708402 - Flags: review?(paolo.mozmail)
Attachment #8708402 - Flags: review?(nalexander)
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.
Attachment #8703568 - Flags: review?(paolo.mozmail) → review+
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?
Attachment #8708402 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/7f2434e93076d5141d7761dcbada0d5378fce116
Bug 1216537 - Check and request storage permission if file download is started. r=nalexander,paolo
Depends on: 1240700
(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.
Depends on: 1240710
https://hg.mozilla.org/mozilla-central/rev/7f2434e93076
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Depends on: 1241887
You need to log in before you can comment on or make changes to this bug.