Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2024-12-03 02:25:34 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Bug 1886327 - [5/5] serialize error locations as constant numeric values r=win-reviewers,gstoll

Browse Source 
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.

Differential Revision: https://phabricator.services.mozilla.com/D205169
This commit is contained in:
Ray Kraesig 2024-04-25 20:42:50 +00:00
parent 45ac071366
commit 5018b1ee50
6 changed files with 149 additions and 61 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
widget/windows/filedialog/WinFileDialogChild.cpp
View File

@ -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));
}
});

29
widget/windows/filedialog/WinFileDialogCommands.cpp
View File

@ -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<RefPtr<IFileDialog>, Error> MakeFileDialog(
@ -159,11 +161,12 @@ mozilla::Result<Results, Error> 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<Results, Error> 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<IShellItemArray> 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<nsString> paths;
@ -193,7 +197,8 @@ mozilla::Result<Results, Error> 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<nsString, Error> 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<Promise<Res>> SpawnFileDialogThread(const char (&where)[N],
nullptr, {.isUiThread = true});
if (NS_FAILED(rv)) {
return Promise<Res>::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;

109
widget/windows/filedialog/WinFileDialogCommands.h
View File

@ -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<decltype(VALID_STRINGS)>;
// 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]
: "<bad filedialog::Error::Location?>";
}
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 <size_t N>
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 <typename R>
using Promise = MozPromise<R, Error, true>;

10
widget/windows/filedialog/WinFileDialogCommandsDefn.ipdlh
View File

@ -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;

32
widget/windows/filedialog/WinFileDialogParent.cpp
View File

@ -61,35 +61,28 @@ static auto ConvertToFDPromise(
Ex&& extractor,
RefPtr<MozPromise<T, mozilla::ipc::ResponseRejectReason, true>>
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<std::invoke_result_t<Ex, T>>;
using ResolveT = typename SrcResultInfo::OkT;
static_assert(std::is_same_v<typename SrcResultInfo::ErrorT, RemoteError>,
"expected T to be a Result<..., RemoteError>");
static_assert(std::is_same_v<typename SrcResultInfo::ErrorT, Error>,
"expected T to be a Result<..., Error>");
using SrcPromiseT = MozPromise<T, mozilla::ipc::ResponseRejectReason, true>;
using DstPromiseT = MozPromise<ResolveT, Error, true>;
RefPtr<DstPromiseT> ret = aSrcPromise->Then(
mozilla::GetCurrentSerialEventTarget(), aMethod,
[extractor, aMethod](T&& val) {
mozilla::Result<ResolveT, RemoteError> result =
extractor(std::move(val));
mozilla::Result<ResolveT, Error> 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 <typename Input, typename Output>
struct Extractor {
template <typename Input::Type tag_, Output const& (Input::*getter_)() const>
static auto get() {
return [](Input&& res) -> Result<Output, RemoteError> {
return [](Input&& res) -> Result<Output, Error> {
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));
};
}
};

26
widget/windows/nsFilePicker.cpp
View File

@ -117,24 +117,20 @@ template <typename ActionType,
static auto ShowRemote(ActionType&& action) -> RefPtr<FDPromise<ReturnType>> {
using RetPromise = FDPromise<ReturnType>;
constexpr static const auto fail =
[](nsLiteralCString where, uint32_t why = 0) -> RefPtr<RetPromise> {
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<FDPromise<ReturnType>> {
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();
});