Bug 1686191 - Remove module argument from LogError. r=dom-workers-and-storage-reviewers,janv

The module argument in LogError is redundant: the module information can
already be determined from the source file path. Indeed, reporting the module
separately in the telemetry events seems unnecessary. Instead, the relative
path is now reported. This is what the analysis of the telemetry data did
reconstruct anyway.

As a consequence, it's no longer necessary to define module-specific
HandleError functions, and therefore it's also unnecessary to define
module-specific TRY macros. These are retained as simple aliases for now,
but can be removed in a later patch entirely.

This also avoids misuses of a TRY macros in the wrong module, which were
happening a few times before, resulting in confusing output or telemetry
events.

Since Bug 1686191 will add some more TRY macro variants that warn, so
simplifying the module-specific aspects will simplify that task. Furthermore,
this is in preparation of moving the TRY macro extensions to MFBT.

Differential Revision: https://phabricator.services.mozilla.com/D102767
This commit is contained in:
Simon Giesecke 2021-03-12 15:01:21 +00:00
Родитель 2fb6095f16
Коммит fbf7a1dad4
8 изменённых файлов: 171 добавлений и 156 удалений

33
dom/cache/CacheCommon.h поставляемый
Просмотреть файл

@ -9,32 +9,11 @@
#include "mozilla/dom/quota/QuotaCommon.h"
// Cache equivalents of QM_TRY.
#define CACHE_TRY_GLUE(...) \
QM_TRY_META(mozilla::dom::cache, MOZ_UNIQUE_VAR(tryResult), ##__VA_ARGS__)
#define CACHE_TRY(...) CACHE_TRY_GLUE(__VA_ARGS__)
// Cache equivalents of QM_TRY_UNWRAP and QM_TRY_INSPECT.
#define CACHE_TRY_ASSIGN_GLUE(accessFunction, ...) \
QM_TRY_ASSIGN_META(mozilla::dom::cache, MOZ_UNIQUE_VAR(tryResult), \
accessFunction, ##__VA_ARGS__)
#define CACHE_TRY_UNWRAP(...) CACHE_TRY_ASSIGN_GLUE(unwrap, __VA_ARGS__)
#define CACHE_TRY_INSPECT(...) CACHE_TRY_ASSIGN_GLUE(inspect, __VA_ARGS__)
// Cache equivalents of QM_TRY_RETURN.
#define CACHE_TRY_RETURN_GLUE(...) \
QM_TRY_RETURN_META(mozilla::dom::cache, MOZ_UNIQUE_VAR(tryResult), \
##__VA_ARGS__)
#define CACHE_TRY_RETURN(...) CACHE_TRY_RETURN_GLUE(__VA_ARGS__)
// Cache equivalents of QM_FAIL.
#define CACHE_FAIL_GLUE(...) QM_FAIL_META(mozilla::dom::cache, ##__VA_ARGS__)
#define CACHE_FAIL(...) CACHE_FAIL_GLUE(__VA_ARGS__)
namespace mozilla::dom::cache {
QM_META_HANDLE_ERROR("Cache"_ns)
} // namespace mozilla::dom::cache
// XXX Replace all uses by the QM_* variants and remove these aliases
#define CACHE_TRY QM_TRY
#define CACHE_TRY_UNWRAP QM_TRY_UNWRAP
#define CACHE_TRY_INSPECT QM_TRY_INSPECT
#define CACHE_TRY_RETURN QM_TRY_RETURN
#define CACHE_FAIL QM_FAIL
#endif // mozilla_dom_cache_CacheCommon_h

Просмотреть файл

@ -12,32 +12,15 @@
class JSStructuredCloneData;
class nsIInputStream;
// IndexedDB equivalents of QM_TRY.
#define IDB_TRY_GLUE(...) \
QM_TRY_META(mozilla::dom::indexedDB, MOZ_UNIQUE_VAR(tryResult), ##__VA_ARGS__)
#define IDB_TRY(...) IDB_TRY_GLUE(__VA_ARGS__)
// IndexedDB equivalents of QM_TRY_UNWRAP and QM_TRY_INSPECT.
#define IDB_TRY_ASSIGN_GLUE(accessFunction, ...) \
QM_TRY_ASSIGN_META(mozilla::dom::indexedDB, MOZ_UNIQUE_VAR(tryResult), \
accessFunction, ##__VA_ARGS__)
#define IDB_TRY_UNWRAP(...) IDB_TRY_ASSIGN_GLUE(unwrap, __VA_ARGS__)
#define IDB_TRY_INSPECT(...) IDB_TRY_ASSIGN_GLUE(inspect, __VA_ARGS__)
// IndexedDB equivalents of QM_TRY_RETURN.
#define IDB_TRY_RETURN_GLUE(...) \
QM_TRY_RETURN_META(mozilla::dom::indexedDB, MOZ_UNIQUE_VAR(tryResult), \
##__VA_ARGS__)
#define IDB_TRY_RETURN(...) IDB_TRY_RETURN_GLUE(__VA_ARGS__)
// IndexedDB equivalents of QM_FAIL.
#define IDB_FAIL_GLUE(...) QM_FAIL_META(mozilla::dom::indexedDB, ##__VA_ARGS__)
#define IDB_FAIL(...) IDB_FAIL_GLUE(__VA_ARGS__)
// XXX Replace all uses by the QM_* variants and remove these aliases
#define IDB_TRY QM_TRY
#define IDB_TRY_UNWRAP QM_TRY_UNWRAP
#define IDB_TRY_INSPECT QM_TRY_INSPECT
#define IDB_TRY_RETURN QM_TRY_RETURN
#define IDB_FAIL QM_FAIL
namespace mozilla::dom::indexedDB {
QM_META_HANDLE_ERROR("IndexedDB"_ns)
static constexpr uint32_t kFileCopyBufferSize = 32768;
nsresult SnappyUncompressStructuredCloneData(

Просмотреть файл

@ -189,29 +189,12 @@
* interface.
*/
// LocalStorage equivalents of QM_TRY.
#define LS_TRY_GLUE(...) \
QM_TRY_META(mozilla::dom::localstorage, MOZ_UNIQUE_VAR(tryResult), \
##__VA_ARGS__)
#define LS_TRY(...) LS_TRY_GLUE(__VA_ARGS__)
// LocalStorage equivalents of QM_TRY_UNWRAP and QM_TRY_INSPECT.
#define LS_TRY_ASSIGN_GLUE(accessFunction, ...) \
QM_TRY_ASSIGN_META(mozilla::dom::localstorage, MOZ_UNIQUE_VAR(tryResult), \
accessFunction, ##__VA_ARGS__)
#define LS_TRY_UNWRAP(...) LS_TRY_ASSIGN_GLUE(unwrap, __VA_ARGS__)
#define LS_TRY_INSPECT(...) LS_TRY_ASSIGN_GLUE(inspect, __VA_ARGS__)
// LocalStorage equivalents of QM_TRY_RETURN.
#define LS_TRY_RETURN_GLUE(...) \
QM_TRY_RETURN_META(mozilla::dom::localstorage, MOZ_UNIQUE_VAR(tryResult), \
##__VA_ARGS__)
#define LS_TRY_RETURN(...) LS_TRY_RETURN_GLUE(__VA_ARGS__)
// LocalStorage equivalents of QM_FAIL.
#define LS_FAIL_GLUE(...) \
QM_FAIL_META(mozilla::dom::localstorage, ##__VA_ARGS__)
#define LS_FAIL(...) LS_FAIL_GLUE(__VA_ARGS__)
// XXX Replace all uses by the QM_* variants and remove these aliases
#define LS_TRY QM_TRY
#define LS_TRY_UNWRAP QM_TRY_UNWRAP
#define LS_TRY_INSPECT QM_TRY_INSPECT
#define LS_TRY_RETURN QM_TRY_RETURN
#define LS_FAIL QM_FAIL
namespace mozilla {
@ -274,12 +257,6 @@ Result<std::pair<nsCString, nsCString>, nsresult> GenerateOriginKey2(
LogModule* GetLocalStorageLogger();
namespace localstorage {
QM_META_HANDLE_ERROR("LocalStorage"_ns)
} // namespace localstorage
} // namespace dom
} // namespace mozilla

Просмотреть файл

@ -310,9 +310,47 @@ void ScopedLogExtraInfo::AddInfo() {
}
#endif
void LogError(const nsLiteralCString& aModule, const nsACString& aExpr,
const nsACString& aSourceFile, int32_t aSourceLine,
Maybe<nsresult> aRv) {
namespace detail {
nsDependentCSubstring GetSourceTreeBase() {
static constexpr auto thisFileRelativeSourceFileName =
"/dom/quota/QuotaCommon.cpp"_ns;
static constexpr auto path = nsLiteralCString(__FILE__);
MOZ_ASSERT(StringEndsWith(path, thisFileRelativeSourceFileName));
return Substring(path, 0,
path.Length() - thisFileRelativeSourceFileName.Length());
}
nsDependentCSubstring MakeRelativeSourceFileName(
const nsACString& aSourceFile) {
static constexpr auto error = "ERROR"_ns;
static const auto sourceTreeBase = GetSourceTreeBase();
if (MOZ_LIKELY(StringBeginsWith(aSourceFile, sourceTreeBase))) {
return Substring(aSourceFile, sourceTreeBase.Length() + 1);
}
nsCString::const_iterator begin, end;
if (RFindInReadable("/"_ns, aSourceFile.BeginReading(begin),
aSourceFile.EndReading(end))) {
// Use the basename as a fallback, to avoid exposing any user parts of the
// path.
++begin;
return Substring(begin, aSourceFile.EndReading(end));
}
return nsDependentCSubstring{static_cast<mozilla::Span<const char>>(
static_cast<const nsCString&>(error))};
}
} // namespace detail
void LogError(const nsACString& aExpr, const nsACString& aSourceFile,
int32_t aSourceLine, Maybe<nsresult> aRv) {
#if defined(EARLY_BETA_OR_EARLIER) || defined(DEBUG)
nsAutoCString extraInfosString;
nsAutoCString rvName;
@ -330,6 +368,10 @@ void LogError(const nsLiteralCString& aModule, const nsACString& aExpr,
!rvName.IsEmpty() ? rvName.get() : "", !rvName.IsEmpty() ? ")" : "");
}
const auto relativeSourceFile =
detail::MakeRelativeSourceFileName(aSourceFile);
#endif
#ifdef QM_ENABLE_SCOPED_LOG_EXTRA_INFO
const auto& extraInfos = ScopedLogExtraInfo::GetExtraInfoMap();
for (const auto& item : extraInfos) {
@ -339,26 +381,26 @@ void LogError(const nsLiteralCString& aModule, const nsACString& aExpr,
#endif
#ifdef DEBUG
NS_DebugBreak(NS_DEBUG_WARNING, nsAutoCString(aModule + " failure"_ns).get(),
NS_DebugBreak(NS_DEBUG_WARNING, nsAutoCString("QM_TRY failure"_ns).get(),
(extraInfosString.IsEmpty()
? nsPromiseFlatCString(aExpr)
: static_cast<const nsCString&>(
nsAutoCString(aExpr + extraInfosString)))
.get(),
nsPromiseFlatCString(aSourceFile).get(), aSourceLine);
nsPromiseFlatCString(relativeSourceFile).get(), aSourceLine);
#endif
#if defined(EARLY_BETA_OR_EARLIER) || defined(DEBUG)
nsCOMPtr<nsIConsoleService> console =
do_GetService(NS_CONSOLESERVICE_CONTRACTID);
if (console) {
NS_ConvertUTF8toUTF16 message(aModule + " failure: '"_ns + aExpr +
"', file "_ns + GetLeafName(aSourceFile) +
", line "_ns + IntToCString(aSourceLine) +
extraInfosString);
NS_ConvertUTF8toUTF16 message("QM_TRY failure: '"_ns + aExpr + "' at "_ns +
relativeSourceFile + ":"_ns +
IntToCString(aSourceLine) + extraInfosString);
// The concatenation above results in a message like:
// QuotaManager failure: 'EXP', file XYZ, line N)
// QM_TRY failure: 'EXPR' failed with result NS_ERROR_FAILURE at
// dom/quota/Foo.cpp:12345
console->LogStringMessage(message.get());
}
@ -371,9 +413,11 @@ void LogError(const nsLiteralCString& aModule, const nsACString& aExpr,
auto extra = Some([&] {
auto res = CopyableTArray<EventExtraEntry>{};
res.SetCapacity(5);
res.AppendElement(EventExtraEntry{"module"_ns, aModule});
res.AppendElement(EventExtraEntry{"source_file"_ns,
nsCString(GetLeafName(aSourceFile))});
// TODO We could still fill the module field, based on the source
// directory, but we probably don't need to.
// res.AppendElement(EventExtraEntry{"module"_ns, aModule});
res.AppendElement(
EventExtraEntry{"source_file"_ns, nsCString(relativeSourceFile)});
res.AppendElement(
EventExtraEntry{"source_line"_ns, IntToCString(aSourceLine)});
res.AppendElement(EventExtraEntry{

Просмотреть файл

@ -1015,9 +1015,16 @@ Result<SingleStepSuccessType<ResultHandling>, nsresult>
CreateAndExecuteSingleStepStatement(mozIStorageConnection& aConnection,
const nsACString& aStatementString);
void LogError(const nsLiteralCString& aModule, const nsACString& aExpr,
const nsACString& aSourceFile, int32_t aSourceLine,
Maybe<nsresult> aRv);
namespace detail {
nsDependentCSubstring GetSourceTreeBase();
nsDependentCSubstring MakeRelativeSourceFileName(const nsACString& aSourceFile);
} // namespace detail
void LogError(const nsACString& aExpr, const nsACString& aSourceFile,
int32_t aSourceLine, Maybe<nsresult> aRv);
#ifdef DEBUG
Result<bool, nsresult> WarnIfFileIsUnknown(nsIFile& aFile,
@ -1081,45 +1088,37 @@ struct MOZ_STACK_CLASS ScopedLogExtraInfo {
#endif
};
#if defined(EARLY_BETA_OR_EARLIER) || defined(DEBUG)
# define QM_META_HANDLE_ERROR(module) \
template <typename T> \
MOZ_COLD inline void HandleError(const char* aExpr, const T& aRv, \
const char* aSourceFile, \
int32_t aSourceLine) { \
if constexpr (std::is_same_v<T, nsresult>) { \
mozilla::dom::quota::LogError(module, nsDependentCString(aExpr), \
nsDependentCString(aSourceFile), \
aSourceLine, Some(aRv)); \
} else { \
mozilla::dom::quota::LogError(module, nsDependentCString(aExpr), \
nsDependentCString(aSourceFile), \
aSourceLine, Nothing{}); \
} \
}
#else
# define QM_META_HANDLE_ERROR(module) \
template <typename T> \
MOZ_ALWAYS_INLINE constexpr void HandleError( \
const char* aExpr, const T& aRv, const char* aSourceFile, \
int32_t aSourceLine) {}
#endif
// As this is a function that will only be called in error cases, this is marked
// with MOZ_COLD to avoid bloating the code of calling functions, if it's not
// empty.
// As HandleError is a function that will only be called in error cases, it is
// marked with MOZ_COLD to avoid bloating the code of calling functions, if it's
// not empty.
//
// For the same reason, the string-ish parameters are of type const char* rather
// than any ns*String type, to minimize the code at each call site. This
// deliberately de-optimizes runtime performance, which is uncritical during
// error handling.
//
// The corresponding functions in the quota clients should be defined using
// QM_META_HANDLE_ERROR, in particular they should have exactly the same
// signature incl. attributes. These functions are not intended to be called
// This functions are not intended to be called
// directly, they should only be called from the QM_* macros.
QM_META_HANDLE_ERROR("QuotaManager"_ns)
#if defined(EARLY_BETA_OR_EARLIER) || defined(DEBUG)
template <typename T>
MOZ_COLD void HandleError(const char* aExpr, const T& aRv,
const char* aSourceFile, int32_t aSourceLine) {
if constexpr (std::is_same_v<T, nsresult>) {
mozilla::dom::quota::LogError(nsDependentCString(aExpr),
nsDependentCString(aSourceFile), aSourceLine,
Some(aRv));
} else {
mozilla::dom::quota::LogError(nsDependentCString(aExpr),
nsDependentCString(aSourceFile), aSourceLine,
Nothing{});
}
}
#else
template <typename T>
MOZ_ALWAYS_INLINE constexpr void HandleError(const char* aExpr, const T& aRv,
const char* aSourceFile,
int32_t aSourceLine) {}
#endif
template <SingleStepResult ResultHandling = SingleStepResult::AssertHasResult,
typename BindFunctor>

Просмотреть файл

@ -1536,6 +1536,62 @@ TEST(QuotaCommon_CallWithDelayedRetriesIfAccessDenied, FailuresAndSuccess)
EXPECT_TRUE(res.isOk());
}
static constexpr auto thisFileRelativeSourceFileName =
"dom/quota/test/gtest/TestQuotaCommon.cpp"_ns;
TEST(QuotaCommon_MakeRelativeSourceFileName, ThisDomQuotaFile)
{
const nsCString relativeSourceFilePath{
mozilla::dom::quota::detail::MakeRelativeSourceFileName(
nsLiteralCString(__FILE__))};
EXPECT_STREQ(relativeSourceFilePath.get(),
thisFileRelativeSourceFileName.get());
}
static nsCString MakeFullPath(const nsACString& aRelativePath) {
nsCString path{mozilla::dom::quota::detail::GetSourceTreeBase()};
path.Append("/");
path.Append(aRelativePath);
return path;
}
TEST(QuotaCommon_MakeRelativeSourceFileName, DomIndexedDBFile)
{
static constexpr auto domIndexedDBFileRelativePath =
"dom/indexedDB/ActorsParent.cpp"_ns;
const nsCString relativeSourceFilePath{
mozilla::dom::quota::detail::MakeRelativeSourceFileName(
MakeFullPath(domIndexedDBFileRelativePath))};
EXPECT_STREQ(relativeSourceFilePath.get(),
domIndexedDBFileRelativePath.get());
}
TEST(QuotaCommon_MakeRelativeSourceFileName, NonDomFile)
{
static constexpr auto nonDomFileRelativePath =
"storage/mozStorageService.cpp"_ns;
const nsCString relativeSourceFilePath{
mozilla::dom::quota::detail::MakeRelativeSourceFileName(
MakeFullPath(nonDomFileRelativePath))};
EXPECT_STREQ(relativeSourceFilePath.get(), nonDomFileRelativePath.get());
}
TEST(QuotaCommon_MakeRelativeSourceFileName, OtherFile)
{
constexpr auto otherName = "/foo/bar/Test.cpp"_ns;
const nsCString relativeSourceFilePath{
mozilla::dom::quota::detail::MakeRelativeSourceFileName(otherName)};
EXPECT_STREQ(relativeSourceFilePath.get(), "Test.cpp");
}
#ifdef __clang__
# pragma clang diagnostic pop
#endif

Просмотреть файл

@ -8,41 +8,19 @@
#define mozilla_dom_simpledb_SimpledbCommon_h
#include "mozilla/dom/quota/QuotaCommon.h"
#include "nsLiteralString.h"
// SimpleDB equivalents of QM_TRY.
#define SDB_TRY_GLUE(...) \
QM_TRY_META(mozilla::dom::simpledb, MOZ_UNIQUE_VAR(tryResult), ##__VA_ARGS__)
#define SDB_TRY(...) SDB_TRY_GLUE(__VA_ARGS__)
// SimpleDB equivalents of QM_TRY_UNWRAP and QM_TRY_INSPECT.
#define SDB_TRY_ASSIGN_GLUE(accessFunction, ...) \
QM_TRY_ASSIGN_META(mozilla::dom::simpledb, MOZ_UNIQUE_VAR(tryResult), \
accessFunction, ##__VA_ARGS__)
#define SDB_TRY_UNWRAP(...) SDB_TRY_ASSIGN_GLUE(unwrap, __VA_ARGS__)
#define SDB_TRY_INSPECT(...) SDB_TRY_ASSIGN_GLUE(inspect, __VA_ARGS__)
// SimpleDB equivalents of QM_TRY_RETURN.
#define SDB_TRY_RETURN_GLUE(...) \
QM_TRY_RETURN_META(mozilla::dom::simpledb, MOZ_UNIQUE_VAR(tryResult), \
##__VA_ARGS__)
#define SDB_TRY_RETURN(...) SDB_TRY_RETURN_GLUE(__VA_ARGS__)
// SimpleDB equivalents of QM_FAIL.
#define SDB_FAIL_GLUE(...) QM_FAIL_META(mozilla::dom::simpledb, ##__VA_ARGS__)
#define SDB_FAIL(...) SDB_FAIL_GLUE(__VA_ARGS__)
// XXX Replace all uses by the QM_* variants and remove these aliases
#define SDB_TRY QM_TRY
#define SDB_TRY_UNWRAP QM_TRY_UNWRAP
#define SDB_TRY_INSPECT QM_TRY_INSPECT
#define SDB_TRY_RETURN QM_TRY_RETURN
#define SDB_FAIL QM_FAIL
namespace mozilla {
namespace dom {
extern const char* kPrefSimpleDBEnabled;
namespace simpledb {
QM_META_HANDLE_ERROR("SimpleDB"_ns)
} // namespace simpledb
} // namespace dom
} // namespace mozilla

Просмотреть файл

@ -2656,7 +2656,6 @@ dom.quota.try:
record_in_processes: ["main", "content"]
extra_keys:
context: The context in which the error occurred, e.g. during a storage initialization. Telemetry events are only emitted for selected contexts.
module: The module (quota manager or one of its clients) where the error occured.
result: Optionally, the name of the error that occurred.
seq: Sequence number.
source_file: The name of the source code file where the error occurred.