1369782 - Require user interaction for downloads.open()

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Mark Striemer [:mstriemer]]

An extension should only be able to call downloads.open() when initiated by a user interaction like a button click or shortcut command. If this is not the case then a user might not be able to tell why files are opening on their computer which could be confusing.

This is how Chrome works, as noted in a Chromium bug [1] but it is not documented.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=349715

Comment 2

[Mark Striemer [:mstriemer]]

Attachment: Bug 1369782 - Require user interaction for downloads.open()

Review commit: https://reviewboard.mozilla.org/r/145824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145824/

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

https://reviewboard.mozilla.org/r/145824/#review149890

A couple of questions:
1. What about a test of calling `download.open()` successfully?
2. Does this have the same issues as bug 1369577?  If so, can you add notes there that the bug covers both permissions.request and download.open pending bug 1350151

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html:44
(Diff revision 1)
> +    // TODO: Once downloads.{pause,cancel,resume} lands (bug 1245602) test that this gives a good
> +    // error when called with an incompleted download.

Hm, I see you just copied this but pause/resume/cancel landed some time ago.  Can you open a new bug for this?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html:90
(Diff revision 1)
> +    // TODO: Once downloads.{pause,cancel,resume} lands (bug 1245602) test that this gives a good
> +    // error when called with an incompleted download.

and this should be removed

Comment 4

[Mark Striemer [:mstriemer]]

Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145824/diff/1-2/

Comment 5

[Mark Striemer [:mstriemer]]

Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

https://reviewboard.mozilla.org/r/145824/#review149890

> A couple of questions:
> 1. What about a test of calling download.open() successfully?

I had a test for it, but it would download the text file to ~/Downloads and open TextEdit when it worked.
> 2. Does this have the same issues as bug 1369577?  If so, can you add notes there that the bug covers both permissions.request and download.open pending bug 1350151

It does. I've commented on both bugs.

> Hm, I see you just copied this but pause/resume/cancel landed some time ago.  Can you open a new bug for this?

Filed bug 1370623.

> and this should be removed

I removed both comments since there's a bug now.

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

https://reviewboard.mozilla.org/r/145824/#review150440

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

https://reviewboard.mozilla.org/r/145824/#review149890

On question 1, is it feasible to override the associated application just for the duration of the test?
That doesn't need to hold this up though...

Comment 8

[Mark Striemer [:mstriemer]]

Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

https://reviewboard.mozilla.org/r/145824/#review149890

I'm not sure, it could be. We had discussed this briefly over IRC but it seemed like it wasn't easy to do.

I filed bug 1371814 to track adding that test.

Comment 9

[Mark Striemer [:mstriemer]]

This needs the fix from bug 1369577 to function in all cases. That will likely need to be uplifted.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe04c0fec69b
Require user interaction for downloads.open() r=aswan

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for eslint failure at toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html:60:

https://hg.mozilla.org/integration/autoland/rev/44a734e1209d8c164a9b1b7e56fc9cb65440efab

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=fe04c0fec69b97608c8b7090e4b7e6ff73ac0f6d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=106068140&repo=autoland

TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html:60:12 | No corresponding 'removeEventListener(keypress)' was found. (mozilla/balanced-listeners)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html:63:58 | There should be no space after '{'. (object-curly-spacing)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html:63:74 | There should be no space before '}'. (object-curly-spacing)

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This also fails the test:

https://treeherder.mozilla.org/logviewer.html#?job_id=106072993&repo=autoland

TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html | Opening download fails. - got true, expected false
TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html | The error is informative. - got undefined, expected "Invalid download id 10"

Comment 13

[Mark Striemer [:mstriemer]]

Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145824/diff/2-3/

Comment 14

[Andrew Swan [:aswan]]

Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

https://reviewboard.mozilla.org/r/145824/#review152200

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html:60
(Diff revisions 2 - 3)
>    await extension.unload();
>  });
>  
>  add_task(async function downloads_open_invalid_id() {
>    async function pageScript() {
> -    window.addEventListener("keypress", () => {
> +    window.addEventListener("keypress", async function handler() {

You can also write `async () => { ...`
And if you pass `{once: true}` as the third parameter to `addEventListener()` then you don't need to explicitly remove the listener.

Comment 15

[Mark Striemer [:mstriemer]]

Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

https://reviewboard.mozilla.org/r/145824/#review152200

> You can also write `async () => { ...`
> And if you pass `{once: true}` as the third parameter to `addEventListener()` then you don't need to explicitly remove the listener.

Ah, that's handy. Thanks!

Comment 16

[Mark Striemer [:mstriemer]]

Tests/lint fixed.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8bb2840c6ee3
Require user interaction for downloads.open() r=aswan

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8bb2840c6ee3

Comment 19

[Mark Striemer [:mstriemer]]

Attachment: downloads.open_test-1.0.zip

Extension to help testing.

Comment 20

[Mark Striemer [:mstriemer]]

Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

Approval Request Comment
[Feature/Bug causing the regression]: bug 1245636
[User impact if declined]: Add-ons would be able to open downloads without a user action which could cause confusion or lead to users opening files they did not intend to open.
[Is this code covered by automated tests?]: Partially
[Has the fix been verified in Nightly?]: I have verified it, following STR below
[Needs manual test from QE? If yes, steps to reproduce]: 
  1. Extract and install the attached add-on in about:debugging.
  2. Click the browser action button for the add-on.
  3. Click the "Start download" button.
    * In release, beta: text file download will complete and be opened
    * In nightly: text file is downloaded and error is reported on the add-on page
  4. Click "Open download" button.
    * Download opens in all versions.
  5. Click "View Firefox logo" button.
  6. Save image with Right click > Save image as...
    * In release, beta: image download will complete and be opened
    * In nightly: image is downloaded and error is reported on the add-on page
[List of other uplifts needed for the feature/fix]: bug 1369577 (that bug also affects bug 1197420)
[Is the change risky?]: No
[Why is the change risky/not risky?]:
  * The code is very similar to permissions.request() which has been on Nightly for a few months.
  * This could cause some existing add-ons to stop working but is compatible with Chrome. The add-ons that will stop working could lead to a bad user experience, however.
[String changes made/needed]: No

Comment 21

[Julien Cristau [:jcristau]]

Bug 1369577 doesn't seem fixed yet.  Does this mean I should hold off on this uplift request, or can this go in independently?  Also, since bug 1245636 is not new (it landed in 48), what's the reason for this skipping a train?

Comment 22

[Mark Striemer [:mstriemer]]

Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

After some more discussion we've decided that since bug 1369577 has hit a bit of a snag we'll let this ride the train to 56.

Comment 23

[Julien Cristau [:jcristau]]

Thanks Mark.

Comment 24

[Will Bamberg [:wbamberg]]

I've added something about this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/downloads/open. Please let me know if this covers it. I didn't document that this is new in Firefox 56, since advertising the old behavior doesn't seem to make sense.

Comment 25

[Mark Striemer [:mstriemer]]

That makes sense to me, thanks!