1240710 - First download may fail even if Storage permission has been granted

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

When a download is started and the user grants the Storage permission then this first download might still fail even though we waited for the permission resolution before performing the download.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Scenario: Fresh install. Storage permission not granted. Starting first download. Permission prompt comes up. Permission is accepted. Download failed. All subsequent downloads run successfully.

Logging the exception in DownloadCore.jsm here:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadCore.jsm#502

I get the following error:
> File error: Access denied"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)

I wonder how this happens because we block this task earlier and wait for the permission to be accepted or denied. Maybe there's some other task trying to access this file and our download is marked as failed before/while we are trying to resolve the permission?

@Paolo: Do you have an idea how this can happen?

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Dumping the stack trace on failure:
> Download failed with error: DownloadError: [Exception... "File error: Access denied"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS 
> frame :: resource://gre/modules/DownloadCore.jsm :: this.DownloadError :: line 1523"  data: no]
>    0: this.DownloadError@resource://gre/modules/DownloadCore.jsm:1558:16
>    1: DLS_onTransferFinished@resource://gre/modules/DownloadCore.jsm:2433:33
>    2: DLT_OSC_onDownload@jar:jar:file:///data/app/org.mozilla.fennec_sebastian-2/base.apk!/assets/omni.ja!/components/DownloadLegacy.js:176:9
>    3: Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:933:23
>    4: this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:812:7
>    5: this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:11

Comment 3

[:Paolo Amadini]

(In reply to Sebastian Kaspari (:sebastian) from comment #1)
> I get the following error:
> > File error: Access denied"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)
> 
> I wonder how this happens because we block this task earlier and wait for
> the permission to be accepted or denied. Maybe there's some other task
> trying to access this file and our download is marked as failed before/while
> we are trying to resolve the permission?

We actually start creating the ".part" file before we show any download progress, so if the operating system is actually denying access to the target directory until the permission is confirmed by the user, we would incur in this issue.

If we have access to a temporary folder in the meantime, we could configure the download to go to a temporary folder first. However, we might still attempt to move the file before the download has finished, unless we wait for the permission in the code path that on Desktop is used to choose a target folder, even before the entire Downloads.jsm code path is invoked.

For the moment you can take a look at these files for experimenting with different download destinations:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#462
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#1352

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Thanks for the input Paolo! The app directory is writable even without the Storage permission. I was trying to return the app's cache directory (inside the app directory) in getTemporaryDownloadsDirectory(), but after further investigation it seems like getTemporaryDownloadsDirectory() is actually never called.

Comment 5

[:Paolo Amadini]

I think the actual code path in your case might be this one:

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#275

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to :Paolo Amadini from comment #5)
> I think the actual code path in your case might be this one:
> 
> http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/
> nsExternalHelperAppService.cpp#275

I hard-coded my app's cache directory here (/data/user/0/org.mozilla.fennec_sebastian/cache) but this seems to break the download code completely without any indication what's going wrong. I don't see the JavaScript code being called and there's no download added/started. Can you help me, Paolo? Unfortunately I do not have C++ experience. (Btw. I'll be in London 2/15 - 2/19 if this is easier to debug in person).

Comment 7

[:Paolo Amadini]

(In reply to Sebastian Kaspari (:sebastian) from comment #6)
> (Btw. I'll be in London 2/15 - 2/19 if this is easier to debug
> in person).

It will definitely be easier if we could work on an Android build together. Whether it works for you depends on your timeline, unfortunately I'll not be able to take a closer look before next week anyways.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: MozReview Request: Bug 1240710 - Pick (temporary) download directory depending on whether permission has been granted. r?nalexander,paolo

As soon as the user clicks on a link to download a file Gecko will start the download - even before prompting the
user. This led to problems when the user hadn't granted the permission to write to the downloads directory yet. The
download would fail even though the user (later) accepted the permission.

With this patch we will start the download to the app's cache directory (only if we do not have the permission) and
prompt the user. As soon as the user has accepted the permission the download will be moved to the public downloads
directory (even while still downloading). If the permission is denied the download will be cancelled.

After the permission has been granted all subsequent downloads will start writing to the downloads directory
directly.

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

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

@Paolo: I guess after this patch has landed I can remove the permission code I added in bug 1216537:
https://hg.mozilla.org/mozilla-central/rev/7f2434e93076

With the changes here the code in DownloadCore.jsm should only run if we have the permission, right?

Comment 10

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Comment on attachment 8724216 [details]
MozReview Request: Bug 1240710 - Pick (temporary) download directory depending on whether permission has been granted. r?nalexander,paolo

https://reviewboard.mozilla.org/r/35475/#review33819

Fine by me.  I have a question and a partial redirect.

::: mobile/android/components/HelperAppDialog.js:248
(Diff revision 1)
> +          // If we do have the STORAGE permission then pick the public downloads directory as destination

I know this is scope bloat, but can we think of a better user experience here?  That is, if I tap away from the permission propmt (by accident?), I lose my file.  (Right?)  Can we try not kill it quite so quickly?

::: widget/android/GeneratedJNIWrappers.h:93
(Diff revision 1)
> +    struct GetTemporaryDownloadDirectory_t {

Flag jchen for these generated bits -- I don't know anything about this and assume it's automatic, but better safe.

Comment 11

[:Paolo Amadini]

Comment on attachment 8724216 [details]
MozReview Request: Bug 1240710 - Pick (temporary) download directory depending on whether permission has been granted. r?nalexander,paolo

https://reviewboard.mozilla.org/r/35475/#review34109

r+ on the JavaScript bits and the general logic, but please re-review specifically the C++/Java interaction indicated by the two questions below.

::: uriloader/exthandler/nsExternalHelperAppService.cpp:385
(Diff revision 1)
> +  auto downloadDir = widget::DownloadsIntegration::GetTemporaryDownloadDirectory();

I'm not familiar with the JNI return type here (looks like it's mozilla::jni::String::LocalRef). What happens if the Java code throws an exception? Is this a pointer that can be null, or do you have to use some other check?

::: uriloader/exthandler/nsExternalHelperAppService.cpp:390
(Diff revision 1)
> -    rv = NS_NewNativeLocalFile(nsDependentCString(downloadDir),
> +    rv = NS_NewNativeLocalFile(downloadDir->ToCString(),

What happens with this conversion method if the returned path contains non-ASCII characters? Is this even possible?

Comment 12

[:Paolo Amadini]

(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> With the changes here the code in DownloadCore.jsm should only run if we
> have the permission, right?

If the download is initiated through one of the Downloads.jsm methods instead of the DownloadLegacy path, we still have to ask for the permission at that point.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8724216 [details]
MozReview Request: Bug 1240710 - Pick (temporary) download directory depending on whether permission has been granted. r?nalexander,paolo

Additionally flagging jchen, especially to review the JNI/C++ bits.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Nick Alexander :nalexander from comment #10)
> I know this is scope bloat, but can we think of a better user experience
> here?  That is, if I tap away from the permission propmt (by accident?), I
> lose my file.  (Right?)  Can we try not kill it quite so quickly?

Just to be clear: The UX is not changed by this patch. The user is prompted right after clicking the download link (but now the download does not fail silently in the background before the user has made a decision).

Tapping the permission prompt away is actually quite difficult: Clicking outside does not close the dialog. Nor does pressing the back button. The user HAS to click "Deny" or "Allow". Of course this is still something that can happen accidentally but I don't think the result is disastrous: The failed download is added to about:downloads and can be retriggered from there or from the website you are currently on.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

@jchen: Can you also look at the questions in comment 11? I'm actually not sure how our JNI bridge will behave here. I modeled this part after already existing code in the tree.

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8724216 [details]
MozReview Request: Bug 1240710 - Pick (temporary) download directory depending on whether permission has been granted. r?nalexander,paolo

https://reviewboard.mozilla.org/r/35475/#review34467

r+ for the JNI bits

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to :Paolo Amadini from comment #11)
> Comment on attachment 8724216 [details]
> MozReview Request: Bug 1240710 - Pick (temporary) download directory
> depending on whether permission has been granted. r?nalexander,paolo
> 
> https://reviewboard.mozilla.org/r/35475/#review34109
> 
> r+ on the JavaScript bits and the general logic, but please re-review
> specifically the C++/Java interaction indicated by the two questions below.
> 
> ::: uriloader/exthandler/nsExternalHelperAppService.cpp:385
> (Diff revision 1)
> > +  auto downloadDir = widget::DownloadsIntegration::GetTemporaryDownloadDirectory();
> 
> I'm not familiar with the JNI return type here (looks like it's
> mozilla::jni::String::LocalRef). What happens if the Java code throws an
> exception? Is this a pointer that can be null, or do you have to use some
> other check?

We will crash if an exception is thrown (same as if there's an unhandled exception in Java).

> ::: uriloader/exthandler/nsExternalHelperAppService.cpp:390
> (Diff revision 1)
> > -    rv = NS_NewNativeLocalFile(nsDependentCString(downloadDir),
> > +    rv = NS_NewNativeLocalFile(downloadDir->ToCString(),
> 
> What happens with this conversion method if the returned path contains
> non-ASCII characters? Is this even possible?

ToCString() returns a UTF-8 string so non-ASCII characters in the file system should be okay.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/129e9b9ce25be48c2544406be1d0e9e9ff33b51b
Bug 1240710 - Pick (temporary) download directory depending on whether permission has been granted. r=nalexander,paolo,jchen

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8724216 [details]
MozReview Request: Bug 1240710 - Pick (temporary) download directory depending on whether permission has been granted. r?nalexander,paolo

(This needs to be uplifted to 46)

Approval Request Comment

[Feature/regressing bug #]: We introduced support for Android runtime permissions in Firefox 46 - See meta bug 1212830 

[User impact if declined]: The first download, where we ask for the STORAGE permission, will always fail, even after the user accepted the permission.

[Describe test coverage new/current, TreeHerder]: Local testing on a Nexus 6P. Runtime permissions can't be tested in automation.

[Risks and why]: This code touches the download code. So there is a risk of breaking downloads. However after testing this I do not really expect issues there. 

Without the permission and with this patch we start the download to the app's cache directory, because this does not require a permission. After accepting the permission (and from there on for all subsequent downloads) the (running) download is moved to the downloads directory. This code and approach is already used by desktop - so I expect it to be stable. Nevertheless this is the first time we download to the cache directory and maybe this will lead to issues on some devices.

We could wait until this has more usage in Nightly/Aurora. But 46 is going to be Beta next week and we should ship this with the other permission patches.

[String/UUID change made/needed]: -

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/129e9b9ce25b

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8724216 [details]
MozReview Request: Bug 1240710 - Pick (temporary) download directory depending on whether permission has been granted. r?nalexander,paolo

Fix for download permissions issue. 
Some risk here of breaking android downloads. 
Let's take it in beta 1 and test more widely.

Comment 22

[Carsten Book [:Tomcat]]

has problems to apply:

grafting 332161:129e9b9ce25b "Bug 1240710 - Pick (temporary) download directory depending on whether permission has been granted. r=nalexander,paolo,jchen"
merging mobile/android/components/HelperAppDialog.js
merging uriloader/exthandler/nsExternalHelperAppService.cpp
merging widget/android/GeneratedJNIWrappers.cpp
merging widget/android/GeneratedJNIWrappers.h
warning: conflicts while merging widget/android/GeneratedJNIWrappers.h! (edit, then use 'hg resolve --mark')

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1240710-AURORA-download-permission.patch

This is the updated patch for Aurora. Unfortunately it does not compile:

> 0:13.35 /Users/sebastian/Projects/Mozilla/aurora/uriloader/exthandler/nsExternalHelperAppService.cpp:390:45: error: 'class mozilla::jni::TypedObject<_jstring*>' has no member named 'ToCString'
> 0:13.35      rv = NS_NewNativeLocalFile(downloadDir->ToCString(),

@jchen: Do you know why this does not compile on Aurora? Have there been some JNI changes recently?

Comment 24

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Sebastian Kaspari (:sebastian) from comment #23)
> Created attachment 8727356 [details] [diff] [review]
> 1240710-AURORA-download-permission.patch
> 
> This is the updated patch for Aurora. Unfortunately it does not compile:
> 
> > 0:13.35 /Users/sebastian/Projects/Mozilla/aurora/uriloader/exthandler/nsExternalHelperAppService.cpp:390:45: error: 'class mozilla::jni::TypedObject<_jstring*>' has no member named 'ToCString'
> > 0:13.35      rv = NS_NewNativeLocalFile(downloadDir->ToCString(),
> 
> @jchen: Do you know why this does not compile on Aurora? Have there been
> some JNI changes recently?

Right, you should use 'nsCString(downloadDir)' instead of 'downloadDir->ToCString()' on Aurora.

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Jim Chen [:jchen] [:darchons] from comment #24)
> Right, you should use 'nsCString(downloadDir)' instead of
> 'downloadDir->ToCString()' on Aurora.

Thank you! :)

Comment 26

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 1240710-BETA-download-permission.patch

This is the updated, working patch. However this needs to be applied to beta now after merge day.

Comment 27

[Carsten Book [:Tomcat]]

lizzard, can this have beta approval please ?

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Yes, but i already built beta 1 so I was waiting for beta 1 to release before starting the next batch of approvals

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8724216 [details]
MozReview Request: Bug 1240710 - Pick (temporary) download directory depending on whether permission has been granted. r?nalexander,paolo

Updating flags: We got aurora approval (46) but now after merge day that's actually beta (46). See comment 19.

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8724216 [details]
MozReview Request: Bug 1240710 - Pick (temporary) download directory depending on whether permission has been granted. r?nalexander,paolo

We want downloads to work, let's uplift this beta.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a466f1fd87d8

Comment 32

[Chenxia Liu [:liuche]]

I'm seeing this bug in a fresh pull from fx-team, 4/14 - in trying to download an image, Android prompts for permission while the download animation is active in the notification bar, but after clicking "Allow", no download happens.

Not sure if all of this is relevant:

04-14 16:08:21.431 8558-8588/org.mozilla.fennec_liuche E/GeckoConsole: [JavaScript Error: "DEPRECATION WARNING: saveImageURL should be passed the private state of the containing window.
                                                                       You may find more details about this deprecation at: https://bugzilla.mozilla.org/show_bug.cgi?id=1243643
                                                                       chrome://global/content/contentAreaUtils.js 152 saveImageURL
                                                                       chrome://browser/content/browser.js 880 BrowserApp.initContextMenu/<
                                                                       chrome://browser/content/browser.js 7326 ContextMenuItem.prototype.callback
                                                                       chrome://browser/content/browser.js 2848 NativeWindow.contextmenus._promptDone
                                                                       resource://gre/modules/Prompt.jsm 178 Prompt.prototype._innerShow/<
                                                                       " {file: "resource://gre/modules/Deprecated.jsm" line: 79}]
                                                                       this.Deprecated.warning@resource://gre/modules/Deprecated.jsm:79:5
                                                                       saveImageURL@chrome://global/content/contentAreaUtils.js:152:5
                                                                       BrowserApp.initContextMenu/<@chrome://browser/content/browser.js:880:9
                                                                       ContextMenuItem.prototype.callback@chrome://browser/content/browser.js:7326:5
                                                                       NativeWindow.contextmenus._promptDone@chrome://browser/content/browser.js:2848:11
                                                                       Prompt.prototype._innerShow/
04-14 16:08:21.560 8558-8583/org.mozilla.fennec_liuche D/GeckoBrowserProvider: Expiring history.
04-14 16:08:21.562 8558-8583/org.mozilla.fennec_liuche D/GeckoBrowserProvider: Expiring thumbnails.
04-14 16:08:22.093 8558-8588/org.mozilla.fennec_liuche I/Gecko: -*- nsDNSServiceDiscovery.js : startDiscovery
04-14 16:08:27.078 8558-8558/org.mozilla.fennec_liuche W/GeckoBatteryManager: Already started!
04-14 16:08:27.094 8558-8583/org.mozilla.fennec_liuche D/GeckoSessInfo: Recording start of session: 1460675307094
04-14 16:08:27.109 8558-8588/org.mozilla.fennec_liuche D/GeckoScreenOrientation: unlocking
04-14 16:08:27.177 8558-8588/org.mozilla.fennec_liuche I/GeckoNotificationHelper: Send {"id":"{21942233-a3b9-43ff-b0e8-1ec7b7d2ead0}","handlerKey":"downloads","cookie":"\"\/storage\/emulated\/0\/Download\/th(2).jpghttps:\/\/sp.yimg.com\/xj\/th?id=OIP.Mad4d2e5668a6f3c0b1a30c380f42ae85H0&pid=15.1&P=0&w=300&h=300Thu Apr 14 2016 16:08:21 GMT-0700 (PDT)\"","eventType":"notification-closed"}
04-14 16:08:27.180 8558-8588/org.mozilla.fennec_liuche I/GeckoConsole: Notification:Event {"id":"{21942233-a3b9-43ff-b0e8-1ec7b

Comment 33

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Chenxia Liu [:liuche] from comment #32)
> I'm seeing this bug in a fresh pull from fx-team, 4/14 - in trying to
> download an image, Android prompts for permission while the download
> animation is active in the notification bar, but after clicking "Allow", no
> download happens.

Ah, great you found this. This seems to be a special case. I moved this to bug 1264869. It seems to be a different cause and the fix here is still working.

Comment 34

[Mira Lobontiu (Android QA)]

Hello,

Seeing that this flag was set a very long time ago, I will remove the qe-verify+ flag.

Thank you!