1884426 - Provide UI notification when the out-of-process file-picker crashes

Status: RESOLVED FIXED

(Core :: Widget: Win32, enhancement, P3)

Description

[Ray Kraesig [:rkraesig]]

When the out-of-process file-picker crashes, we currently restart in-process. If it wasn't an instacrash, this is likely to be confusing. If it was, it'll just bring down the browser.

We should probably notify the user instead, giving them an option to retry or (in the case of save-dialogs) to just save to a temporary or default location.

Comment 1

[Ray Kraesig [:rkraesig]]

(After some discussion, it seems likely that the UI provided won't be a retry UI specifically.)

Comment 2

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1884426 - [1/6] Extend error propagation slightly r?#win-reviewers!

Propagate error signal up into nsFilePicker::Open(), to unify error
handling between files and folders.

No functional changes (yet).

Comment 3

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1884426 - [2/5] Add method to get downloads directory r?Gijs

nsIExternalHelperAppService computes, but does not expose, the preferred
downloads directory's location. Expose it, for use by default-save-location
fallback code when the file-dialog fails.

Additionally, add a test to confirm that the value produced by this
method is, as the comment nearby implies, the same as that produced by
the eponymous method on DownloadIntegration.

Comment 4

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1884426 - [2/6] Write saved file to fallback location if possible r?#win-reviewers!

If the user is trying to save a file, don't discard that file on
failure; instead, write it to some default location on the filesystem.

Comment 5

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1884426 - [4/6] Add ForwardingObserver r?Gijs

Implement ForwardingObserver, which acts as a proxy observer-service.

If a message is sufficiently rare, it may be desirable to avoid loading
the module implementing its handling until actually necessary.
ForwardingObserver allows lazy registration of observers for such
events; one instead registers an observer-getter, which will not be
invoked until and unless the relevant message is sent.

(Credit to :Gijs for the suggestion.)

Comment 6

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1884426 - [3/6] Implement file-picker-crashed UI message r?Gijs!,#win-reviewers

Fire a signal (via the observer service) when the file-dialog crashes or
otherwise fails.

Set up a listener for that signal which reports the failure to the
end-user via the NotificationBox.

Comment 7

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1884426 - [4/4] Switch to only out-of-process on Nightly r?#win-reviewers!

Now that we have a mechanism to report file-dialog failure to the user,
turn off the in-process fallback by default.

(The in-process fallback is still available if enabled explicitly --
in which case the failure message won't be shown, unless by some miracle
the in-process file-dialog fails without crashing.)

Comment 8

[Phabricator Automation]

Comment on attachment 9408304 [details]
Bug 1884426 - [2/5] Add method to get downloads directory r?Gijs

Revision D214189 was moved to bug 1904160. Setting attachment 9408304 [details] to obsolete.

Comment 9

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1884426 - [5/6] Refactor AsyncExecute strategy-implementation code r?#win-reviewers

DRY up code slightly in preparation for adding a new strategy.

No functional changes.

Comment 10

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1884426 - [6/6] Add AsyncExecute strategy: retry locally except on crashes r?#win-reviewers!

... and set this as the new default strategy on Nightly.

Note that the file-picker failure UI will still be displayed if the
file-dialog encounters a nonfatal local failure as well.

Comment 11

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1884426 - [4/6] Add test for file-picker crash notification r?Gijs!,#firefox-desktop-core-reviewers!

Test that the file-picker-crashed notification appears when the
file-picker utility process is terminated with a file-picker dialog
active.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e0849feca9b
[1/6] Extend error propagation slightly  r=win-reviewers,gstoll
https://hg.mozilla.org/integration/autoland/rev/23bba9b337d7
[2/6] Write saved file to fallback location if possible  r=win-reviewers,gstoll
https://hg.mozilla.org/integration/autoland/rev/b0a5e530a5b4
[3/6] Implement file-picker-crashed UI message  r=Gijs,win-reviewers,fluent-reviewers,firefox-desktop-core-reviewers ,flod,gstoll
https://hg.mozilla.org/integration/autoland/rev/340686fd504b
[4/6] Add test for file-picker crash notification  r=Gijs,firefox-desktop-core-reviewers
https://hg.mozilla.org/integration/autoland/rev/d97d123115e5
[5/6] Refactor AsyncExecute strategy-implementation code  r=win-reviewers,gstoll
https://hg.mozilla.org/integration/autoland/rev/7635ab5e3dcf
[6/6] Add AsyncExecute strategy: retry locally except on crashes  r=win-reviewers,gstoll

Comment 13

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/6e0849feca9b
https://hg.mozilla.org/mozilla-central/rev/23bba9b337d7
https://hg.mozilla.org/mozilla-central/rev/b0a5e530a5b4
https://hg.mozilla.org/mozilla-central/rev/340686fd504b
https://hg.mozilla.org/mozilla-central/rev/d97d123115e5
https://hg.mozilla.org/mozilla-central/rev/7635ab5e3dcf

Comment 14

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1884426 - [7/6] Document filepicker async-execution strategies DONTBUILD r?#win-reviewers

As a followup to the content patches of a couple of months ago: document
the meaning of the values of AsyncExecute's enum Strategy, and in
particular the reasoning behind the last strategy's heuristic.

No functional changes; this patch only adds comments.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5fd7b37ae01c
[7/6] Document filepicker async-execution strategies  DONTBUILD  r=win-reviewers,gstoll

Comment 17

[Sandor Molnar[:smolnar]]

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