Bug 1383501: Add crash report annotations to proxy unmarshaling failure paths; r=jimm
authorAaron Klotz <aklotz@mozilla.com>
Wed, 09 Aug 2017 15:07:11 -0600
changeset 647797 22c5313e3d659033b2f8920923f19e76f064ad29
parent 647796 c217dd347b0170650b8f90b2f4d48f683be832c6
child 647798 00e918528ce08cf5ddb61a46692bc04aed95e911
push id74542
push usermaglione.k@gmail.com
push dateWed, 16 Aug 2017 23:10:03 +0000
reviewersjimm
bugs1383501
milestone57.0a1
Bug 1383501: Add crash report annotations to proxy unmarshaling failure paths; r=jimm
ipc/mscom/COMPtrHolder.h
ipc/mscom/ProxyStream.cpp
ipc/mscom/ProxyStream.h
--- a/ipc/mscom/COMPtrHolder.h
+++ b/ipc/mscom/COMPtrHolder.h
@@ -11,16 +11,19 @@
 #include "mozilla/Attributes.h"
 #include "mozilla/DebugOnly.h"
 #include "mozilla/Move.h"
 #include "mozilla/mscom/ProxyStream.h"
 #include "mozilla/mscom/Ptr.h"
 #if defined(MOZ_CONTENT_SANDBOX)
 #include "mozilla/SandboxSettings.h"
 #endif // defined(MOZ_CONTENT_SANDBOX)
+#if defined(MOZ_CRASHREPORTER)
+#include "nsExceptionHandler.h"
+#endif // defined(MOZ_CRASHREPORTER)
 
 namespace mozilla {
 namespace mscom {
 
 template<typename Interface, const IID& _IID>
 class COMPtrHolder
 {
 public:
@@ -168,16 +171,20 @@ struct ParamTraits<mozilla::mscom::COMPt
       buf = mozilla::MakeUnique<BYTE[]>(length);
       if (!aMsg->ReadBytesInto(aIter, buf.get(), length)) {
         return false;
       }
     }
 
     mozilla::mscom::ProxyStream proxyStream(_IID, buf.get(), length);
     if (!proxyStream.IsValid()) {
+#if defined(MOZ_CRASHREPORTER)
+      CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("ProxyStreamValid"),
+                                         NS_LITERAL_CSTRING("false"));
+#endif // defined(MOZ_CRASHREPORTER)
       return false;
     }
 
     typename paramType::COMPtrType ptr;
     if (!proxyStream.GetInterface(mozilla::mscom::getter_AddRefs(ptr))) {
       return false;
     }
 
--- a/ipc/mscom/ProxyStream.cpp
+++ b/ipc/mscom/ProxyStream.cpp
@@ -34,25 +34,37 @@ ProxyStream::ProxyStream()
 // reconstructing the stream from a buffer anyway.
 ProxyStream::ProxyStream(REFIID aIID, const BYTE* aInitBuf,
                          const int aInitBufSize)
   : mStream(InitStream(aInitBuf, static_cast<const UINT>(aInitBufSize)))
   , mGlobalLockedBuf(nullptr)
   , mHGlobal(nullptr)
   , mBufSize(aInitBufSize)
 {
+#if defined(MOZ_CRASHREPORTER)
+  NS_NAMED_LITERAL_CSTRING(kCrashReportKey, "ProxyStreamUnmarshalStatus");
+#endif
+
   if (!aInitBufSize) {
+#if defined(MOZ_CRASHREPORTER)
+    CrashReporter::AnnotateCrashReport(kCrashReportKey,
+                                       NS_LITERAL_CSTRING("!aInitBufSize"));
+#endif // defined(MOZ_CRASHREPORTER)
     // We marshaled a nullptr. Nothing else to do here.
     return;
   }
   // NB: We can't check for a null mStream until after we have checked for
   // the zero aInitBufSize above. This is because InitStream will also fail
   // in that case, even though marshaling a nullptr is allowable.
   MOZ_ASSERT(mStream);
   if (!mStream) {
+#if defined(MOZ_CRASHREPORTER)
+    CrashReporter::AnnotateCrashReport(kCrashReportKey,
+                                       NS_LITERAL_CSTRING("!mStream"));
+#endif // defined(MOZ_CRASHREPORTER)
     return;
   }
 
   HRESULT unmarshalResult = S_OK;
 
   // We need to convert to an interface here otherwise we mess up const
   // correctness with IPDL. We'll request an IUnknown and then QI the
   // actual interface later.
@@ -143,30 +155,43 @@ ProxyStream::InitStream(const BYTE* aIni
   if (FAILED(hr)) {
     return nullptr;
   }
 
   return stream.forget();
 }
 
 ProxyStream::ProxyStream(ProxyStream&& aOther)
+  : mGlobalLockedBuf(nullptr)
+  , mHGlobal(nullptr)
+  , mBufSize(0)
 {
   *this = mozilla::Move(aOther);
 }
 
 ProxyStream&
 ProxyStream::operator=(ProxyStream&& aOther)
 {
+  if (mHGlobal && mGlobalLockedBuf) {
+    DebugOnly<BOOL> result = ::GlobalUnlock(mHGlobal);
+    MOZ_ASSERT(!result && ::GetLastError() == NO_ERROR);
+  }
+
   mStream = Move(aOther.mStream);
+
   mGlobalLockedBuf = aOther.mGlobalLockedBuf;
   aOther.mGlobalLockedBuf = nullptr;
+
+  // ::GlobalFree() was called implicitly when mStream was replaced.
   mHGlobal = aOther.mHGlobal;
   aOther.mHGlobal = nullptr;
+
   mBufSize = aOther.mBufSize;
   aOther.mBufSize = 0;
+
   mUnmarshaledProxy = Move(aOther.mUnmarshaledProxy);
   return *this;
 }
 
 ProxyStream::~ProxyStream()
 {
   if (mHGlobal && mGlobalLockedBuf) {
     DebugOnly<BOOL> result = ::GlobalUnlock(mHGlobal);
@@ -215,16 +240,23 @@ ProxyStream::GetInterface(void** aOutInt
   // We should not have a locked buffer on this side
   MOZ_ASSERT(!mGlobalLockedBuf);
   MOZ_ASSERT(aOutInterface);
 
   if (!aOutInterface) {
     return false;
   }
 
+#if defined(MOZ_CRASHREPORTER)
+  if (!mUnmarshaledProxy) {
+    CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("ProxyStreamUnmarshalStatus"),
+                                       NS_LITERAL_CSTRING("!mUnmarshaledProxy"));
+  }
+#endif // defined(MOZ_CRASHREPORTER)
+
   *aOutInterface = mUnmarshaledProxy.release();
   return true;
 }
 
 ProxyStream::ProxyStream(REFIID aIID, IUnknown* aObject)
   : mGlobalLockedBuf(nullptr)
   , mHGlobal(nullptr)
   , mBufSize(0)
@@ -232,71 +264,117 @@ ProxyStream::ProxyStream(REFIID aIID, IU
   if (!aObject) {
     return;
   }
 
   RefPtr<IStream> stream;
   HGLOBAL hglobal = NULL;
   int streamSize = 0;
 
+  HRESULT createStreamResult = S_OK;
   HRESULT marshalResult = S_OK;
+  HRESULT statResult = S_OK;
+  HRESULT getHGlobalResult = S_OK;
 
   auto marshalFn = [&]() -> void
   {
-    HRESULT hr = ::CreateStreamOnHGlobal(nullptr, TRUE, getter_AddRefs(stream));
-    if (FAILED(hr)) {
+    createStreamResult = ::CreateStreamOnHGlobal(nullptr, TRUE, getter_AddRefs(stream));
+    if (FAILED(createStreamResult)) {
       return;
     }
 
-    hr = ::CoMarshalInterface(stream, aIID, aObject, MSHCTX_LOCAL, nullptr,
-                              MSHLFLAGS_NORMAL);
-    if (FAILED(hr)) {
-      marshalResult = hr;
+    marshalResult = ::CoMarshalInterface(stream, aIID, aObject, MSHCTX_LOCAL,
+                                         nullptr, MSHLFLAGS_NORMAL);
+    if (FAILED(marshalResult)) {
       return;
     }
 
     STATSTG statstg;
-    hr = stream->Stat(&statstg, STATFLAG_NONAME);
-    if (SUCCEEDED(hr)) {
+    statResult = stream->Stat(&statstg, STATFLAG_NONAME);
+    if (SUCCEEDED(statResult)) {
       streamSize = static_cast<int>(statstg.cbSize.LowPart);
+    } else {
+      return;
     }
 
-    hr = ::GetHGlobalFromStream(stream, &hglobal);
-    MOZ_ASSERT(SUCCEEDED(hr));
+    getHGlobalResult = ::GetHGlobalFromStream(stream, &hglobal);
+    MOZ_ASSERT(SUCCEEDED(getHGlobalResult));
   };
 
   if (XRE_IsParentProcess()) {
     // We'll marshal this stuff directly using the current thread, therefore its
     // stub will reside in the same apartment as the current thread.
     marshalFn();
   } else {
     // When marshaling in child processes, we want to force the MTA.
     EnsureMTA mta(marshalFn);
   }
 
 #if defined(MOZ_CRASHREPORTER)
+  if (FAILED(createStreamResult)) {
+    nsPrintfCString hrAsStr("0x%08X", createStreamResult);
+    CrashReporter::AnnotateCrashReport(
+        NS_LITERAL_CSTRING("CreateStreamOnHGlobalFailure"),
+        hrAsStr);
+  }
+
   if (FAILED(marshalResult)) {
     AnnotateInterfaceRegistration(aIID);
     nsPrintfCString hrAsStr("0x%08X", marshalResult);
     CrashReporter::AnnotateCrashReport(
         NS_LITERAL_CSTRING("CoMarshalInterfaceFailure"), hrAsStr);
   }
+
+  if (FAILED(statResult)) {
+    nsPrintfCString hrAsStr("0x%08X", statResult);
+    CrashReporter::AnnotateCrashReport(
+        NS_LITERAL_CSTRING("StatFailure"),
+        hrAsStr);
+  }
+
+  if (FAILED(getHGlobalResult)) {
+    nsPrintfCString hrAsStr("0x%08X", getHGlobalResult);
+    CrashReporter::AnnotateCrashReport(
+        NS_LITERAL_CSTRING("GetHGlobalFromStreamFailure"),
+        hrAsStr);
+  }
 #endif // defined(MOZ_CRASHREPORTER)
 
   mStream = mozilla::Move(stream);
-  mBufSize = streamSize;
 
-  if (hglobal) {
-    mGlobalLockedBuf = reinterpret_cast<BYTE*>(::GlobalLock(hglobal));
-    mHGlobal = hglobal;
+  if (streamSize) {
+#if defined(MOZ_CRASHREPORTER)
+    CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("ProxyStreamSizeFrom"),
+                                       NS_LITERAL_CSTRING("IStream::Stat"));
+#endif // defined(MOZ_CRASHREPORTER)
+    mBufSize = streamSize;
+  }
+
+  if (!hglobal) {
+    return;
+  }
+
+  mGlobalLockedBuf = reinterpret_cast<BYTE*>(::GlobalLock(hglobal));
+  mHGlobal = hglobal;
 
-    // If we couldn't get the stream size directly from mStream, we may use
-    // the size of the memory block allocated by the HGLOBAL, though it might
-    // be larger than the actual stream size.
-    if (!streamSize) {
-      mBufSize = static_cast<int>(::GlobalSize(hglobal));
-    }
+  // If we couldn't get the stream size directly from mStream, we may use
+  // the size of the memory block allocated by the HGLOBAL, though it might
+  // be larger than the actual stream size.
+  if (!streamSize) {
+#if defined(MOZ_CRASHREPORTER)
+    CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("ProxyStreamSizeFrom"),
+                                       NS_LITERAL_CSTRING("GlobalSize"));
+#endif // defined(MOZ_CRASHREPORTER)
+    mBufSize = static_cast<int>(::GlobalSize(hglobal));
   }
+
+#if defined(MOZ_CRASHREPORTER)
+  nsAutoCString strBufSize;
+  strBufSize.AppendInt(mBufSize);
+
+  CrashReporter::AnnotateCrashReport(NS_LITERAL_CSTRING("ProxyStreamSize"),
+                                     strBufSize);
+#endif // defined(MOZ_CRASHREPORTER)
 }
 
 } // namespace mscom
 } // namespace mozilla
 
--- a/ipc/mscom/ProxyStream.h
+++ b/ipc/mscom/ProxyStream.h
@@ -29,17 +29,17 @@ public:
   ProxyStream(const ProxyStream& aOther) = delete;
   ProxyStream& operator=(const ProxyStream& aOther) = delete;
 
   ProxyStream(ProxyStream&& aOther);
   ProxyStream& operator=(ProxyStream&& aOther);
 
   inline bool IsValid() const
   {
-    return !(mStream && mUnmarshaledProxy);
+    return !(mUnmarshaledProxy && mStream);
   }
 
   bool GetInterface(void** aOutInterface);
   const BYTE* GetBuffer(int& aReturnedBufSize) const;
   RefPtr<IStream> GetStream() const;
 
   bool operator==(const ProxyStream& aOther) const
   {
@@ -51,15 +51,14 @@ private:
                                               const UINT aInitBufSize);
 
 private:
   RefPtr<IStream> mStream;
   BYTE*           mGlobalLockedBuf;
   HGLOBAL         mHGlobal;
   int             mBufSize;
   ProxyUniquePtr<IUnknown> mUnmarshaledProxy;
-  HRESULT         mUnmarshalResult;
 };
 
 } // namespace mscom
 } // namespace mozilla
 
 #endif // mozilla_mscom_ProxyStream_h