Bug 1588245 - More values to DetourResultCode. r=mhowell

This is the third attempt to investigate the launcher failure of our detour.
The previous commits d8315e4ed18d and 1b81ea85c43d added the assembly bytes
of a detour target and a special error code `DetourResultCode` to the launcher
failure ping.

In the latest telemetry data, however, the most common value of `hresult`
is still `ERROR_UNIDENTIFIED_ERROR`, meaning the previous commit missed to
set an error code in the common fallible codepath we wanted to know.
Besides `ERROR_UNIDENTIFIED_ERROR`, we're seeing `DETOUR_PATCHER_DO_RESERVE_ERROR`
in the telemetry, but having that code is not enough to pinpoint a falling
operation.

For further investigation, this patch adds ten more values to `DetourResultCode`.
`FUNCHOOKCROSSPROCESS_COPYSTUB_ERROR` is the last codepath we forgot to cover
in the previous commit.  The values of `MMPOLICY_RESERVE_*` are to investigate
`DETOUR_PATCHER_DO_RESERVE_ERROR` in the MMPolicy level.  In both cases, we add
the last Windows error code to `DetourError::mOrigBytes`.

Differential Revision: https://phabricator.services.mozilla.com/D92974

mozglue/misc/interceptor/MMPolicies.h

@@ -150,6 +150,18 @@ class MOZ_TRIVIAL_CTOR_DTOR MMPolicyBase {
   }
 
  public:
+#if defined(NIGHTLY_BUILD)
+  Maybe<DetourError> mLastError;
+  const Maybe<DetourError>& GetLastError() const { return mLastError; }
+  template <typename... Args>
+  void SetLastError(Args&&... aArgs) {
+    mLastError = Some(DetourError(std::forward<Args>(aArgs)...));
+  }
+#else
+  template <typename... Args>
+  void SetLastError(Args&&... aArgs) {}
+#endif  // defined(NIGHTLY_BUILD)
+
   DWORD ComputeAllocationSize(const uint32_t aRequestedSize) const {
     MOZ_ASSERT(aRequestedSize);
     DWORD result = aRequestedSize;
@@ -297,15 +309,17 @@ class MOZ_TRIVIAL_CTOR_DTOR MMPolicyBase {
    * large.
    */
   PVOID FindRegion(HANDLE aProcess, const size_t aDesiredBytesLen,
-                   const uint8_t* aRangeMin, const uint8_t* aRangeMax) const {
+                   const uint8_t* aRangeMin, const uint8_t* aRangeMax) {
     const DWORD kGranularity = GetAllocGranularity();
     MOZ_ASSERT(aDesiredBytesLen >= kGranularity);
     if (!aDesiredBytesLen) {
+      SetLastError(MMPOLICY_RESERVE_FINDREGION_INVALIDLEN);
       return nullptr;
     }
 
     MOZ_ASSERT(aRangeMin < aRangeMax);
     if (aRangeMin >= aRangeMax) {
+      SetLastError(MMPOLICY_RESERVE_FINDREGION_INVALIDRANGE);
       return nullptr;
     }
 
@@ -341,6 +355,8 @@ class MOZ_TRIVIAL_CTOR_DTOR MMPolicyBase {
           reinterpret_cast<const uint8_t*>(mbi.BaseAddress) + mbi.RegionSize;
     }
 
+    SetLastError(MMPOLICY_RESERVE_FINDREGION_VIRTUALQUERY_ERROR,
+                 ::GetLastError());
     return nullptr;
   }
 
@@ -363,10 +379,14 @@ class MOZ_TRIVIAL_CTOR_DTOR MMPolicyBase {
   PVOID Reserve(HANDLE aProcess, const uint32_t aSize,
                 const ReserveFnT& aReserveFn,
                 const ReserveRangeFnT& aReserveRangeFn,
-                const Maybe<Span<const uint8_t>>& aBounds) const {
+                const Maybe<Span<const uint8_t>>& aBounds) {
     if (!aBounds) {
       // No restrictions, let the OS choose the base address
-      return aReserveFn(aProcess, nullptr, aSize);
+      PVOID ret = aReserveFn(aProcess, nullptr, aSize);
+      if (!ret) {
+        SetLastError(MMPOLICY_RESERVE_NOBOUND_RESERVE_ERROR, ::GetLastError());
+      }
+      return ret;
     }
 
     const uint8_t* lowerBound = GetLowerBound(aBounds.ref());
@@ -404,8 +424,11 @@ class MOZ_TRIVIAL_CTOR_DTOR MMPolicyBase {
     // If we run out of attempts, we fall through to the default case where
     // the system chooses any base address it wants. In that case, the hook
     // will be set on a best-effort basis.
-
-    return aReserveFn(aProcess, nullptr, aSize);
+    PVOID ret = aReserveFn(aProcess, nullptr, aSize);
+    if (!ret) {
+      SetLastError(MMPOLICY_RESERVE_FINAL_RESERVE_ERROR, ::GetLastError());
+    }
+    return ret;
   }
 };
 
@@ -802,11 +825,13 @@ class MMPolicyOutOfProcess : public MMPolicyBase {
   uint32_t Reserve(const uint32_t aSize,
                    const Maybe<Span<const uint8_t>>& aBounds) {
     if (!aSize || !mProcess) {
+      SetLastError(MMPOLICY_RESERVE_INVALIDARG);
       return 0;
     }
 
     if (mRemoteView) {
       MOZ_ASSERT(mReservationSize >= aSize);
+      SetLastError(MMPOLICY_RESERVE_ZERO_RESERVATIONSIZE);
       return mReservationSize;
     }
 
@@ -816,12 +841,14 @@ class MMPolicyOutOfProcess : public MMPolicyBase {
                                    PAGE_EXECUTE_READWRITE | SEC_RESERVE, 0,
                                    mReservationSize, nullptr);
     if (!mMapping) {
+      SetLastError(MMPOLICY_RESERVE_CREATEFILEMAPPING, ::GetLastError());
       return 0;
     }
 
     mLocalView = static_cast<uint8_t*>(
         ::MapViewOfFile(mMapping, FILE_MAP_WRITE, 0, 0, 0));
     if (!mLocalView) {
+      SetLastError(MMPOLICY_RESERVE_MAPVIEWOFFILE, ::GetLastError());
       return 0;
     }
 

mozglue/misc/interceptor/PatcherBase.h

@@ -121,6 +121,16 @@ class WindowsDllPatcherBase {
     return mVMPolicy.IsPageAccessible(aAddress);
   }
 
+#if defined(NIGHTLY_BUILD)
+  const Maybe<DetourError>& GetLastError() const {
+    return mVMPolicy.GetLastError();
+  }
+#endif  // defined(NIGHTLY_BUILD)
+  template <typename... Args>
+  void SetLastError(Args&&... aArgs) {
+    mVMPolicy.SetLastError(std::forward<Args>(aArgs)...);
+  }
+
  protected:
   VMPolicy mVMPolicy;
 };

mozglue/misc/interceptor/PatcherDetour.h

@@ -128,10 +128,6 @@ class WindowsDllDetourPatcher final
   using PrimitiveT = WindowsDllDetourPatcherPrimitive<MMPolicyT>;
   Maybe<DetourFlags> mFlags;
 
-#if defined(NIGHTLY_BUILD)
-  Maybe<DetourError> mLastError;
-#endif  // defined(NIGHTLY_BUILD)
-
  public:
   template <typename... Args>
   explicit WindowsDllDetourPatcher(Args&&... aArgs)
@@ -144,15 +140,6 @@ class WindowsDllDetourPatcher final
   WindowsDllDetourPatcher& operator=(const WindowsDllDetourPatcher&) = delete;
   WindowsDllDetourPatcher& operator=(WindowsDllDetourPatcher&&) = delete;
 
-#if defined(NIGHTLY_BUILD)
-  const Maybe<DetourError>& GetLastError() const { return mLastError; }
-  void SetLastError(DetourResultCode aError) {
-    mLastError = Some(DetourError(aError));
-  }
-#else
-  void SetLastError(DetourResultCode) {}
-#endif  // defined(NIGHTLY_BUILD)
-
   void Clear() {
     if (!this->mVMPolicy.ShouldUnhookUponDestruction()) {
       return;
@@ -553,9 +540,11 @@ class WindowsDllDetourPatcher final
 #endif  // defined(_M_X64)
 
     Maybe<TrampPoolT> maybeTrampPool = this->mVMPolicy.Reserve(pivot, distance);
-    if (!maybeTrampPool) {
+#if defined(NIGHTLY_BUILD)
+    if (!maybeTrampPool && this->GetLastError().isNothing()) {
       SetLastError(DetourResultCode::DETOUR_PATCHER_DO_RESERVE_ERROR);
     }
+#endif  // defined(NIGHTLY_BUILD)
     return maybeTrampPool;
   }
 
@@ -943,18 +932,19 @@ class WindowsDllDetourPatcher final
 #if defined(NIGHTLY_BUILD)
       origBytes.Rewind();
       SetLastError(DetourResultCode::DETOUR_PATCHER_CREATE_TRAMPOLINE_ERROR);
+      DetourError& lastError = *this->mVMPolicy.mLastError;
       size_t bytesToCapture = std::min(
-          ArrayLength(mLastError->mOrigBytes),
+          ArrayLength(lastError.mOrigBytes),
           static_cast<size_t>(PrimitiveT::GetWorstCaseRequiredBytesToPatch()));
 #  if defined(_M_ARM64)
       size_t numInstructionsToCapture = bytesToCapture / sizeof(uint32_t);
-      auto origBytesDst = reinterpret_cast<uint32_t*>(mLastError->mOrigBytes);
+      auto origBytesDst = reinterpret_cast<uint32_t*>(lastError.mOrigBytes);
       for (size_t i = 0; i < numInstructionsToCapture; ++i) {
         origBytesDst[i] = origBytes.ReadNextInstruction();
       }
 #  else
       for (size_t i = 0; i < bytesToCapture; ++i) {
-        mLastError->mOrigBytes[i] = origBytes[i];
+        lastError.mOrigBytes[i] = origBytes[i];
       }
 #  endif  // defined(_M_ARM64)
 #else

mozglue/misc/nsWindowsDllInterceptor.h

@@ -229,7 +229,12 @@ class MOZ_ONLY_USED_TO_AVOID_STATIC_CONSTRUCTORS FuncHookCrossProcess final {
       return false;
     }
 
-    return CopyStubToChildProcess(origFunc, aProcess);
+    bool ret = CopyStubToChildProcess(origFunc, aProcess);
+    if (!ret) {
+      aInterceptor.SetLastError(FUNCHOOKCROSSPROCESS_COPYSTUB_ERROR,
+                                ::GetLastError());
+    }
+    return ret;
   }
 
   bool SetDetour(HANDLE aProcess, InterceptorT& aInterceptor, const char* aName,
@@ -240,7 +245,12 @@ class MOZ_ONLY_USED_TO_AVOID_STATIC_CONSTRUCTORS FuncHookCrossProcess final {
       return false;
     }
 
-    return CopyStubToChildProcess(origFunc, aProcess);
+    bool ret = CopyStubToChildProcess(origFunc, aProcess);
+    if (!ret) {
+      aInterceptor.SetLastError(FUNCHOOKCROSSPROCESS_COPYSTUB_ERROR,
+                                ::GetLastError());
+    }
+    return ret;
   }
 
   explicit operator bool() const { return !!mOrigFunc; }
@@ -366,6 +376,10 @@ class WindowsDllInterceptor final
     return mDetourPatcher.GetLastError();
   }
 #endif  // defined(NIGHTLY_BUILD)
+  template <typename... Args>
+  void SetLastError(Args&&... aArgs) {
+    return mDetourPatcher.SetLastError(std::forward<Args>(aArgs)...);
+  }
 
   constexpr static uint32_t GetWorstCaseRequiredBytesToPatch() {
     return WindowsDllDetourPatcherPrimitive<

widget/windows/WinHeaderOnlyUtils.h

@@ -218,6 +218,16 @@ enum DetourResultCode : uint32_t {
   DETOUR_PATCHER_INVALID_TRAMPOLINE,
   DETOUR_PATCHER_WRITE_POINTER_ERROR,
   DETOUR_PATCHER_CREATE_TRAMPOLINE_ERROR,
+  FUNCHOOKCROSSPROCESS_COPYSTUB_ERROR,
+  MMPOLICY_RESERVE_INVALIDARG,
+  MMPOLICY_RESERVE_ZERO_RESERVATIONSIZE,
+  MMPOLICY_RESERVE_CREATEFILEMAPPING,
+  MMPOLICY_RESERVE_MAPVIEWOFFILE,
+  MMPOLICY_RESERVE_NOBOUND_RESERVE_ERROR,
+  MMPOLICY_RESERVE_FINDREGION_INVALIDLEN,
+  MMPOLICY_RESERVE_FINDREGION_INVALIDRANGE,
+  MMPOLICY_RESERVE_FINDREGION_VIRTUALQUERY_ERROR,
+  MMPOLICY_RESERVE_FINAL_RESERVE_ERROR,
 };
 
 #if defined(NIGHTLY_BUILD)
@@ -228,6 +238,12 @@ struct DetourError {
   uint8_t mOrigBytes[16];
   explicit DetourError(DetourResultCode aError)
       : mErrorCode(aError), mOrigBytes{} {}
+  DetourError(DetourResultCode aError, DWORD aWin32Error)
+      : mErrorCode(aError), mOrigBytes{} {
+    static_assert(sizeof(mOrigBytes) >= sizeof(aWin32Error),
+                  "Can't fit a DWORD in mOrigBytes");
+    *reinterpret_cast<DWORD*>(mOrigBytes) = aWin32Error;
+  }
   operator WindowsError() const {
     return WindowsError::FromHResult(mErrorCode);
   }