1096014 - Regression: Unable to complete a download without error; downloads broken

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Download Manager, defect)

Description

[joejob357]

User Agent: Mozilla/5.0 (Android; Tablet; rv:35.0) Gecko/35.0 Firefox/35.0
Build ID: 20141107004008

Steps to reproduce:

* Install Nightly on CyanogenMod Nightly
* Download PDF, e.g. from http://cryptome.org/



Actual results:

* Animated progress bar in notification tray completes as normal.
* Firefox displays an error message on a white background entitled "Download error", with the text "The download can not be saved because an unknown error occurred. Please try again."
* There is not a new file in the Downloads directory.


Expected results:

* The file should have either opened in a PDF viewer or been saved to Downloads. (On Aurora it simply saves the file, which is fine.)

Image files download correctly, and the Page -> Save as PDF feature works, but epub files also display this problem.

Comment 1

[Bill Gianopoulos [:WG9s]]

Confirming this.  I can no longer install .apk files using Firefox.

Comment 2

[Bill Gianopoulos [:WG9s]]

I have verified via backout that this a regression form bug 1074793.

Comment 3

[Bill Gianopoulos [:WG9s]]

I have updated this to a blocker because I am unable to produce builds that can be installed form my website, yet am not willing to back out the regressor because it is a security bug.

Comment 4

[Aaron Train [:aaronmt]]

I don't have access to bug 1074793, maybe we can needinfo Magnus or Wes to comment on what's going on here on the toolkit code changes that affect Android.

Comment 5

[Bill Gianopoulos [:WG9s]]

OK well since this has been changed back to normal I am going to include the backout of the security fix in my builds.

Comment 6

[Magnus Melin [:mkmelin]]

Bug 1074793 changed the file permissions to be less restrictive. 0400 for temp files, 0600 until the file is in the user's download folder - after which system umask is applied. 

As for how that would cause problems, I don't know.

Comment 8

[Aaron Train [:aaronmt]]

I'm not sure how the above would be related either.

On download complete (I used http://nightly.mozilla.org and picked our APK)

* I/GeckoConsole(22009): Error getting pref for application/vnd.android.package-archive.

* I/GeckoNotificationHelper(22009): Send {"cookie":"\"4ci1Nh-h2bih\"","id":"{8827b85f-3c2e-4e10-b053-8abe0caec0d6}","handlerKey":"downloads","eventType":"notification-closed"}

* I/GeckoConsole(22009): Sending message

* I/GeckoConsole(22009): Notification:Event {"cookie":"\"4ci1Nh-h2bih\"","id":"{8827b85f-3c2e-4e10-b053-8abe0caec0d6}","handlerKey":"downloads","eventType":"notification-closed"}

Comment 9

[Aaron Train [:aaronmt]]

I'll grab a regression-window.

Comment 10

[Aaron Train [:aaronmt]]

Aforementioned bug is correct

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6c2fb5df16ba&tochange=[e-mail]

Fri Nov 07 19:21:23 2014 +0000	ca0a0888311b	Magnus Melin — Bug 1074793 - Set more restrictive permissions for downloads in the temporary directory. r=paolo

Comment 11

[:Paolo Amadini]

Hm, maybe we're operating on a file system that doesn't support setting permissions, and this is causing this SetPermissions call to fail:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#3364

I would simply try the following two changes to ignore errors:

if (NS_SUCCEEDED(rv)) {
  (void)target->SetPermissions(0666 & ~gUserUmask);
}
return NS_OK;

As an aside, I look forward to see bug 901360 solved so that we don't have to maintain two implementations of the downloads code. I believe this case is handled correctly by the new JavaScript API for downloads.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

I can't even run tests locally. I get a bunch of these errors:

I/GeckoConsole(25334): 1415678692267	addons.xpi	WARN	Failed to set permissions 755 on /storage/emulated/legacy/tests/profile/extensions/staged: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.permissions]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: setFilePermissions :: line 274"  data: no]

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #12)
> I can't even run tests locally. I get a bunch of these errors:

For the record, I have not determined the permission errors in comment 12 are related. Working on that.

I'll try the suggestion in comment 11 as well

Comment 14

[Magnus Melin [:mkmelin]]

(In reply to :Paolo Amadini from comment #11)
> Hm, maybe we're operating on a file system that doesn't support setting
> permissions, and this is causing this SetPermissions call to fail:

That could well be the case. I read FAT32 is default (or at least very commonly used) for SD cards on Android.

Comment 15

[:Paolo Amadini]

Attachment: Untested patch

The best I can do to help move this forward is to provide my suggestion from comment 11 in patch form. This would need to be applied and tested on an Android build, to see if the guess is correct and it solves the problem.

Comment 16

[Bill Gianopoulos [:WG9s]]

(In reply to :Paolo Amadini from comment #15)
> Created attachment 8520644 [details] [diff] [review]
> Untested patch
> 
> The best I can do to help move this forward is to provide my suggestion from
> comment 11 in patch form. This would need to be applied and tested on an
> Android build, to see if the guess is correct and it solves the problem.

This patch fixes my issue with .apk files.

Comment 17

[Bill Gianopoulos [:WG9s]]

I do have builds including this patch available at http://www.wg9s.com/mozilla/firefox/

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8520644 [details] [diff] [review]
Untested patch

This seemed to work for me too.

Comment 19

[joejob357]

(In reply to Bill Gianopoulos [:WG9s] from comment #17)
> I do have builds including this patch available at
> http://www.wg9s.com/mozilla/firefox/

This build resolves the issue for me, too.

Comment 20

[Aaron Train [:aaronmt]]

Let's land it

Comment 21

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/3c84bb853ee6

Comment 22

[:Paolo Amadini]

Landed on fx-team, thanks all for testing!

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3c84bb853ee6

Comment 24

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in Firefox for Android 36.0a1 (2014-11-18)
Device: Nexus 4 (Android 4.4.4)

Comment 25

[Magnus Melin [:mkmelin]]

Comment on attachment 8520644 [details] [diff] [review]
Untested patch

Approval Request Comment
[Risks and why]: No risk, needs to land hand in hand with bug 1074793 as this is a regression caused by that on android

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8520644 [details] [diff] [review]
Untested patch

Approving for Aurora only, this doesn't meet ESR landing criteria

Comment 27

[Carsten Book [:Tomcat]]

i guess ryan works on this today (or better later today)

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Yep, I'll get to them when I get to them today.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ca3200f1bbf