Closed Bug 1096014 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox for Android :: Download Manager, defect)

36 Branch
Other
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 36
Tracking Status
firefox35 --- fixed
firefox36 --- verified
fennec 36+ ---

People

(Reporter: joejob357, Assigned: Paolo)

References

Details

(Keywords: regression, reproducible)

Attachments

(1 file)

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.
Version: Firefox 35 → Firefox 36
Confirming this.  I can no longer install .apk files using Firefox.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Severity: normal → major
I have verified via backout that this a regression form bug 1074793.
Keywords: regression
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.
Severity: major → blocker
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.
Severity: blocker → normal
tracking-fennec: --- → ?
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(wjohnston)
Flags: needinfo?(3x2sei)
Flags: needinfo?(mkmelin+mozilla)
OK well since this has been changed back to normal I am going to include the backout of the security fix in my builds.
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.
Flags: needinfo?(mkmelin+mozilla)
Duplicate of this bug: 1096276
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"}
Flags: in-testsuite?
I'll grab a regression-window.
Keywords: reproducible
Aforementioned bug is correct

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6c2fb5df16ba&tochange=ca0a0888311bmkmelin@iki.fi

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
Flags: needinfo?(wjohnston)
Flags: needinfo?(3x2sei)
Summary: "Download error" regression for certain file types → Regression: Unable to complete a download without error; downloads broken
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.
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]
(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
(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.
Attached patch Untested patchSplinter Review
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.
Attachment #8520644 - Flags: review?(mark.finkle)
(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.
I do have builds including this patch available at http://www.wg9s.com/mozilla/firefox/
Comment on attachment 8520644 [details] [diff] [review]
Untested patch

This seemed to work for me too.
Attachment #8520644 - Flags: review?(mark.finkle) → review+
(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.
Let's land it
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Landed on fx-team, thanks all for testing!
tracking-fennec: ? → 36+
https://hg.mozilla.org/mozilla-central/rev/3c84bb853ee6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Verified as fixed in Firefox for Android 36.0a1 (2014-11-18)
Device: Nexus 4 (Android 4.4.4)
Status: RESOLVED → VERIFIED
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
Attachment #8520644 - Flags: approval-mozilla-esr31?
Attachment #8520644 - Flags: approval-mozilla-aurora?
Comment on attachment 8520644 [details] [diff] [review]
Untested patch

Approving for Aurora only, this doesn't meet ESR landing criteria
Attachment #8520644 - Flags: approval-mozilla-esr31?
Attachment #8520644 - Flags: approval-mozilla-esr31-
Attachment #8520644 - Flags: approval-mozilla-aurora?
Attachment #8520644 - Flags: approval-mozilla-aurora+
Keywords: checkin-needed
Whiteboard: [checkin-needed: mozilla-aurora; land together with bug 1074793, this patch second]
Flags: needinfo?(cbook)
i guess ryan works on this today (or better later today)
Flags: needinfo?(cbook) → needinfo?(ryanvm)
Yep, I'll get to them when I get to them today.
https://hg.mozilla.org/releases/mozilla-aurora/rev/0ca3200f1bbf
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
Whiteboard: [checkin-needed: mozilla-aurora; land together with bug 1074793, this patch second]
You need to log in before you can comment on or make changes to this bug.