1750253 - Always Open with "Firefox" triggers continous spam tabs

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect, P3)

Description

[Adrian Florinescu [:aflorinescu]]

Attachment: always_open_with_firefox.mp4

[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.

Comment 1

[Adrian Florinescu [:aflorinescu]]

Attachment: always_open_with_firefox.jpg

Comment 2

[Molly Howell (she/her) (no longer active)]

@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?

Comment 3

[:Gijs (he/him)]

(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?!

Comment 4

[:Gijs (he/him)]

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

Comment 5

[:Gijs (he/him)]

(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.

Comment 6

[:Gijs (he/him)]

Attachment: Bug 1750253 - stop infinite file handling loops by checking if we're about to pass files to ourselves via the OS, r?mhowell,mak

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Alexandru Michis [:malexandru]]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Sandor Molnar[:smolnar]]

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

Comment 11

[:Gijs (he/him)]

(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

Comment 12

[:dhouse]

(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).

Comment 13

[:Gijs (he/him)]

(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.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/5a1e21232a3b

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 17

[Adrian Florinescu [:aflorinescu]]

(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 ?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Alice Wyman]

Since this is fixed in Firefox 98, I've updated https://support.mozilla.org/en-US/kb/firefox-repeatedly-opens-empty-tabs-or-windows