User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0 Build ID: 20171024165158 Steps to reproduce: run downloads.download with saveAs = true Actual results: The download always fails with an error "Unexpected error ..." Expected results: According to the documentation here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/downloads/download saveAs is supported for Firefox Android >= 52, so the download should work normally. If it's not supported and cannot be supported on Firefox Android, I think we should at least: 1. Fix it so that the file is downloaded without selection prompt (i.e. ignore the saveAs property) instead of throw an error. 2. Revise the documentation to inform the user that it's not supported.

Hello, I would like to work on and solve this bug. As I am a beginner, could you direct me to the files affected? Kind regards, Nathan

This basically happens for any file.

saveAs doesn't actually work on Android. Nathan, if you'd like to work on this please go ahead, the code in question is here: https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/toolkit/components/extensions/ext-downloads.js#526-543 That code should be skipped on Android, which you can test with something like ``` if (AppConstants.platform == "android") ``` But first, if you haven't done it yet, here are some instructions to get started on downloading the source code and building Firefox for Android: https://wiki.mozilla.org/Mobile/Get_Involved Feel free to needinfo me with any questions that come up along the way or when you have a patch ready to look at.

I am new here. I would like to contribute to this bug. I have done the setup so Please help me how to proceed.

Comment 3 contains some basic info. Else using a needinfo to :aswan would be a next step if there is a part of comment 3 that is confusing.

I wanted to confirm whether saveAs is still unsupported and will produce an error or if the code has changed so that the parameter is ignored. I'm planning to update the browser compatibility information but also to add a note to the page explaining the situation.

Nothing has changed, passing saveAs on android still fails with "Unexpected error". In any case, marking it as unsupported on android is the right thing to do, thanks!

Is this issue still not fixed? I can take this up, if no one is working on it.

Go for it

So for the details in Comment 3 I need to wrap that piece of code in this if condition ``` if (AppConstants.platform !== "android") { // Setup the file picker Save As dialog. const picker = Cc["@mozilla.org/filepicker;1"].createInstance(Ci.nsIFilePicker); picker.init(window, null, Ci.nsIFilePicker.modeSave); picker.displayDirectory = new FileUtils.File(dir); picker.appendFilters(Ci.nsIFilePicker.filterAll); picker.defaultString = basename; // Configure a default file extension, used as fallback on Windows. picker.defaultExtension = ext && ext[1]; // Open the dialog and resolve/reject with the result. return new Promise((resolve, reject) => { picker.open(result => { if (result === Ci.nsIFilePicker.returnCancel) { reject({message: "Download canceled by the user"}); } else { resolve(picker.file.path); } }); }); } ``` Is it correct way to skip the code for android? Also does it requires any test to be written? Or I need to verify some existing test case?

The code from comment 10 won't work as written since it will cause createTarget() to not return anything on android. Eslint ought to complain about that, but if you actually run that code, that will just cause downloads with saveAs=true to fail in a different way. Instead, I guess we should just return `target` without trying to show the file picker. As for testing, there is an existing test for saveAs at toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.html. This is a chrome mochitest (https://developer.mozilla.org/en-US/docs/Mozilla/QA/Chrome_tests), which doesn't run on Android. Ideally we would add a new test case to run on Android that verifies they expected behavior: that a downloads with saveAs succeeds on Android.

(In reply to Andrew Swan [:aswan] from comment #11) > As for testing, there is an existing test for saveAs at > toolkit/components/extensions/test/mochitest/ > test_chrome_ext_downloads_saveAs.html. This is a chrome mochitest > (https://developer.mozilla.org/en-US/docs/Mozilla/QA/Chrome_tests), which > doesn't run on Android. Ideally we would add a new test case to run on > Android that verifies they expected behavior: that a downloads with saveAs > succeeds on Android. How we can write a test case for android? And how can I verify it in my local?

It can just be a plain mochitest that calls download() passing saveAs=true. Without a fix for this bug, that will fail. If you don't have an Android device for testing on, you can use the Android emulator. I'm not sure where up-to-date instructions are for that, Luca do you know?

(In reply to Andrew Swan [:aswan] from comment #13) > It can just be a plain mochitest that calls download() passing saveAs=true. > Without a fix for this bug, that will fail. > > If you don't have an Android device for testing on, you can use the Android > emulator. I'm not sure where up-to-date instructions are for that, Luca do > you know? Hi Arshad, `./mach bootstrap` should be able to setup the development environment for the "Firefox for Android artifacts build" (more info on https://developer.mozilla.org/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build#I_want_to_work_on_the_front-end). You can take a look at the existing mochitests specific to Firefox for Android here: https://searchfox.org/mozilla-central/source/mobile/android/components/extensions/test/mochitest Once you has complete the setup of the development environment, it is usually a good idea to try to run one of the existing tests before any change has been applied and check that the test passes successfully, e.g. you can try to run one of the small ones: ./mach mochitest mobile/android/components/extensions/test/mochitest/test_ext_all_apis.html ---- Some additional side notes related to fix/workaround/investigate issues on "running the tests on Android": If something goes wrong (or gets stuck), you can take a look to the verbose logs by adding --log-mach-level=debug to the mach command (and/or looking to the android logs using 'adb logcat' is sometimes helpful). Recently, at least in my local development environment, the Firefox for Android mochitests seems to get stuck pretty often on the test page, but reloading the test page from the emulator seems to be enough to unblock the test and force it to run as expected, this issue doesn't seem to happen on the try pushes. There is currently some issues with running xpcshell tests on the 7.0 emulator (Bug 1451930), you may workaround it by explicitly installing and running the 4.2 x86 emulator (more info on the command to choose the emulator and running the tests are on https://wiki.mozilla.org/Mobile/Fennec/Android/Testing#Running_tests_on_the_Android_emulator).

Thanks Luca, for this detailed information I didn't knew that this issue requires an android build. As I my laptop config is a bit low it cannot handle android builds at persent. So I will not be able to work on this, for now. Util I get a new machine. I looking to work only in the mozilla central repo except android related things. If anyone is interested to take this up. Please go ahead and follow the steps given in Comment 14

I have got my laptop upgraded. I can work on this now. Assigning to myself and will start looking into it.

I am done with the setup, but on running this test, ./mach mochitest mobile/android/components/extensions/test/mochitest/test_ext_all_apis.html I am getting this error ADBRootError: Can not run command test -d /storage/sdcard0 as root! My device is not rooted. Is there something, which I am missing?

Luca is on PTO this week, but your options are to run on a rooted physical device or use the emulator. Instructions for using the emulator are linked in comment 14.

Ok. Thank you for the info Andrew. I will try it out myself.

I tried with this script <!DOCTYPE HTML> <html> <head> <title>Tabs captureVisibleTab Test</title> <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> <script type="text/javascript" src="/tests/SimpleTest/AddTask.js"></script> <script type="text/javascript" src="/tests/SimpleTest/ExtensionTestUtils.js"></script> <script type="text/javascript" src="head.js"></script> <link rel="stylesheet" href="/tests/SimpleTest/test.css"/> </head> <body> <script type="text/javascript"> "use strict"; add_task(async function testDownload() { let extension = ExtensionTestUtils.loadExtension({ manifest: {}, background: async function() { await browser.downloads.download({ url : "https://bugzilla.mozilla.org/show_bug.cgi?id=1333787" }) browser.test.notifyPass("downloadPass"); }, }); await extension.startup(); await extension.awaitFinish("downloadPass"); await extension.unload(); }); </script> </body> </html> But it just freezes and gives this error Test timed out. reportError@SimpleTest/TestRunner.js:121:7 TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7 I am running this script using below command in emulator ./mach mochitest mobile/android/components/extensions/test/mochitest/test_ext_browsingData_downloads_saveAs.html Is there something I am missing? Andrew, will you be able to help me with this? Or should I wait for Luca

Figured it out. I was missing downloads permissions. Its working now.

Comment on attachment 9012678 [details] Bug 1420744 - Extensions downloads.download api with saveAs flag, skips file picker in android Andrew Swan [:aswan] has approved the revision.

Pushed by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0d3748a0ad24 Extensions downloads.download api with saveAs flag, skips file picker in android r=aswan

Backout by shindli@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2c0daf489bc9 Backed out changeset 0d3748a0ad24 for c4 failures in toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_saveAs.htm

What is a c4 failure?

That means the 4th chunk of mochitest chrome tests. The sheriffs usually include a link to the test failure when a patch is backed out, not sure why that didn't happen here: https://treeherder.mozilla.org/logviewer.html#?job_id=202687669&repo=autoland&lineNumber=1933 I thought there was a try run on this patch before landing but now I don't see one. Anyway, the failing test is test_chrome_ext_downloads_saveAs.html. That test probably shouldn't run on Android at all, the fact that is passes is because the test replaces the implementation of the file picker. Let's just add an "os == android" clause here: https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/toolkit/components/extensions/test/mochitest/chrome.ini#25

This test test_chrome_ext_downloads_saveAs.html, I have written for this bug. Skip only works for the above test right? I think skip was there for someother test and I have placed my test there, is it due to that?

Ok. I got confused with my test, this is chrome.ini I have similar test in mochitest.ini. I will try to add this "os == android" and check it.

Andrew, Made your suggested change. How can I test it on treeherder?

https://wiki.mozilla.org/ReleaseEngineering/TryServer#Using_mach In this case, I think something like this should be good: mach try -b o -p android-api-16 -u mochitests -t none

Does it needs any special access? will I be able to run it?

It is documented elsewhere on that same page: https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server

Andrew, I was reading through that docs, it says I can get Level 1 access which will give be treeherder access. But I need a vouch from some existing Level 2+ member. Where can I get that vouch from?

Am I even eligible to get that access at this stage?

I was chicking, should I get it to checked-in again to get it verified?

Have you verified that tests are passing? Or do you need me to start a try run for you?

I was not able to run it on try server. If you can run it for me, it will be great. I have already pushed the code to phabricator

Thanks Andrew for running this.

Andrew, tests here is completed. https://treeherder.mozilla.org/#/jobs?repo=try&revision=02b2d11e3e74f4bc6cc4e5b5e2baba19495c9849 I don't see any failed tests here. Looks like everything is fine. Should I add the check-in needed flag here now?

The build failed on Linux for some unrelated reason. I've retriggered it. Once we get a full test run on Linux, if the tests are green then go ahead.

Is it retriggered here itself? https://treeherder.mozilla.org/#/jobs?repo=try&revision=02b2d11e3e74f4bc6cc4e5b5e2baba19495c9849 or some other url?

url in Comment 39 shows the tests completed. is there some other url generated for new test

whoops, the first retrigger failed for some reason. Note there's no M job (M for mochitests) for Linux. Once the retrigger happens, it will appear on that same page. When it appears, if it is green you can set checkin-needed.

Is the tests completed? I still see two red bars in that url, looks like something is still failing

We never got Linux tests in that previous try run and I was unable to retrigger them. Lets just do this over: https://treeherder.mozilla.org/#/jobs?repo=try&revision=257bdd6c008baea10b8c6879ab9b85fa74565a23

Looks like this test has been completed, and there is something failed in it.

I started a separate Linux test run, the failures here all look like known intermittents: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5776ef30cacaa75f4b167ddf9fb5ef10e1b7316b&selectedJob=204807515 But there's still a failure on Android, I commented over on phabricator

Andrew, I am done with the suggested changes. Can you start a try again for the latest code for me? I have run all the tests in my local system and all the tests are passed.

(In reply to Arshad Kazmi [:arshadkazmi42] from comment #50) > Andrew, I am done with the suggested changes. Can you start a try again for > the latest code for me? I have run all the tests in my local system and all > the tests are passed. Hi Arshad, I just pushed on try the last version of the patch attached on Phabricator: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d527e6d7034cdad0b5b7400622f25c8715cc48c3

Thanks Luca

Luca, Andrew Treeherder tests are completed and it is showing 5 failed tests. Can you check if these are blocking tests or it can be ignored?

I can't imagine how the BrowserErrorReporter test failure would be related to your change, but it happened twice and we don't have an open bug for a known intermittent failure. Osmose have you seen this particular failure before: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d527e6d7034cdad0b5b7400622f25c8715cc48c3&selectedJob=206139218

I've seen that shape of failure, but I can't see how this patch would have an effect; the test is creating a ScriptError and passing it to a method that ends up calling a mocked fetch function, and then searches through available calls on that mocked fetch. The failure wasn't even on Android, which is the only tests affected by this change if I'm reading the patch correctly. I can't replicate this locally running the BrowserErrorReporter tests with the patch applied. It might be worth another try run to see if this is really as consistent as we suspect based on the two runs.

Andrew, Can you run the tests on try server one more time as suggested by Michael in Comment 55

I just retriggered the bc1 job a couple of times

And, the retriggers failed, a sheriff suggested that was due to the base revision for the patch being quite old. Arshad, can you please rebase your patch onto a recent mozilla-central revision?

Sure. I will do it in some time and update it on phabricator

I am done with rebasing. Updated the code in Phabricator.

(In reply to Arshad Kazmi [:arshadkazmi42] from comment #60) > I am done with rebasing. Updated the code in Phabricator. Hi Arshad, I just pushed to try the rebased patch (as currently attached on Phabricator): - https://treeherder.mozilla.org/#/jobs?repo=try&revision=975bd84c2bdbd6aa74663c08b610857e9a12e743

Thanks Luca. In the test result here https://treeherder.mozilla.org/#/jobs?repo=try&revision=975bd84c2bdbd6aa74663c08b610857e9a12e743 There are still some errors showing up. Are these related to my fix? Or these are intermittent issues?

(In reply to Arshad Kazmi [:arshadkazmi42] from comment #62) > Thanks Luca. > > In the test result here > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=975bd84c2bdbd6aa74663c08b610857e9a12e743 > > There are still some errors showing up. Are these related to my fix? Or > these are intermittent issues? Hi Arshad, My guess is that your last rebase didn't yet include the fixes for some unrelated failures that have been fixed in the meantime, here is another try push that I pushed after rebasing your patch on top of a more recent mozilla-central tip (to be precise, on top of the commit https://hg.mozilla.org/try/rev/0029fe1e56d7, which merged inbound to central on Oct 31): https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c24a3453c8459fffe1b2b3cc30b811c7e941be8

Thanks, Luca for the rebase and re-running it. I think there are still some tests which are failing.

Those all looks like known intermittents, please go ahead and request checkin. Thanks for all your hard work!

Great. Thanks for all the help.

Pushed by rvandermeulen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8160c2d57418 Extensions downloads.download api with saveAs flag, skips file picker in android r=aswan

Hello, it's my understanding that this is covered by automated testing, will you also require manual testing here? if not, please set the qe-verify- flag, thanks!

Automated testing is there for this. But I think we will need manual testing also for this. Any thoughts Andrew on this? We would need manual testing for this, right?

The automated testing seems sufficient to me, why do you think we need additional manual testing?

Just checked. It was WebExtension bug, I totally forgot, I thought its Firefox Android App related issue. That's why I was suggesting for manual testing. I have been working on this, for almost a month :P We would not be needing manual testing for it. I will add qe-verify- flag to this. Thank you for all the help and guidance Andrew and Luca