1886327 - Improve error propagation for out-of-process file-picker failures

Status: RESOLVED FIXED

(Core :: Widget: Win32, task)

Description

[Ray Kraesig [:rkraesig]]

In order to implement either of bug 1884221 or bug 1884226, we'll need to propagate errors from the child process in such a way that they carry more data about their source and nature.

Comment 1

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1886327 - [1/5] wrap Send* calls in FilePickerParent r?#win-reviewers

... in preparation for more interesting changes surrounding the error type.

No functional changes.

Comment 2

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1886327 - [2/5] hide ShowFilePickerRemote _et al._ r?#win-reviewers

The Show{File,Folder}Picker{Local,Remote} functions don't depend on
anything in nsFilePicker itself except the mostly-vestigial promise
type. Pull all of them into nsFilePicker.cpp as static functions in an
anonymous namespace.

No functional changes.

Comment 3

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1886327 - [3/5] rework promise type to carry metadata r?#win-reviewers

Replace most promise-types in widget/windows/filedialog with something
capable of carrying the initial cause and location of a failure.

This does not properly convey that metadata across the IPDL connection
where possible; that will be done in an upcoming commit.

Comment 4

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1886327 - [4/5] propagate ordinary filepicker errors through IPDL r?#win-reviewers,#ipc-reviewers

Don't crash the file-picker host process just because a nonfatal failure
occurred. Instead, report the failure location and code over the IPDL
interface.

Comment 5

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1886327 - [5/5] serialize error locations as constant numeric values r?#win-reviewers,#ipc-reviewers

Instead of passing potentially-dynamic strings over the IPDL interface,
pass an integer which corresponds to one of a fixed set of static
strings. (This is basically an enum with more steps -- or with fewer
steps, depending on which side of it you're standing on.)

This ensures that the set of possible location-strings is fixed, which
in turn both ensures that aggregation of telemetry stays relatively
simple and guards against accidental leakage of PII thereinto. (Or,
alternatively and equivalently, this minimizes the code region that
data-review needs to consider once the location string is reported to
telemetry.)

Note that we do not yet actually report the location-string via
telemetry; that's planned to occur in bug 1884221.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42a620278be5
[1/5] wrap Send* calls in FilePickerParent  r=win-reviewers,gstoll
https://hg.mozilla.org/integration/autoland/rev/bf4c746f5100
[2/5] hide ShowFilePickerRemote _et al._  r=win-reviewers,gstoll
https://hg.mozilla.org/integration/autoland/rev/280a41bf4b09
[3/5] rework promise type to carry metadata  r=win-reviewers,gstoll
https://hg.mozilla.org/integration/autoland/rev/88fea5453f0c
[4/5] propagate ordinary filepicker errors through IPDL  r=win-reviewers,gstoll
https://hg.mozilla.org/integration/autoland/rev/6a497992821d
[5/5] serialize error locations as constant numeric values  r=win-reviewers,gstoll

Comment 7

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/42a620278be5
https://hg.mozilla.org/mozilla-central/rev/bf4c746f5100
https://hg.mozilla.org/mozilla-central/rev/280a41bf4b09
https://hg.mozilla.org/mozilla-central/rev/88fea5453f0c
https://hg.mozilla.org/mozilla-central/rev/6a497992821d