1737711 - With browser.download.improvements_to_download_panel=true, ICS attachments can no longer be handled by Thunderbird - TEST-UNEXPECTED-FAIL | comm/calendar/test/browser/invitations/browser_icsAttachment.js

Status: VERIFIED FIXED

(Thunderbird :: Upstream Synchronization, defect)

Description

[Geoff Lankow (:darktrojan)]

browser_icsAttachment.js is broken: if you click to open an ICS attachment, a Save As file picker appears. It doesn't ask if the setting is "Always ask", or use Thunderbird or another application if the setting is "Use …".

We do force Content-Disposition: attachment on attachments, and that may be part of the problem, although I tried not doing that and nothing changed.

Comment 1

[Geoff Lankow (:darktrojan)]

Also mail/test/browser/attachment/browser_attachment.js is failing, same problem.

Comment 2

[:Gijs (out for now; he/him)]

I expect this goes away if you set a download directory in the prefs. We're asking for a location to put the saved file, opening the file happens after that point. I'm not really clear on whether that's a problem for Thunderbird or just a surprise for tests.

Comment 3

[Geoff Lankow (:darktrojan)]

We're not wanting to save the file at all, just open it (in this case offer to import the contents to a calendar) like we do with PDFs.

(Side note, browser_attachment.js isn't a problem, we can just change the test as it's expecting the unknown content type dialog but not actually testing that behaviour.)

Comment 4

[Geoff Lankow (:darktrojan)]

Saving the file with the "Use Thunderbird" setting only saves the file, it doesn't do anything with it once saved.

Comment 5

[:Gijs (out for now; he/him)]

(In reply to Geoff Lankow (:darktrojan) from comment #3)

We're not wanting to save the file at all, just open it (in this case offer to import the contents to a calendar) like we do with PDFs.

I'm sorry, this doesn't make sense to me without a lot more context. The comment you linked to in comment 0 specifically says that TB wants to open rather than display (and therefore sets CD: attachment). With the pref off, how does this work - surely you're opening it from disk, so the file is saved to disk? Because if it's showing the unknown content type dialog, we're already in the process of saving to disk...

Anyway, preserving existing behaviour should be possible by setting the alwaysAskBeforeHandling bool to true on the relevant mime info, though it does sound wrong if the preferred action is set to "Use ..." and that doesn't happen after saving to disk. You've also not really clarified if you changed "always ask me where to save files" to a directory or if you just tried to pick something in the picker... (cf bug 1719901 and friends)

For 95, we're not shipping this change (it's nightly only), so we have some time to figure this out. I'm happy to help, but atm there isn't enough context in this bug to understand what TB is doing, what the state of prefs is when it's trying to do that, and how the changes would have impacted TB at all considering those. Given the existing test coverage in Firefox, and how much work we put into making sure we tested both pref on / pref off stuff and didn't break anything in the process, the implied claims that literally everything about this is broken are pretty confusing.

Comment 6

[Geoff Lankow (:darktrojan)]

To be clear, I'm not saying that any of the changes you have made are wrong. IME it's quite likely there are one or more places where Thunderbird does something stupid. I don't know, at this point I'm just here because the tests are failing and we need somewhere to deal with that.

I'm sorry, this doesn't make sense to me without a lot more context. The comment you linked to in comment 0 specifically says that TB wants to open rather than display (and therefore sets CD: attachment). With the pref off, how does this work - surely you're opening it from disk, so the file is saved to disk? Because if it's showing the unknown content type dialog, we're already in the process of saving to disk...

Okay, you're right, we were saving it to a temporary file and then opening it again. Now the only available option (with handlers.json cleared out) is to save the file and not open it again. If I've got "always ask me where to save files" selected, it does that, otherwise it saves to the chosen directory, as expected.

I can't see anywhere that we register an nsIMimeInfo for the ICS type. It seems that we should do that.

Comment 7

[:Gijs (out for now; he/him)]

Does Thunderbird surface downloads / saved items anywhere at all? I don't see any in my regular release build, but perhaps I've just always been unaware of it. Re-reading the comments here, I wonder if the code responsible for launching the download lives in toolkit/components/downloads/DownloadIntegration.jsm and if that code is just not running for TB? That would explain some of what we're seeing here. It would also mean that the new default (saving files to disk rather than prompting) means that as far as the user is concerned "nothing happens" - because there is no UI that allows opening the file after it's been saved to disk. That doesn't sound very good...

We now default to not showing the prompt anymore in Nightly (when the improvements pref is turned on). If we can't easily get the DownloadIntegration code to run and/or to provide some UI here, perhaps we'll need a separate way for TB to opt into the "always ask" behaviour here. (We intend to eventually get rid of the improvements pref because we don't want to indefinitely maintain 2 codepaths.)

For reference, there are some user-facing docs for the Nightly changes that might be useful in understanding what changed here.

Comment 8

[Geoff Lankow (:darktrojan)]

Does Thunderbird surface downloads / saved items anywhere at all? I don't see any in my regular release build, but perhaps I've just always been unaware of it.

Yes, you can open about:downloads by going to Saved Files on the Tools menu.

Re-reading the comments here, I wonder if the code responsible for launching the download lives in toolkit/components/downloads/DownloadIntegration.jsm and if that code is just not running for TB? That would explain some of what we're seeing here. It would also mean that the new default (saving files to disk rather than prompting) means that as far as the user is concerned "nothing happens" - because there is no UI that allows opening the file after it's been saved to disk. That doesn't sound very good...

Plausible. If I save a file and then double-click on it in about:downloads, it does open.

We now default to not showing the prompt anymore in Nightly (when the improvements pref is turned on). If we can't easily get the DownloadIntegration code to run and/or to provide some UI here, perhaps we'll need a separate way for TB to opt into the "always ask" behaviour here. (We intend to eventually get rid of the improvements pref because we don't want to indefinitely maintain 2 codepaths.)

For reference, there are some user-facing docs for the Nightly changes that might be useful in understanding what changed here.

I understand a bit better now. In Thunderbird the desired action is as likely to be "open" as it is "save" and I think if a user wants to open an attachment then they won't be expecting the file saved anywhere, (even though we always have written out the file to a temporary place because programs need a real file to open).

I'm starting to think we need to handle attachment opening separately. At the moment we just give the docshell a URL and let it figure out what to do. (Probably only because it worked and was convenient at the time.) Perhaps we should ask using our own UI, and in the "open" case do the writing to disk and launching, only giving the URL to docshell in the "save" case.

Comment 9

[:Gijs (out for now; he/him)]

(In reply to Geoff Lankow (:darktrojan) from comment #8)

I understand a bit better now. In Thunderbird the desired action is as likely to be "open" as it is "save" and I think if a user wants to open an attachment then they won't be expecting the file saved anywhere, (even though we always have written out the file to a temporary place because programs need a real file to open).

Yep, that makes sense.

I'm starting to think we need to handle attachment opening separately. At the moment we just give the docshell a URL and let it figure out what to do. (Probably only because it worked and was convenient at the time.) Perhaps we should ask using our own UI, and in the "open" case do the writing to disk and launching, only giving the URL to docshell in the "save" case.

Hm. You could potentially invoke the same dialog using the relevant XPCOM contract? Then you don't have to reinvent too many wheels. The docshell code ends up in nsExternalHelperAppService, which creates the dialog here: https://searchfox.org/mozilla-central/rev/f351e19360729b351bfc7c1386d6e4ca4ea676e2/uriloader/exthandler/nsExternalHelperAppService.cpp#1914-1920 . You'd want to pass a custom launcher and dialog parent with the right mime info etc, but it's probably easier than rolling everything yourself? That would also give you an opportunity to make your own decisions wrt bug 453455 which I know is seeing a lot of TB dupes at the moment. The pref flip takes care of that bug in Firefox, but I agree that it looks like TB would benefit from different UX here.

Comment 11

[ISHIKAWA, Chiaki]

Bug 1682772
comm/mail/test/browser/composition/browser_attachment.js is failing due to similar reason discussed here.
I am saying this because I see the same dialog that gets stuck in this and the other test.

Oh, already known in comment 1.

Comment 12

[ISHIKAWA, Chiaki]

(In reply to ISHIKAWA, Chiaki from comment #11)

Bug 1682772
comm/mail/test/browser/composition/browser_attachment.js is failing due to similar reason discussed here.
I am saying this because I see the same dialog that gets stuck in this and the other test.

Oh, already known in comment 1.

Wait. I thought I was seeing exactly the same dialog in this and the other test that got stuck. But comment 5 and/or comment 6 suggests otherwise?

TB seems to ask for the file to SAVE and that is where it stopped proceeding.
You can see that in https://bugzilla.mozilla.org/show_bug.cgi?id=1682772#c15, I attached the screen dump of the stuck dialog which even has a file name in the input line of the file chooser.
So something is not doing it right from there.

Comment 13

[ISHIKAWA, Chiaki]

Is there a way to skip this test and another test
comm/mail/test/browser/composition/browser_attachment.js
until the problem is sorted out when I locally run |mach mochitest|?

I am trying to run TB under valgrind during mochitest and due to the slowdown of execution, I set certain timeout values in mach and other python test codes to 3600 seconds.
This and the other test hit this timeout unfortunately, meaning that they spend two hours in total until TB times out.
Right now, on my local machine, |mach mochitst| as a whole takes 240 minutes to finish (without valgrind), meaning that this and the other test consume two hours out of four hours, exactly half the elapsed time.
Obviously, it would be great if can halve the total execution time by half.

Comment 14

[Magnus Melin [:mkmelin]]

Yes, just remove them from browser.ini - https://searchfox.org/comm-central/source/mail/test/browser/attachment/browser.ini#12

Comment 15

[ISHIKAWA, Chiaki]

(In reply to Magnus Melin [:mkmelin] from comment #14)

Yes, just remove them from browser.ini - https://searchfox.org/comm-central/source/mail/test/browser/attachment/browser.ini#12

Thank you. Will do. :-)

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1737711 - Handle opening of attachments ourselves. r=mkmelin

Comment 17

[Intermittent Failures Robot]

7 failures in 3030 pushes (0.002 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 7

Platform and build breakdown:

• linux64: 2
  • debug: 1
  • opt: 1
• linux64-shippable: 2
  • opt: 2
• macosx1015-64: 1
  • opt: 1
• macosx1015-64-shippable: 2
  • opt: 2

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1737711&startday=2021-11-01&endday=2021-11-07&tree=all

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f83d2faebb77
Handle opening of attachments ourselves. r=mkmelin

Comment 19

[Geoff Lankow (:darktrojan)]

We'll see how this goes then consider taking it back to bug 453455. I think the test behaves slightly differently with the pref off.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/876f7809442b
follow-up - Fix test on Windows. rs=me

Comment 21

[ISHIKAWA, Chiaki]

(In reply to ISHIKAWA, Chiaki from comment #12)

(In reply to ISHIKAWA, Chiaki from comment #11)

Bug 1682772
comm/mail/test/browser/composition/browser_attachment.js is failing due to similar reason discussed here.
I am saying this because I see the same dialog that gets stuck in this and the other test.

Oh, already known in comment 1.

Wait. I thought I was seeing exactly the same dialog in this and the other test that got stuck. But comment 5 and/or comment 6 suggests otherwise?

TB seems to ask for the file to SAVE and that is where it stopped proceeding.
You can see that in https://bugzilla.mozilla.org/show_bug.cgi?id=1682772#c15, I attached the screen dump of the stuck dialog which even has a file name in the input line of the file chooser.
So something is not doing it right from there.

With the fix, the test in bug 1682772

comm/mail/test/browser/composition/browser_attachment.js
no longer gets stuck but runs successfully. (My early post about a bug was a premature post. It seems if I change browser.ini, I have to re-run
|make configure| to propagate the proper change of browser.ini? Anyway, more in bug 1682772.

Comment 22

[Thomas D. (:thomas8)]

We want to uplift this asap as this fixes popular Bug 1690395 (14 TB dupes), per Geoff's comment 61 there.

Comment 23

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1737711 - Test improvements. r=mkmelin

In backporting this test I discovered some mistakes and some things that could be done to improve it.
Earlier versions save some information based on the extension rather than the content type, so I've given each attachment a unique extension.

Comment 24

[Geoff Lankow (:darktrojan)]

Attachment: 1737711-open-attachment-beta.diff

This patch is all of the changes so far in this bug, for beta, and I think it will also work for ESR. There are slight differences in the test due to the fact we're patching a different version of the Gecko code. Most notably I've dropped the last two sub-tests (which I already called "weird but plausible") because they don't work in the test environment.

[Approval Request Comment]
Regression caused by (bug #): bug 1732347
User impact if declined: on beta and ESR, this fixes the "do this automatically" check box
Testing completed (on c-c, etc.): landed earlier this week
Risk to taking this patch (and alternatives if risky): moderate – this (almost) completely replaces the way we've opened attachments for years and years. There's potential for side-effects I haven't thought of. I'd like it to have as much time on beta as we can give it.

Comment 25

[Wayne Mery (:wsmwk)]

Comment on attachment 9250129 [details] [diff] [review]
1737711-open-attachment-beta.diff

[Triage Comment]
Approved for beta

Comment 26

[Rachel Martin]

Does this improve the situation in bug 1738490?

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/74e2340b2729
Test improvements. r=mkmelin

Comment 28

[Rob Lemley [:rjl]]

Thunderbird 95.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/fe6f9fd6c97f

Comment 29

[WaltS48 [:walts48]]

Verified importing the only saved .ics calendar I have from 2014-2017 into 95.0b3 on Windows 10.

Comment 30

[Intermittent Failures Robot]

1 failures in 2723 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 1

Platform and build breakdown:

• linux64: 1
  • opt: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=1737711&startday=2021-11-08&endday=2021-11-14&tree=all

Comment 31

[Geoff Lankow (:darktrojan)]

Walt, despite the title this bug is more about opening attachments (any file type) from email and choosing what to do with them. (ICS calendar files are a bit of a special case because we handle those ourselves.) I'd appreciate if you could give the attachment opening process a bit of a workout, as I'm a bit nervous about this going to 91. All behaviours should be as they were before except the "do this every time" checkbox should work now.

Comment 32

[WaltS48 [:walts48]]

Okay, I sent myself a .jpg, .wav, .wmv and .mp3 file from my main account to my Gmail and AOL accounts using Thunderbird 95.0b3 on Windows 10.

All the attachments opened in the expected application without repeated prompting. The "do this every time" checkbox is working.

Comment 33

[Rachel Martin]

(In reply to Geoff Lankow (:darktrojan) from comment #24)

Risk to taking this patch (and alternatives if risky): moderate – this (almost) completely replaces the way we've opened attachments for years and years. There's potential for side-effects I haven't thought of. I'd like it to have as much time on beta as we can give it.

Early warning: There are reports here
https://www.thunderbird-mail.de/forum/thread/88446-anh%C3%A4nge-werden-nicht-ge%C3%B6ffnet-betterbird/
that opening attachments with external programs doesn't work in TB 95 beta 3 and our fork which is already shipping the patch to fix bug 1738490. We can't reproduce the reports. So there will be a small set of users running into problems.

Comment 34

[Rachel Martin]

This has been reported as bug 1741510.

Comment 35

[Wayne Mery (:wsmwk)]

Geoff,
We're prepared to take an uplift for 91.4.0

Comment 36

[Geoff Lankow (:darktrojan)]

Comment on attachment 9250129 [details] [diff] [review]
1737711-open-attachment-beta.diff

Okay, let's do this. Targeting 91.4.1. Needs bug 1741820.

[Approval Request Comment]
Regression caused by (bug #): bug 1732347
User impact if declined: on beta and ESR, this fixes the "do this automatically" check box
Testing completed (on c-c, etc.): went into 95.0b3
Risk to taking this patch (and alternatives if risky): moderate

Comment 37

[Wayne Mery (:wsmwk)]

Comment on attachment 9250129 [details] [diff] [review]
1737711-open-attachment-beta.diff

[Triage Comment]
Approved for esr91

Comment 38

[Rachel Martin]

Please note the regression in bug 1744709.

Comment 39

[Rob Lemley [:rjl]]

Thunderbird 91.4.1:
https://hg.mozilla.org/releases/comm-esr91/rev/c399335b8f75