1114637 - Don't display "Unix error 1 during operation setPermissions" when a download terminates on Firefox for Android

Status: RESOLVED FIXED

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

Description

[:Paolo Amadini]

When a download completes on a recent Nightly build of Firefox for Android, a message similar to the following is displayed:

W/GeckoConsole(22113): [JavaScript Error: "Unix error 1 during operation setPermissions on file /storage/emulated/0/Download/Mozilla.pdf (Operation not permitted)" {file: "resource://gre/modules/DownloadIntegration.jsm" line: 486}]

This happens because we try to set final file permissions, but they are not supported on the FAT file system where downloads are saved. We could catch that OS.File exception specifically on Android, and avoid reporting it to the Console, or display a log message instead.

Comment 1

[Magnus Melin [:mkmelin]]

I guess it could happen on linux too, just that not many people use a FAT file system there.

Comment 2

[Ganesh Sahukari]

Hi! Its my first time solving a bug. Can you guide as to how to get started?

Comment 3

[:Paolo Amadini]

(In reply to Ganesh Sahukari from comment #2)
> Hi! Its my first time solving a bug. Can you guide as to how to get started?

Welcome! You will need to build Firefox for Android, or alternatively you can work on Firefox for Desktop on Linux or Mac OS X, and test on a FAT file system like a USB key.

Maybe Margaret can provide some pointers for the Android build, or if you work on Desktop, you can find the build instructions here:

https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

Comment 4

[:Margaret Leibovic]

Thanks, Paolo. Build instructions for Firefox for Android can be found here:
https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec

Let us know if you need help building! For help with Firefox for Android, you can ask questions in #mobile on irc.mozilla.org.

Comment 5

[Anirudh GP(:anirudhgp)]

Hi can i work on this bug?

Comment 6

[:Paolo Amadini]

(In reply to anirudh.gp from comment #5)
> Hi can i work on this bug?

Ganesh may be working on this bug already. But I see you're working on bug 1116600, and it seems a good choice to me!

Comment 7

[Ganesh Sahukari]

i have built firefox for desktop on linux. Trying to recreate the issue. 
Formatted a usb pen drive to FAT32 and changed the Downloads folder to USB drive.
But i don't see any error message in the firebug console when a download completes.

Comment 8

[:Paolo Amadini]

(In reply to Ganesh Sahukari from comment #7)
> i have built firefox for desktop on linux. Trying to recreate the issue. 
> Formatted a usb pen drive to FAT32 and changed the Downloads folder to USB
> drive.
> But i don't see any error message in the firebug console when a download
> completes.

I haven't tried with Firebug, but I see the error in the Browser Console (from the Developer menu). The error is expected for single-file downloads, if you save a complete web pages it may not appear.

If you use the "chmod" command from a terminal on a file on the drive, do you get the "Operation not permitted" error?

Comment 9

[Ganesh Sahukari]

(In reply to :Paolo Amadini from comment #8)
> (In reply to Ganesh Sahukari from comment #7)
> > i have built firefox for desktop on linux. Trying to recreate the issue. 
> > Formatted a usb pen drive to FAT32 and changed the Downloads folder to USB
> > drive.
> > But i don't see any error message in the firebug console when a download
> > completes.
> 
> I haven't tried with Firebug, but I see the error in the Browser Console
> (from the Developer menu). The error is expected for single-file downloads,
> if you save a complete web pages it may not appear.
> 
> If you use the "chmod" command from a terminal on a file on the drive, do
> you get the "Operation not permitted" error?

I'm not getting "Operation not permitted" error. The drive is fat32 formatted

Comment 10

[:Paolo Amadini]

(In reply to Ganesh Sahukari from comment #9)
> I'm not getting "Operation not permitted" error. The drive is fat32 formatted

I was able to get the error on my machine reliably, testing with a loopback device. Actually the error does not occur if I have root privileges, maybe you are running Firefox or the chmod command from a root terminal?

I did this first from a root terminal (this actually creates a FAT16 file system, but should work the same way):

dd if=/dev/zero of=floppy.img bs=512 count=2880
losetup /dev/loop0 floppy.img
mkdosfs /dev/loop0
mount -t msdos -orw,umask=0000 /dev/loop0 /mnt/loop0

From a non-privileged terminal:

touch /mnt/loop0/test.txt
chmod 600 /mnt/loop0/test.txt

I see "chmod: changing permissions of `/mnt/loop0/test.txt': Operation not permitted.", and downloading files with Firefox to that directory also causes the error.

Comment 11

[Ganesh Sahukari]

(In reply to :Paolo Amadini from comment #10)
> (In reply to Ganesh Sahukari from comment #9)
> > I'm not getting "Operation not permitted" error. The drive is fat32 formatted
> 
> I was able to get the error on my machine reliably, testing with a loopback
> device. Actually the error does not occur if I have root privileges, maybe
> you are running Firefox or the chmod command from a root terminal?
> 
> I did this first from a root terminal (this actually creates a FAT16 file
> system, but should work the same way):
> 
> dd if=/dev/zero of=floppy.img bs=512 count=2880
> losetup /dev/loop0 floppy.img
> mkdosfs /dev/loop0
> mount -t msdos -orw,umask=0000 /dev/loop0 /mnt/loop0
> 
> From a non-privileged terminal:
> 
> touch /mnt/loop0/test.txt
> chmod 600 /mnt/loop0/test.txt
> 
> I see "chmod: changing permissions of `/mnt/loop0/test.txt': Operation not
> permitted.", and downloading files with Firefox to that directory also
> causes the error.

I tried via above method and able to reproduce the bug. downloading files with firefox to that directory is failing now.

Found following error in developer console

"NS_ERROR_NOT_AVAILABLE: Async version must be used" from ".../dist/bin/components/nsHelperAppDlg.js" line 209:0

Comment 12

[:Paolo Amadini]

(In reply to Ganesh Sahukari from comment #11)
> Found following error in developer console
> 
> "NS_ERROR_NOT_AVAILABLE: Async version must be used" from
> ".../dist/bin/components/nsHelperAppDlg.js" line 209:0

Yeah, you can ignore this additional error for the moment - this is due to unresolved bug 1114594, but should not be blocking the download. Also, I had some unrelated errors due to long file names, please make sure you're downloading a single file (like a setup program or package) with an 8+3 DOS short name (like "setup.exe"). The download should succeed, but you should see the "Operation not permitted" error.

At this point, can you see the source line from the error message? It should be the same as the one in the bug description (comment 0). The first thing I suggest is to comment out or delete the OS.File.setPermissions call that causes the error in the original file - it's not the full solution, but you should see the message go away if everything is OK with your build.

Comment 13

[Ganesh Sahukari]

Sorry for the delay,

I commented out the line, build was successful and the error was not showing up now.

Comment 14

[:Paolo Amadini]

(In reply to Ganesh Sahukari from comment #13)
> I commented out the line, build was successful and the error was not showing
> up now.

Great! So you're ready to build the actual patch for the bug.

In the final fix, we should do the call and report all errors other than "Operation not permitted". We have similar conditional checking elsewhere (see <http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#629>). You can check the "unixErrno" property (see <https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File.Error>).

When you've tested that other errors are reported but the other error isn't, you can then create a patch with your change and submit it to me for review:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Comment 15

[Ganesh Sahukari]

Attachment: Patch of Bug 1114637

Comment 16

[:Paolo Amadini]

Comment on attachment 8549376 [details] [diff] [review]
Patch of Bug 1114637

This looks quite good! I've started these tryserver builds:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c6261eeec27
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fea24451044c

While there is no automated test that covers this change, and no need for one, it's still worth running the others for an automatic verification that everything else still works. When they've passed, we're ready for check-in in the source tree!

Comment 17

[Ganesh Sahukari]

the tryserver builds are successful, 

Does i need to run the automated tests locally if so can you provide me the wiki link , or will they get executed automatically when i try to commit ?

Can i go ahead with the commit?

Comment 18

[:Paolo Amadini]

(In reply to Ganesh Sahukari from comment #17)
> the tryserver builds are successful, 

Good!

> Does i need to run the automated tests locally if so can you provide me the
> wiki link , or will they get executed automatically when i try to commit ?

The tryserver build is the only requirement, because a more complete set of tests will run automatically at commit time anyways. If you want to run the tests locally, often useful while developing, you can use the "mach test" command. In this case:

./mach xpcshell-test toolkit/components/jsdownloads/

For more details:

https://developer.mozilla.org/en-US/docs/Running_automated_tests

> Can i go ahead with the commit?

Yes! Normally you would set the "checkin-needed" keyword on the bug at this point, but in this case it's not required because I already did the first commit to an integration branch for you:

https://hg.mozilla.org/integration/fx-team/rev/0f3b187814d7

Comment 19

[Ganesh Sahukari]

> 
> > Can i go ahead with the commit?
> 
> Yes! Normally you would set the "checkin-needed" keyword on the bug at this
> point, but in this case it's not required because I already did the first
> commit to an integration branch for you:
> 
> https://hg.mozilla.org/integration/fx-team/rev/0f3b187814d7

Sorry for troubling you by repetitive questions. 
But if you have already commited to fx-team branch and as it was mentioned in wiki that changes will be merged to mozilla-central branch periodically once in a day. Then do i need to commit (maybe to some other branch)?

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0f3b187814d7

Comment 21

[:Paolo Amadini]

(In reply to Ganesh Sahukari from comment #19)
> Sorry for troubling you by repetitive questions.

No trouble, feel free to ask whenever you want.

> But if you have already commited to fx-team branch and as it was mentioned
> in wiki that changes will be merged to mozilla-central branch periodically
> once in a day. Then do i need to commit (maybe to some other branch)?

As you've noticed, what happens is that the change is merged to mozilla-central, and the bug is then marked as RESOLVED. The Target Milestone is updated as well, the fix will be present in Firefox 38 and it should already be available in today's Nightly.

Congratulations for fixing your first bug!

It was a very good first patch. Now that you've seen how the process works, would you be interested in something more challenging? I'm thinking of bug 1114594, since you've incurred in it while working on this bug, but if you prefer something else I'd be glad to help.

Comment 22

[Ganesh Sahukari]

Thank you @Paolo for helping me in fixing my first bug.

I would like to work on bug 1114594.