Closed Bug 1750253 Opened 3 years ago Closed 3 years ago

Always Open with "Firefox" triggers continous spam tabs

Categories

(Firefox :: Downloads Panel, defect, P3)

Firefox 97
Desktop
All
defect
Points:
3

Tracking

()

RESOLVED FIXED
98 Branch
Tracking Status
firefox-esr91 --- wontfix
firefox96 --- wontfix
firefox97 --- wontfix
firefox98 --- fixed

People

(Reporter: aflorinescu, Assigned: Gijs)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [fidefe-mr11-downloads])

Attachments

(3 files)

Attached video always_open_with_firefox.mp4 (deleted) —
[Description:]

When Firefox gets set as default open for a most non-default extensions, it triggers continous open tabs with the download.

[Environment:]

Windows 10, Mac 11, Ubuntu 20
78.* ESR
91.* ESR
96
97.0b3
98.0a1 2022-01-13

[Steps:]
  1. New profile.
  2. (various ways to achieve this)From your operating system set up Firefox to be default application for the mime type you are testing with (e.g set csv to always open with firefox)
  3. Download a csv / open a csv.
[Actual Result:]

See attached video: tabs start being generated.

[Expected Result:]

If the file cannot be opened by Firefox abort or throw error.

[Note:]
  1. Download might not be the best component, but I believe that the new download changes will make the application handler more accesible than in the past.
  2. S3 severity until properly triaged.
Attached image always_open_with_firefox.jpg (deleted) —

@Gijs, this immediately reminds me of bug 1678255. Do you think we can extend what that patch does to cover file types, or is something else going on?

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Molly Howell (she/her) [:mhowell] from comment #2)

@Gijs, this immediately reminds me of bug 1678255. Do you think we can extend what that patch does to cover file types, or is something else going on?

This is technically a dupe of bug 167320 and bug 215554. Yes, the problem is that old.

We can do something similar to bug 1678255 to catch it at the second iteration, but also (at least on Windows and macOS) we should be able to figure out whether we're the OS default app and catch it that way, on the initial load (and force-prompt the user, even if the filetype is set to "always open with default app"), similar to the fix in bug 1496380. And I actually thought I implemented something like that but I can't find the bug right now, so maybe I imagined it?!

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mhowell)

(In reply to :Gijs (he/him) from comment #4)

I gotta run but https://searchfox.org/mozilla-central/rev/3de56eb5f266f523340e739ae1b53258e0a95dfe/uriloader/exthandler/nsMIMEInfoImpl.cpp#446-461 looks relevant.

Looks like the only consumer of that is https://searchfox.org/mozilla-central/rev/3de56eb5f266f523340e739ae1b53258e0a95dfe/toolkit/components/pdfjs/content/PdfStreamConverter.jsm#1091 , and although the thought to use it for the infinite loop prevention was there (see tail of https://bugzilla.mozilla.org/show_bug.cgi?id=1633790#c3 ) that was never implemented.

Flags: needinfo?(mhowell)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Priority: -- → P3
Whiteboard: [fidefe-mr11-downloads]
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/2a43fb1b7fef stop infinite file handling loops by checking if we're about to pass files to ourselves via the OS, r=mhowell

Backed out changeset 2a43fb1b7fef (Bug 1750253) for causing bc failures in browser_local_files_open_doesnt_duplicate.js
Backout link: https://hg.mozilla.org/integration/autoland/rev/091ad3f3a491395b3f917c5635959b5d2c4d0352
Push with failures, failure log.

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/47b92c53d99d stop infinite file handling loops by checking if we're about to pass files to ourselves via the OS, r=mhowell

Backed out changeset 47b92c53d99d (bug 1750253) for causing bc failures in uriloader/exthandler/tests/mochitest/browser_open_internal_choice_persistence.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/ffbd5764b978c5ecf2bf3d1ab1ab0b87588e301e

Push with failures

Failure log

INFO - Testing with file_image_svgxml.svg; Pref enabled - external handling as default should not change prefs
[task 2022-01-26T19:37:37.877Z] 19:37:37     INFO - Load window and tabs
[task 2022-01-26T19:37:37.877Z] 19:37:37     INFO - Buffered messages finished
[task 2022-01-26T19:37:37.878Z] 19:37:37     INFO - TEST-UNEXPECTED-FAIL | uriloader/exthandler/tests/mochitest/browser_open_internal_choice_persistence.js | UCT window should not have appeared - Got true, expected false
[task 2022-01-26T19:37:37.878Z] 19:37:37     INFO - Stack trace:
[task 2022-01-26T19:37:37.878Z] 19:37:37     INFO - chrome://mochikit/content/browser-test.js:test_is:1403
[task 2022-01-26T19:37:37.878Z] 19:37:37     INFO - chrome://mochitests/content/browser/uriloader/exthandler/tests/mochitest/browser_open_internal_choice_persistence.js:test_check_saving_handler_choices_with_downloads_pref_enabled:405
Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Sandor Molnar from comment #10)

Backed out changeset 47b92c53d99d (bug 1750253) for causing bc failures in uriloader/exthandler/tests/mochitest/browser_open_internal_choice_persistence.js

wth. How is this orange on macOS 11 but not on 10.15. Try was green. And the path-based filtering on autoland on treeherder filters out half the backfill tasks that got run (but somehow not all of them)? :-\

https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Crunning%2Cpending%2Crunnable&searchStr=macos&tochange=285565ecc5817ff823a7f4e0d87d52471cac8f2b&fromchange=9b990a82db175cd688c4f54e4756409fce0f5296&test_paths=uriloader%2Fexthandler%2Ftests%2Fmochitest

(In reply to :Gijs (he/him) from comment #11)

(In reply to Sandor Molnar from comment #10)

Backed out changeset 47b92c53d99d (bug 1750253) for causing bc failures in uriloader/exthandler/tests/mochitest/browser_open_internal_choice_persistence.js

wth. How is this orange on macOS 11 but not on 10.15. Try was green. And the path-based filtering on autoland on treeherder filters out half the backfill tasks that got run (but somehow not all of them)? :-\

https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Crunning%2Cpending%2Crunnable&searchStr=macos&tochange=285565ecc5817ff823a7f4e0d87d52471cac8f2b&fromchange=9b990a82db175cd688c4f54e4756409fce0f5296&test_paths=uriloader%2Fexthandler%2Ftests%2Fmochitest

I'll dig into this on the macos 11 workers. FIrst, I'll get a re-install to a fresh build on 1+ workers and see if I can reproduce it.

We're getting leftover state from somewhere; This reminds me that we will benefit from moving non-perf tasks into macos vm's so that we can completely reset state with an image (on my personal goal/dream list for this year).

Points: --- → 3

(In reply to :dhouse from comment #12)

(In reply to :Gijs (he/him) from comment #11)

(In reply to Sandor Molnar from comment #10)

Backed out changeset 47b92c53d99d (bug 1750253) for causing bc failures in uriloader/exthandler/tests/mochitest/browser_open_internal_choice_persistence.js

wth. How is this orange on macOS 11 but not on 10.15. Try was green. And the path-based filtering on autoland on treeherder filters out half the backfill tasks that got run (but somehow not all of them)? :-\

https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Crunning%2Cpending%2Crunnable&searchStr=macos&tochange=285565ecc5817ff823a7f4e0d87d52471cac8f2b&fromchange=9b990a82db175cd688c4f54e4756409fce0f5296&test_paths=uriloader%2Fexthandler%2Ftests%2Fmochitest

I'll dig into this on the macos 11 workers. FIrst, I'll get a re-install to a fresh build on 1+ workers and see if I can reproduce it.

We're getting leftover state from somewhere; This reminds me that we will benefit from moving non-perf tasks into macos vm's so that we can completely reset state with an image (on my personal goal/dream list for this year).

Hm, so I just pushed to try with a bunch of logging, and all the macOS 11 jobs came back green: https://treeherder.mozilla.org/jobs?repo=try&revision=302c22ebc21d5feebe185127d5ca9a105d161faa . (AFAICT all the 1-7 jobs for all mochitest flavours are just running the dir that broke before, so that's some 28 attempts and they're all green...)

I guess I'll try relanding, and if this goes orange again on macOS 11 will skip the test there for now and defer further investigation to a follow-up bug - if the issue doesn't reliably repro on try or locally then I'm not sure how to figure it out.

Flags: needinfo?(gijskruitbosch+bugs)
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5a1e21232a3b stop infinite file handling loops by checking if we're about to pass files to ourselves via the OS, r=mhowell
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch

Given that we're calling out changes around this in the Fx97 relnotes ("Now, you can set a default app to open a file type. Choose the application you want to use to open files of a specific type in your Firefox settings."), I'm leaving this on the radar for a possible dot release ride-along should the need arise after launch.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)

Given that we're calling out changes around this in the Fx97 relnotes ("Now, you can set a default app to open a file type. Choose the application you want to use to open files of a specific type in your Firefox settings."), I'm leaving this on the radar for a possible dot release ride-along should the need arise after launch.

This is pref'd off for 97 ( bug 1753096 ), maybe release notes should be updated aswell ?

Flags: needinfo?(ryanvm)

Sorry, I missed that that change was also gated behind the pref. Removed.

Flags: needinfo?(ryanvm)
Regressions: 1752482
Blocks: 1784695
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: