diff --git a/widget/windows/filedialog/WinFileDialogChild.cpp b/widget/windows/filedialog/WinFileDialogChild.cpp index cb0f8de08013..ddb972534a86 100644 --- a/widget/windows/filedialog/WinFileDialogChild.cpp +++ b/widget/windows/filedialog/WinFileDialogChild.cpp @@ -60,7 +60,7 @@ WinFileDialogChild::IPCResult WinFileDialogChild::RecvShowFileDialog( resolver(val.ResolveValue()); } else { auto err = val.RejectValue(); - resolver(RemoteError(err.where, err.why)); + resolver(RemoteError(err.where.Serialize(), err.why)); } }); @@ -81,7 +81,7 @@ WinFileDialogChild::IPCResult WinFileDialogChild::RecvShowFolderDialog( resolver(val.ResolveValue()); } else { auto err = val.RejectValue(); - resolver(RemoteError(err.where, err.why)); + resolver(RemoteError(err.where.Serialize(), err.why)); } }); diff --git a/widget/windows/filedialog/WinFileDialogCommands.cpp b/widget/windows/filedialog/WinFileDialogCommands.cpp index 61685b421c50..75abbad3435c 100644 --- a/widget/windows/filedialog/WinFileDialogCommands.cpp +++ b/widget/windows/filedialog/WinFileDialogCommands.cpp @@ -112,10 +112,12 @@ static HRESULT GetShellItemPath(IShellItem* aItem, nsString& aResultString) { } } // namespace -#define MOZ_ENSURE_HRESULT_OK(where, call_) \ - do { \ - HRESULT const _tmp_hr_ = (call_); \ - if (FAILED(_tmp_hr_)) return mozilla::Err(Error::Local(where, _tmp_hr_)); \ +#define MOZ_ENSURE_HRESULT_OK(where, call_) \ + do { \ + HRESULT const _tmp_hr_ = (call_); \ + if (FAILED(_tmp_hr_)) { \ + return mozilla::Err(MOZ_FD_LOCAL_ERROR(where, _tmp_hr_)); \ + } \ } while (0) mozilla::Result, Error> MakeFileDialog( @@ -159,11 +161,12 @@ mozilla::Result GetFileResults(::IFileDialog* dialog) { MOZ_ENSURE_HRESULT_OK("IFileDialog::GetResult", dialog->GetResult(getter_AddRefs(item))); if (!item) { - return Err(Error::Local("IFileDialog::GetResult: item", E_POINTER)); + return Err(MOZ_FD_LOCAL_ERROR("IFileDialog::GetResult: item", E_POINTER)); } nsAutoString path; - MOZ_ENSURE_HRESULT_OK("GetShellItemPath (1)", GetShellItemPath(item, path)); + MOZ_ENSURE_HRESULT_OK("GetFileResults: GetShellItemPath (1)", + GetShellItemPath(item, path)); return Results({path}, index); } @@ -173,14 +176,15 @@ mozilla::Result GetFileResults(::IFileDialog* dialog) { dialog->QueryInterface(IID_IFileOpenDialog, getter_AddRefs(openDlg)); if (!openDlg) { MOZ_ASSERT(false, "a file-save dialog was given FOS_ALLOWMULTISELECT?"); - return Err(Error::Local("Save + FOS_ALLOWMULTISELECT", E_UNEXPECTED)); + return Err(MOZ_FD_LOCAL_ERROR("Save + FOS_ALLOWMULTISELECT", E_UNEXPECTED)); } RefPtr items; MOZ_ENSURE_HRESULT_OK("IFileOpenDialog::GetResults", openDlg->GetResults(getter_AddRefs(items))); if (!items) { - return Err(Error::Local("IFileOpenDialog::GetResults: items", E_POINTER)); + return Err( + MOZ_FD_LOCAL_ERROR("IFileOpenDialog::GetResults: items", E_POINTER)); } nsTArray paths; @@ -193,7 +197,8 @@ mozilla::Result GetFileResults(::IFileDialog* dialog) { items->GetItemAt(idx, getter_AddRefs(item))); nsAutoString str; - MOZ_ENSURE_HRESULT_OK("GetShellItemPath (2)", GetShellItemPath(item, str)); + MOZ_ENSURE_HRESULT_OK("GetFileResults: GetShellItemPath (2)", + GetShellItemPath(item, str)); paths.EmplaceBack(str); } @@ -211,7 +216,7 @@ mozilla::Result GetFolderResults(::IFileDialog* dialog) { // might be due to misbehaving shell extensions? MOZ_ASSERT(false, "unexpected lack of item: was `Show`'s return value checked?"); - return Err(Error::Local("IFileDialog::GetResult: item", E_POINTER)); + return Err(MOZ_FD_LOCAL_ERROR("IFileDialog::GetResult: item", E_POINTER)); } // If the user chose a Win7 Library, resolve to the library's @@ -347,7 +352,7 @@ RefPtr> SpawnFileDialogThread(const char (&where)[N], nullptr, {.isUiThread = true}); if (NS_FAILED(rv)) { return Promise::CreateAndReject( - Error::Local("NS_NewNamedThread", (HRESULT)rv), where); + MOZ_FD_LOCAL_ERROR("NS_NewNamedThread", (HRESULT)rv), where); } } // `thread` is single-purpose, and should not perform any additional work @@ -460,7 +465,7 @@ auto SpawnPickerT(HWND parent, FileDialogType type, ExtractorF&& extractor, if (rv == HRESULT_FROM_WIN32(ERROR_CANCELLED)) { return ActionRetT{Nothing()}; } - return mozilla::Err(Error::Local("IFileDialog::Show", rv)); + return mozilla::Err(MOZ_FD_LOCAL_ERROR("IFileDialog::Show", rv)); } RetT res; diff --git a/widget/windows/filedialog/WinFileDialogCommands.h b/widget/windows/filedialog/WinFileDialogCommands.h index 9d5e215b3541..800ce832d551 100644 --- a/widget/windows/filedialog/WinFileDialogCommands.h +++ b/widget/windows/filedialog/WinFileDialogCommands.h @@ -83,25 +83,114 @@ struct Error { IPCError, }; + // "Enum" denoting error-location. Members are in `VALID_STRINGS`, and have no + // name other than their string. + // + // (Note: under C++20, this could reasonably be replaced with an `nsString` + // alongside a check that all constructors are either a) consteval or b) from + // IPC.) + class Location { + uint32_t value; + constexpr explicit Location(uint32_t value) : value(value) {} + + // Valid locations for errors. (Indices do not need to remain stable between + // releases; but -- where meaningful -- string values themselves should, for + // ease of telemetry-aggregation.) + constexpr static std::string_view const VALID_STRINGS[] = { + "ApplyCommands", + "CoCreateInstance(CLSID_ShellLibrary)", + "GetFileResults: GetShellItemPath (1)", + "GetFileResults: GetShellItemPath (2)", + "GetShellItemPath", + "IFileDialog::GetFileTypeIndex", + "IFileDialog::GetOptions", + "IFileDialog::GetResult", + "IFileDialog::GetResult: item", + "IFileDialog::Show", + "IFileOpenDialog::GetResults", + "IFileOpenDialog::GetResults: items", + "IPC", + "IShellItemArray::GetCount", + "IShellItemArray::GetItemAt", + "MakeFileDialog", + "NS_NewNamedThread", + "Save + FOS_ALLOWMULTISELECT", + "ShowFilePicker", + "ShowFolderPicker", + "ShowRemote: UtilityProcessManager::GetSingleton", + "ShowRemote: invocation of CreateWinFileDialogActor", + "UtilityProcessManager::CreateWinFileDialogActor", + "internal IPC failure?", + }; + constexpr static size_t VALID_STRINGS_COUNT = + std::extent_v; + + // Prevent duplicates from occurring in VALID_STRINGS by forcing it to be + // sorted. (Note that std::is_sorted is not constexpr until C++20.) + static_assert( + []() { + for (size_t i = 0; i + 1 < VALID_STRINGS_COUNT; ++i) { + if (!(VALID_STRINGS[i] < VALID_STRINGS[i + 1])) { + return false; + } + } + return true; + }(), + "VALID_STRINGS should be ASCIIbetically sorted"); + + public: + constexpr uint32_t Serialize() const { return value; } + constexpr static Location Deserialize(uint32_t val) { + return Location{val}; + } + + public: + constexpr static Location npos() { return Location{~uint32_t(0)}; } + + constexpr bool IsValid() const { return value < VALID_STRINGS_COUNT; } + + constexpr std::string_view ToString() const { + return value < VALID_STRINGS_COUNT ? VALID_STRINGS[value] + : ""; + } + constexpr static Location FromString(std::string_view str) { + for (uint32_t i = 0; i < VALID_STRINGS_COUNT; ++i) { + if (str == VALID_STRINGS[i]) return Location{i}; + } + return npos(); + } + + constexpr char const* c_str() const { return ToString().data(); } + }; + // Where and how (run-time) this error occurred. Kind kind; - // Where (compile-time) this error occurred. (Not functionally dynamic, unless - // of type `RemoteError` and child process has been subverted.) - nsCString where; + // Where (compile-time) this error occurred. + Location where; // Why (run-time) this error occurred. Probably an HRESULT. uint32_t why; // `impl Debug for Kind` static const char* KindName(Kind); - - template - static Error Local(const char (&where)[N], HRESULT why) { - return Error{.kind = LocalError, - .where = nsLiteralCString(where), - .why = (uint32_t)why}; - } }; +// Create a filedialog::Error, confirming at compile-time that the supplied +// where-string is valid. +#define MOZ_FD_ERROR(kind_, where_, why_) \ + ([](HRESULT why_arg_) -> ::mozilla::widget::filedialog::Error { \ + using Error = ::mozilla::widget::filedialog::Error; \ + constexpr static const Error::Location loc = \ + Error::Location::FromString(where_); \ + static_assert( \ + loc.IsValid(), \ + "filedialog::Error: location not found in Error::VALID_STRINGS"); \ + return Error{ \ + .kind = Error::kind_, .where = loc, .why = (uint32_t)why_arg_}; \ + }(why_)) + +// Create a filedialog::Error of kind LocalError (the usual case). +#define MOZ_FD_LOCAL_ERROR(where_, why_) MOZ_FD_ERROR(LocalError, where_, why_) + template using Promise = MozPromise; diff --git a/widget/windows/filedialog/WinFileDialogCommandsDefn.ipdlh b/widget/windows/filedialog/WinFileDialogCommandsDefn.ipdlh index 7e244b78a070..352b46df173d 100644 --- a/widget/windows/filedialog/WinFileDialogCommandsDefn.ipdlh +++ b/widget/windows/filedialog/WinFileDialogCommandsDefn.ipdlh @@ -46,12 +46,12 @@ struct Results { // Homolog of filedialog::Err. (Always Err::Kind::RemoteError, by definition.) struct RemoteError { - // A string describing the location where the error was detected. + // An enum (`filedialog::Error::Location`) describing the compile-time location + // where the error was detected. // - // This is an `nsCString` only for simplicity of serialization and data- - // transfer; it may go to telemetry, and so should only be constructed from - // fixed strings. - nsCString where; + // (This value is validated at use-sites; if the child process sends a bad + // value, nothing of import will happen.) + uint32_t where; // An error code describing the error itself more precisely. Its semantics // depend on the context provided by `where`, but it's probably an HRESULT. uint32_t why; diff --git a/widget/windows/filedialog/WinFileDialogParent.cpp b/widget/windows/filedialog/WinFileDialogParent.cpp index 5f6234103d1f..329c72cc94ef 100644 --- a/widget/windows/filedialog/WinFileDialogParent.cpp +++ b/widget/windows/filedialog/WinFileDialogParent.cpp @@ -61,35 +61,28 @@ static auto ConvertToFDPromise( Ex&& extractor, RefPtr> aSrcPromise) { - // The extractor must produce a `mozilla::Result<..., RemoteError>` from `T`. + // The extractor must produce a `mozilla::Result<..., Error>` from `T`. using SrcResultInfo = detail::DestructureResult>; using ResolveT = typename SrcResultInfo::OkT; - static_assert(std::is_same_v, - "expected T to be a Result<..., RemoteError>"); + static_assert(std::is_same_v, + "expected T to be a Result<..., Error>"); using SrcPromiseT = MozPromise; using DstPromiseT = MozPromise; RefPtr ret = aSrcPromise->Then( mozilla::GetCurrentSerialEventTarget(), aMethod, + [extractor, aMethod](T&& val) { - mozilla::Result result = - extractor(std::move(val)); + mozilla::Result result = extractor(std::move(val)); if (result.isOk()) { return DstPromiseT::CreateAndResolve(result.unwrap(), aMethod); } - - RemoteError err = result.unwrapErr(); - return DstPromiseT::CreateAndReject(Error{.kind = Error::RemoteError, - .where = err.where(), - .why = err.why()}, - aMethod); + return DstPromiseT::CreateAndReject(result.unwrapErr(), aMethod); }, [aMethod](typename mozilla::ipc::ResponseRejectReason&& val) { - return DstPromiseT::CreateAndReject(Error{.kind = Error::IPCError, - .where = "IPC"_ns, - .why = (uint32_t)val}, - aMethod); + return DstPromiseT::CreateAndReject( + MOZ_FD_ERROR(IPCError, "IPC", (uint32_t)val), aMethod); }); return ret; @@ -99,15 +92,18 @@ template struct Extractor { template static auto get() { - return [](Input&& res) -> Result { + return [](Input&& res) -> Result { if (res.type() == tag_) { return (res.*getter_)(); } if (res.type() == Input::TRemoteError) { - return Err(res.get_RemoteError()); + RemoteError err = res.get_RemoteError(); + return Err(Error{.kind = Error::RemoteError, + .where = Error::Location::Deserialize(err.where()), + .why = err.why()}); } MOZ_ASSERT_UNREACHABLE("internal IPC failure?"); - return Err(RemoteError("internal IPC failure?"_ns, (uint32_t)E_FAIL)); + return Err(MOZ_FD_ERROR(IPCError, "internal IPC failure?", E_FAIL)); }; } }; diff --git a/widget/windows/nsFilePicker.cpp b/widget/windows/nsFilePicker.cpp index 48ef3643ca1f..9a6110e1ad53 100644 --- a/widget/windows/nsFilePicker.cpp +++ b/widget/windows/nsFilePicker.cpp @@ -117,24 +117,20 @@ template RefPtr> { using RetPromise = FDPromise; - constexpr static const auto fail = - [](nsLiteralCString where, uint32_t why = 0) -> RefPtr { - return RetPromise::CreateAndReject( - Error{.kind = Error::LocalError, .where = std::move(where), .why = why}, - __PRETTY_FUNCTION__); - }; +// "function-local" #define +#define FAIL(where_, why_) \ + return RetPromise::CreateAndReject(MOZ_FD_LOCAL_ERROR(where_, why_), \ + __PRETTY_FUNCTION__) auto mgr = mozilla::ipc::UtilityProcessManager::GetSingleton(); if (!mgr) { MOZ_ASSERT(false); - return fail("ShowRemote: UtilityProcessManager::GetSingleton"_ns, - E_POINTER); + FAIL("ShowRemote: UtilityProcessManager::GetSingleton", E_POINTER); } auto wfda = mgr->CreateWinFileDialogActor(); if (!wfda) { - return fail("ShowRemote: invocation of CreateWinFileDialogActor"_ns, - E_POINTER); + FAIL("ShowRemote: invocation of CreateWinFileDialogActor", E_POINTER); } using mozilla::widget::filedialog::sLogFileDialog; @@ -161,9 +157,11 @@ static auto ShowRemote(ActionType&& action) -> RefPtr> { MOZ_LOG(sLogFileDialog, LogLevel::Error, ("could not acquire WinFileDialog: %zu", size_t(error))); // TODO: pipe more data up from utility-process creation - return fail("UtilityProcessManager::CreateWinFileDialogActor"_ns, - (uint32_t)error); + FAIL("UtilityProcessManager::CreateWinFileDialogActor", + (uint32_t)error); }); + +#undef FAIL } namespace { @@ -403,7 +401,7 @@ static auto AsyncExecute(Fn1 local, Fn2 remote, Args const&... args) -> MOZ_ASSERT(err.kind == Error::LocalError); MOZ_LOG(filedialog::sLogFileDialog, LogLevel::Info, ("local file-dialog failed: where=%s, why=%08" PRIX32, - err.where.get(), err.why)); + err.where.c_str(), err.why)); return Ok(); }); } @@ -414,7 +412,7 @@ static auto AsyncExecute(Fn1 local, Fn2 remote, Args const&... args) -> MOZ_LOG( filedialog::sLogFileDialog, LogLevel::Info, ("remote file-dialog failed: kind=%s, where=%s, why=%08" PRIX32, - Error::KindName(err.kind), err.where.get(), err.why)); + Error::KindName(err.kind), err.where.c_str(), err.why)); return Ok(); });