Closed Bug 1420744 Opened 7 years ago Closed 6 years ago

downloads.download with saveAs=true always gets an error

Categories

(WebExtensions :: Android, defect, P5)

Product:
WebExtensions
Component:
Android
Version:
57 Branch
Platform:
All
Android
Type:
defect

Tracking

(firefox65 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla65
Tracking Flags:
Tracking Status
firefox65 --- fixed

People

(Reporter: danny0838, Assigned: arshadkazmi42, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

Bug 1420744 - Extensions downloads.download api with saveAs flag, skips file picker in android
46 bytes, text/x-phabricator-request
aswan
: review+
Details | Review 
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.
Component: General → WebExtensions: Android
OS: Unspecified → Android
Product: Firefox for Android → Toolkit
Hardware: Unspecified → All
Version: Firefox 57 → 57 Branch
Priority: -- → P5
Keywords: good-first-bug
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
Flags: needinfo?(danny0838)
This basically happens for any file.
Flags: needinfo?(danny0838)
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.
Mentor: aswan
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.
Product: Toolkit → WebExtensions
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.
Flags: needinfo?(aswan)
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!
Flags: needinfo?(aswan)
Is this issue still not fixed? 

I can take this up, if no one is working on it.
Flags: needinfo?(aswan)
Go for it
Assignee: nobody → arshadkazmi42
Flags: needinfo?(aswan)
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?
Flags: needinfo?(aswan)
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.
Flags: needinfo?(aswan)
(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?
Flags: needinfo?(aswan)
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?
Flags: needinfo?(aswan) → needinfo?(lgreco)
(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).
Flags: needinfo?(lgreco)
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
Assignee: arshadkazmi42 → nobody
I have got my laptop upgraded. I can work on this now. Assigning to myself and will start looking into it.
Assignee: nobody → arshadkazmi42
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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?
Flags: needinfo?(lgreco)
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.
Flags: needinfo?(lgreco)
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
Flags: needinfo?(aswan)
Figured it out. I was missing downloads permissions. Its working now.
Flags: needinfo?(aswan)
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.
Attachment #9012678 - Flags: review+
Keywords: checkin-needed
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
Keywords: checkin-needed
Blocks: 1495617
No longer blocks: 1495617
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?
Flags: needinfo?(aswan)
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
Flags: needinfo?(aswan)
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?
Flags: needinfo?(aswan)
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.
Flags: needinfo?(aswan)
Andrew, Made your suggested change.

How can I test it on treeherder?
Flags: needinfo?(aswan)
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
Flags: needinfo?(aswan)
Does it needs any special access? will I be able to run it?
Flags: needinfo?(aswan)
It is documented elsewhere on that same page:
https://wiki.mozilla.org/ReleaseEngineering/TryServer#Getting_access_to_the_Try_Server
Flags: needinfo?(aswan)
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?
Flags: needinfo?(aswan)
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?
Flags: needinfo?(aswan)
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
Flags: needinfo?(aswan)
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?
Flags: needinfo?(aswan)
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.
Flags: needinfo?(aswan)
url in Comment 39 shows the tests completed.
is there some other url generated for new test
Flags: needinfo?(aswan)
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.
Flags: needinfo?(aswan)
Is the tests completed? I still see two red bars in that url, looks like something is still failing
Flags: needinfo?(aswan)
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
Flags: needinfo?(aswan)
Looks like this test has been completed, and there is something failed in it.
Flags: needinfo?(aswan)
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
Flags: needinfo?(aswan)
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.
Flags: needinfo?(aswan)
(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
Flags: needinfo?(aswan)
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?
Flags: needinfo?(lgreco)
Flags: needinfo?(aswan)
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
Flags: needinfo?(mkelly)
Flags: needinfo?(lgreco)
Flags: needinfo?(aswan)
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.
Flags: needinfo?(mkelly)
Andrew, 
Can you run the tests on try server one more time as suggested by Michael in Comment 55
Flags: needinfo?(aswan)
I just retriggered the bc1 job a couple of times
Flags: needinfo?(aswan)
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?
Flags: needinfo?(arshadkazmi42)
Sure. I will do it in some time and update it on phabricator
Flags: needinfo?(arshadkazmi42)
I am done with rebasing. Updated the code in Phabricator.
Flags: needinfo?(aswan)
(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
Flags: needinfo?(aswan)
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?
Flags: needinfo?(lgreco)
Flags: needinfo?(aswan)
(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
Flags: needinfo?(lgreco)
Thanks, Luca for the rebase and re-running it.

I think there are still some tests which are failing.
Flags: needinfo?(lgreco)
Those all looks like known intermittents, please go ahead and request checkin.
Thanks for all your hard work!
Flags: needinfo?(lgreco)
Flags: needinfo?(aswan)
Great. Thanks for all the help.
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8160c2d57418
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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!
Flags: needinfo?(arshadkazmi42)
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?
Flags: needinfo?(arshadkazmi42) → needinfo?(aswan)
The automated testing seems sufficient to me, why do you think we need additional manual testing?
Flags: needinfo?(aswan)
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
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: