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 375102 22c5313e3d659033b2f8920923f19e76f064ad29
parent 375101 c217dd347b0170650b8f90b2f4d48f683be832c6
child 375103 00e918528ce08cf5ddb61a46692bc04aed95e911
push id93834
push useraklotz@mozilla.com
push dateWed, 16 Aug 2017 22:23:23 +0000
treeherdermozilla-inbound@22c5313e3d65 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjimm
bugs1383501
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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