1858225 - Async Win32 file-picker

Status: RESOLVED FIXED

(Core :: Widget: Win32, enhancement)

Description

[Ray Kraesig [:rkraesig]]

Our internal file-picker interface is already mostly async. However, the Win32 file dialog itself is synchronous and modal; it will need to be placed on a separate thread to avoid blocking the main event loop.

(This is formally orthogonal to moving it out-of-process, per bug 1837079, but the two tasks touch enough of the same code that in practice they have to be sequenced somehow.)

Comment 1

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1858225 - [0/9] drive-by: add FileDialog to Windows profiler preset r?#profiler-reviewers!

Comment 2

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1858225 - [1/9] rename CreateWinFileDialogAsync to CreateWinFileDialogActor r?#ipc-reviewers!

We're about to add some code that will actually create a file dialog,
rather than just the actor, asynchronously. Rename this function to make
it clear which of those it does.

(That it does so asynchronously is probably sufficiently communicated by
the return type.)

Depends on D193732

Comment 3

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1858225 - [2/9] inline `nsFilePicker::...Impl()` functions r?#win-reviewers!

These functions are essentially sprues, left over from earlier
development. Trim them away.

No functional changes.

Depends on D193733

Comment 4

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1858225 - [3/9] introduce "local" async-filepicker implementation functions r?handyman!,#win-reviewers!

Introduce functions which create and invoke the Windows file-picker in a
separate thread.

Technically, no functional changes: these functions aren't actually
invoked yet. (That will occur in an upcoming commit.)

Depends on D193734

Comment 5

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1858225 - [4/9] use alternate-thread file picker in remote process r?#win-reviewers!

When opening a file picker in a utility process, do so off the utility
process's main thread, allowing multiple dialogs to resolve in parallel.

The relevant IPC calls are already formally async, so this suffices to
convert them to being functionally async as well. (While this is
technically a functional change, it's not yet a user-visible one: the
parent-process code is still synchronous.)

Depends on D193735

Comment 6

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1858225 - [5/9] use local-async functions for local calls r?#win-reviewers!

When opening a file dialog locally (that is, in the parent process), do
so asynchronously on a separate thread.

Since the calling code is still synchronous, this necessitates adding
new calls to ImmorallyDrivePromiseToCompletion at the sync-to-async
boundary. These are very temporary -- they'll be removed before this
patchset is done.

Depends on D193736

Comment 7

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1858225 - [6/9] hoist syncification one layer up r?#win-reviewers!

Pull the sync-to-async boundary up one function-call layer, from
ShowXxxPicker{Local,Remote} to ShowXxxPicker. This coalesces the
remote and local sync-to-async boundaries.

Additionally, standardize on exclusive MozPromises, since none of them
happen to need multiple listeners.

This is arguably, but not practically, a functional change, as a tiny
bit of glue code is now executed on a different thread.

(N.B.: While this reduces the total number of sync-to-async boundaries
back down to two, this is not the removal promised in the previous
commit summary.)

Depends on D193737

Comment 8

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1858225 - [7/9] hoist syncification one more layer r?#win-reviewers!

Pull the sync-to-async boundary up another function-call layer. This
unifies the file- and folder-picker sync-to-async boundaries.

Again, no functional changes except for the migration of a tiny bit of
glue code.

Depends on D193738

Comment 9

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1858225 - [8/9] cancel out sync/async transitions r?#win-reviewers!

Pull the async-to-sync transition down one (or two) class-layers, while
pulling the sync-to-async transition up one more function layer. This
allows them to meet in a single function -- where they cleanly annihilate.

This is a significant functional change, as it leaves the file-dialog
implementation fully asynchronous on Windows! \o/

Depends on D193739

Comment 10

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1858225 - [9/9] remove async-to-sync code from Windows build r?#win-reviewers!

This is no longer needed on Windows, so #ifdef it out.

Depends on D193740

Comment 11

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1858225 - [10/10] separate pref-decoding from invocation r?handyman,#win-reviewers!

For clarity, separate out decoding what the preference says to do (which
is not as trivial as might be desired) from actually doing it (ditto).

Depends on D193741

Comment 12

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1858225 - [2/2] Add lint for ApartmentRegion and derivatives r?handyman!

Add a lint (paralleling the existing lint for CoInitializeEx) for new
uses of the utility classes defined in ApartmentRegion.h.

Depends on D194302

Comment 13

[Phabricator Automation]

Comment on attachment 9364851 [details]
Bug 1858225 - [2/2] Add lint for ApartmentRegion and derivatives r?handyman!

Revision D194303 was moved to bug 1865867. Setting attachment 9364851 [details] to obsolete.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82b7b851c429
[0/9] drive-by: add FileDialog to Windows profiler preset  r=profiler-reviewers,julienw
https://hg.mozilla.org/integration/autoland/rev/c31dfa5f9be5
[1/9] rename CreateWinFileDialogAsync to CreateWinFileDialogActor  r=win-reviewers,mhowell,gerard-majax
https://hg.mozilla.org/integration/autoland/rev/2eb38cba55d6
[2/9] inline `nsFilePicker::...Impl()` functions  r=win-reviewers,mhowell
https://hg.mozilla.org/integration/autoland/rev/6aacb8fde7e1
[3/9] introduce "local" async-filepicker implementation functions  r=handyman,win-reviewers,mhowell
https://hg.mozilla.org/integration/autoland/rev/9b2c6eaf137e
[4/9] use alternate-thread file picker in remote process  r=win-reviewers,handyman,mhowell
https://hg.mozilla.org/integration/autoland/rev/f57ac4a387f2
[5/9] use local-async functions for local calls  r=win-reviewers,mhowell
https://hg.mozilla.org/integration/autoland/rev/9b1f0d37b74b
[6/9] hoist syncification one layer up  r=win-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/61c8a62b47c5
[7/9] hoist syncification one more layer  r=win-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/d1efbf517c7e
[8/9] cancel out sync/async transitions  r=win-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/e6f456e40dda
[9/9] remove async-to-sync code from Windows build  r=win-reviewers,mhowell

Comment 15

[Ray Kraesig [:rkraesig]]

Attachment: Bug 1858225 - [10/9] add thread name to permitted-threads list r?cosmins,handyman

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/808f68da4438
[10/9] add thread name to permitted-threads list. r=bustage-fix CLOSED TREE

Comment 17

[Cosmin Sabou [:CosminS]]

Backed out for causing build bustages on nsFilePicker.cpp.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=e6f456e40dda9f799c2fef84edc219d26631e268&selectedTaskRun=CKMWPR2rQnSy9P4Ov4bHDw.0

Failure logs:

• https://treeherder.mozilla.org/logviewer?job_id=438369662&repo=autoland
• https://treeherder.mozilla.org/logviewer?job_id=438379399&repo=autoland

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

Comment 18

[Ray Kraesig [:rkraesig]]

Reresolved the misresolved merge conflict, then ran git rebase -i central --exec './mach build' with the clang-plugin active; all green.

A current try-job is also green so far, except for known intermittents. Attempting a reland shortly...

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b56a5c339c3
[0/9] drive-by: add FileDialog to Windows profiler preset  r=profiler-reviewers,julienw
https://hg.mozilla.org/integration/autoland/rev/54e12c84d094
[1/9] rename CreateWinFileDialogAsync to CreateWinFileDialogActor  r=win-reviewers,mhowell,gerard-majax
https://hg.mozilla.org/integration/autoland/rev/32f455e67029
[2/9] inline `nsFilePicker::...Impl()` functions  r=win-reviewers,mhowell
https://hg.mozilla.org/integration/autoland/rev/1c7b0f8d1e8d
[3/9] introduce "local" async-filepicker implementation functions  r=handyman,win-reviewers,mhowell
https://hg.mozilla.org/integration/autoland/rev/1a25d0d1161b
[4/9] use alternate-thread file picker in remote process  r=win-reviewers,handyman,mhowell
https://hg.mozilla.org/integration/autoland/rev/d08b97ae0098
[5/9] use local-async functions for local calls  r=win-reviewers,mhowell
https://hg.mozilla.org/integration/autoland/rev/e66d99182863
[6/9] hoist syncification one layer up  r=win-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/735ce96c3325
[7/9] hoist syncification one more layer  r=win-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/1c6cd3fa8c52
[8/9] cancel out sync/async transitions  r=win-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/3e082a66c721
[9/9] remove async-to-sync code from Windows build  r=win-reviewers,mhowell

Comment 20

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/6b56a5c339c3
https://hg.mozilla.org/mozilla-central/rev/54e12c84d094
https://hg.mozilla.org/mozilla-central/rev/32f455e67029
https://hg.mozilla.org/mozilla-central/rev/1c7b0f8d1e8d
https://hg.mozilla.org/mozilla-central/rev/1a25d0d1161b
https://hg.mozilla.org/mozilla-central/rev/d08b97ae0098
https://hg.mozilla.org/mozilla-central/rev/e66d99182863
https://hg.mozilla.org/mozilla-central/rev/735ce96c3325
https://hg.mozilla.org/mozilla-central/rev/1c6cd3fa8c52
https://hg.mozilla.org/mozilla-central/rev/3e082a66c721