Require user interaction for downloads.open()

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: General
RESOLVED FIXED
3 months ago
2 days ago

People

(Reporter: mstriemer, Assigned: mstriemer)

Tracking

({dev-doc-needed})

unspecified
mozilla56
dev-doc-needed
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 wontfix, firefox56 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

3 months ago
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
(Assignee)

Updated

3 months ago
Duplicate of this bug: 1367529
Comment hidden (mozreview-request)

Comment 3

2 months ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 5

2 months ago
mozreview-review-reply
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

2 months ago
mozreview-review
Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

https://reviewboard.mozilla.org/r/145824/#review150440
Attachment #8874458 - Flags: review?(aswan) → review+

Comment 7

2 months ago
mozreview-review-reply
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...
(Assignee)

Comment 8

2 months ago
mozreview-review-reply
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.
(Assignee)

Updated

2 months ago
Keywords: checkin-needed
(Assignee)

Updated

2 months ago
See Also: → bug 1369577
(Assignee)

Comment 9

2 months ago
This needs the fix from bug 1369577 to function in all cases. That will likely need to be uplifted.
See Also: → bug 1350151

Comment 10

2 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fe04c0fec69b
Require user interaction for downloads.open() r=aswan
Keywords: checkin-needed
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)
Flags: needinfo?(mstriemer)
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 hidden (mozreview-request)

Comment 14

2 months ago
mozreview-review
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.
(Assignee)

Comment 15

2 months ago
mozreview-review-reply
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!
(Assignee)

Comment 16

2 months ago
Tests/lint fixed.
Flags: needinfo?(mstriemer)
Keywords: checkin-needed

Updated

2 months ago
Whiteboard: triaged

Comment 17

2 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8bb2840c6ee3
Require user interaction for downloads.open() r=aswan
Keywords: checkin-needed

Comment 18

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8bb2840c6ee3
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Comment 19

2 months ago
Created attachment 8877662 [details]
downloads.open_test-1.0.zip

Extension to help testing.
(Assignee)

Comment 20

2 months ago
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
Attachment #8874458 - Flags: approval-mozilla-beta?
(Assignee)

Updated

2 months ago
status-firefox55: --- → affected
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?
Flags: needinfo?(mstriemer)
(Assignee)

Comment 22

2 months ago
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.
Flags: needinfo?(mstriemer)
Attachment #8874458 - Flags: approval-mozilla-beta?
Thanks Mark.
status-firefox55: affected → wontfix

Updated

3 days ago
See Also: → bug 1390882
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.