Backed out changeset 2b1ffc8004d5 (bug 1035892) for causing bug 1574515. a=backout
authorRazvan Maries <rmaries@mozilla.com>
Mon, 19 Aug 2019 18:04:05 +0300
changeset 488675 1868b438bd8307c67a4b3f569239410b843722e0
parent 488674 fd450d47a9f764098c887cfa8cdf9c390d0e3063
child 488711 115a4bcdb596c246f51658458951eda23bf4e696
push id36452
push userrmaries@mozilla.com
push dateMon, 19 Aug 2019 15:09:44 +0000
treeherdermozilla-central@1868b438bd83 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbackout
bugs1035892, 1574515
milestone70.0a1
backs out2b1ffc8004d5e8160bc5bb1109c9f67a003048ef
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
Backed out changeset 2b1ffc8004d5 (bug 1035892) for causing bug 1574515. a=backout
toolkit/crashreporter/breakpad-client/mac/crash_generation/crash_generation_client.cc
toolkit/crashreporter/breakpad-client/mac/crash_generation/crash_generation_client.h
toolkit/crashreporter/breakpad-client/mac/crash_generation/crash_generation_server.h
toolkit/crashreporter/breakpad-client/mac/handler/exception_handler.cc
toolkit/crashreporter/breakpad-client/mac/handler/exception_handler.h
toolkit/crashreporter/breakpad-client/mac/handler/minidump_generator.h
--- a/toolkit/crashreporter/breakpad-client/mac/crash_generation/crash_generation_client.cc
+++ b/toolkit/crashreporter/breakpad-client/mac/crash_generation/crash_generation_client.cc
@@ -34,17 +34,17 @@
 
 #include "mozilla/recordreplay/ChildIPC.h"
 
 namespace google_breakpad {
 
 bool CrashGenerationClient::RequestDumpForException(
     int exception_type,
     int exception_code,
-    int64_t exception_subcode,
+    int exception_subcode,
     mach_port_t crashing_thread) {
   // The server will send a message to this port indicating that it
   // has finished its work.
   ReceivePort acknowledge_port;
 
   MachSendMessage message(kDumpRequestMessage);
   message.AddDescriptor(mach_task_self());            // this task
   message.AddDescriptor(crashing_thread);             // crashing thread
--- a/toolkit/crashreporter/breakpad-client/mac/crash_generation/crash_generation_client.h
+++ b/toolkit/crashreporter/breakpad-client/mac/crash_generation/crash_generation_client.h
@@ -40,17 +40,17 @@ class CrashGenerationClient {
     : sender_(mach_port_name) {
   }
 
   // Request the crash server to generate a dump.
   //
   // Return true if the dump was successful; false otherwise.
   bool RequestDumpForException(int exception_type,
 			       int exception_code,
-			       int64_t exception_subcode,
+			       int exception_subcode,
 			       mach_port_t crashing_thread);
 
   bool RequestDump() {
     return RequestDumpForException(0, 0, 0, MACH_PORT_NULL);
   }
 
  private:
   MachPortSender sender_;
--- a/toolkit/crashreporter/breakpad-client/mac/crash_generation/crash_generation_server.h
+++ b/toolkit/crashreporter/breakpad-client/mac/crash_generation/crash_generation_server.h
@@ -46,17 +46,17 @@ enum {
   kAcknowledgementMessage = 2,
   kQuitMessage            = 3
 };
 
 // Exception details sent by the client when requesting a dump.
 struct ExceptionInfo {
   int32_t exception_type;
   int32_t exception_code;
-  int64_t exception_subcode;
+  int32_t exception_subcode;
   int32_t child_pid;
 };
 
 class CrashGenerationServer {
  public:
   // WARNING: callbacks may be invoked on a different thread
   // than that which creates the CrashGenerationServer.  They must
   // be thread safe.
--- a/toolkit/crashreporter/breakpad-client/mac/handler/exception_handler.cc
+++ b/toolkit/crashreporter/breakpad-client/mac/handler/exception_handler.cc
@@ -84,205 +84,148 @@ static union {
 #endif  // USE_PROTECTED_ALLOCATIONS
   google_breakpad::ExceptionHandler *handler;
 } gProtectedData;
 
 using std::map;
 
 // These structures and techniques are illustrated in
 // Mac OS X Internals, Amit Singh, ch 9.7
-#pragma pack(push, 4)
 struct ExceptionMessage {
   mach_msg_header_t           header;
   mach_msg_body_t             body;
   mach_msg_port_descriptor_t  thread;
   mach_msg_port_descriptor_t  task;
   NDR_record_t                ndr;
   exception_type_t            exception;
   mach_msg_type_number_t      code_count;
-  mach_exception_data_type_t  code[EXCEPTION_CODE_MAX];
+  integer_t                   code[EXCEPTION_CODE_MAX];
   char                        padding[512];
 };
-#pragma pack(pop)
 
 struct ExceptionParameters {
   ExceptionParameters() : count(0) {}
   mach_msg_type_number_t count;
   exception_mask_t masks[EXC_TYPES_COUNT];
   mach_port_t ports[EXC_TYPES_COUNT];
   exception_behavior_t behaviors[EXC_TYPES_COUNT];
   thread_state_flavor_t flavors[EXC_TYPES_COUNT];
 };
 
-#pragma pack(push, 4)
 struct ExceptionReplyMessage {
   mach_msg_header_t  header;
   NDR_record_t       ndr;
   kern_return_t      return_code;
 };
-#pragma pack(pop)
 
 // Only catch these three exceptions.  The other ones are nebulously defined
 // and may result in treating a non-fatal exception as fatal.
 exception_mask_t s_exception_mask = EXC_MASK_BAD_ACCESS |
 EXC_MASK_BAD_INSTRUCTION | EXC_MASK_ARITHMETIC | EXC_MASK_BREAKPOINT;
 
+#if !TARGET_OS_IPHONE
+extern "C" {
+  // Forward declarations for functions that need "C" style compilation
+  boolean_t exc_server(mach_msg_header_t* request,
+                       mach_msg_header_t* reply);
+
+  // This symbol must be visible to dlsym() - see
+  // https://bugs.chromium.org/p/google-breakpad/issues/detail?id=345 for details.
+  kern_return_t catch_exception_raise(mach_port_t target_port,
+                                      mach_port_t failed_thread,
+                                      mach_port_t task,
+                                      exception_type_t exception,
+                                      exception_data_t code,
+                                      mach_msg_type_number_t code_count)
+      __attribute__((visibility("default")));
+}
+#endif
+
 kern_return_t ForwardException(mach_port_t task,
                                mach_port_t failed_thread,
                                exception_type_t exception,
-                               mach_exception_data_t code,
+                               exception_data_t code,
                                mach_msg_type_number_t code_count);
 
-// The contents of mach_exc_server() and mach_exception_raise() are derived
-// from /usr/include/mach/mach_exc.defs, as follows:
-//
-// 1) Run 'mig mach_exc.defs' which creates the following files:
-//    mach_exc.h
-//    mach_excServer.c
-//    mach_excUser.c
-// 2) The relevant code for mach_exc_server() comes from the following methods
-//    in mach_excServer.c:
-//      mach_exc_server()
-//      _Xmach_exception_raise()
-// 3) The relevant code for mach_exception_raise() comes from the following
-//    method in mach_excUser.c:
-//      mach_exception_raise()
-boolean_t mach_exc_server(mach_msg_header_t* InHeadP,
-                          mach_msg_header_t* OutHeadP)
-{
+#if TARGET_OS_IPHONE
+// Implementation is based on the implementation generated by mig.
+boolean_t breakpad_exc_server(mach_msg_header_t* InHeadP,
+                              mach_msg_header_t* OutHeadP) {
   OutHeadP->msgh_bits =
-    MACH_MSGH_BITS(MACH_MSGH_BITS_REMOTE(InHeadP->msgh_bits), 0);
+      MACH_MSGH_BITS(MACH_MSGH_BITS_REMOTE(InHeadP->msgh_bits), 0);
   OutHeadP->msgh_remote_port = InHeadP->msgh_remote_port;
-  /* Minimal size: mach_exception_raise() will update it if different */
+  /* Minimal size: routine() will update it if different */
   OutHeadP->msgh_size = (mach_msg_size_t)sizeof(mig_reply_error_t);
   OutHeadP->msgh_local_port = MACH_PORT_NULL;
   OutHeadP->msgh_id = InHeadP->msgh_id + 100;
-  OutHeadP->msgh_reserved = 0;
 
-  if (InHeadP->msgh_id != 2405) {
+  if (InHeadP->msgh_id != 2401) {
     ((mig_reply_error_t*)OutHeadP)->NDR = NDR_record;
     ((mig_reply_error_t*)OutHeadP)->RetCode = MIG_BAD_ID;
     return FALSE;
   }
 
-#pragma pack(push, 4)
+#ifdef  __MigPackStructs
+#pragma pack(4)
+#endif
   typedef struct {
     mach_msg_header_t Head;
     /* start of the kernel processed data */
     mach_msg_body_t msgh_body;
     mach_msg_port_descriptor_t thread;
     mach_msg_port_descriptor_t task;
     /* end of the kernel processed data */
     NDR_record_t NDR;
     exception_type_t exception;
     mach_msg_type_number_t codeCnt;
-    int64_t code[2];
+    integer_t code[2];
     mach_msg_trailer_t trailer;
   } Request;
 
   typedef struct {
     mach_msg_header_t Head;
     NDR_record_t NDR;
     kern_return_t RetCode;
   } Reply;
-#pragma pack(pop)
+#ifdef  __MigPackStructs
+#pragma pack()
+#endif
 
   Request* In0P = (Request*)InHeadP;
   Reply* OutP = (Reply*)OutHeadP;
 
   if (In0P->task.name != mach_task_self()) {
     return FALSE;
   }
   OutP->RetCode = ForwardException(In0P->task.name,
                                    In0P->thread.name,
                                    In0P->exception,
                                    In0P->code,
                                    In0P->codeCnt);
   OutP->NDR = NDR_record;
   return TRUE;
 }
-
-kern_return_t mach_exception_raise(mach_port_t exception_port,
-                                   mach_port_t thread,
-                                   mach_port_t task,
-                                   exception_type_t exception,
-                                   mach_exception_data_t code,
-                                   mach_msg_type_number_t codeCnt)
-{
-#pragma pack(push, 4)
-  typedef struct {
-    mach_msg_header_t Head;
-    /* start of the kernel processed data */
-    mach_msg_body_t msgh_body;
-    mach_msg_port_descriptor_t thread;
-    mach_msg_port_descriptor_t task;
-    /* end of the kernel processed data */
-    NDR_record_t NDR;
-    exception_type_t exception;
-    mach_msg_type_number_t codeCnt;
-    int64_t code[2];
-  } Request;
-
-  typedef struct {
-    mach_msg_header_t Head;
-    NDR_record_t NDR;
-    kern_return_t RetCode;
-    mach_msg_trailer_t trailer;
-  } Reply;
-#pragma pack(pop)
-
-  Request In;
-  Request *InP = &In;
-
-  mach_msg_return_t msg_result;
-  unsigned int msgh_size;
+#else
+boolean_t breakpad_exc_server(mach_msg_header_t* request,
+                              mach_msg_header_t* reply) {
+  return exc_server(request, reply);
+}
 
-  InP->msgh_body.msgh_descriptor_count = 2;
-  InP->thread.name = thread;
-  InP->thread.disposition = 19;
-  InP->thread.type = MACH_MSG_PORT_DESCRIPTOR;
-  InP->task.name = task;
-  InP->task.disposition = 19;
-  InP->task.type = MACH_MSG_PORT_DESCRIPTOR;
-
-  InP->NDR = NDR_record;
-
-  InP->exception = exception;
-
-  if (codeCnt > 2) {
-    { return MIG_ARRAY_TOO_LARGE; }
+// Callback from exc_server()
+kern_return_t catch_exception_raise(mach_port_t port, mach_port_t failed_thread,
+                                    mach_port_t task,
+                                    exception_type_t exception,
+                                    exception_data_t code,
+                                    mach_msg_type_number_t code_count) {
+  if (task != mach_task_self()) {
+    return KERN_FAILURE;
   }
-  (void)memcpy((char *) InP->code, (const char *) code, 8 * codeCnt);
-
-  InP->codeCnt = codeCnt;
-
-  msgh_size = (mach_msg_size_t)(sizeof(Request) - 16) + ((8 * codeCnt));
-  InP->Head.msgh_bits = MACH_MSGH_BITS_COMPLEX |
-    MACH_MSGH_BITS(19, MACH_MSG_TYPE_MAKE_SEND_ONCE);
-  /* msgh_size passed as argument */
-  InP->Head.msgh_remote_port = exception_port;
-  InP->Head.msgh_local_port = mig_get_reply_port();
-  InP->Head.msgh_id = 2405;
-  InP->Head.msgh_reserved = 0;
-
-  msg_result = mach_msg(&InP->Head,
-                        MACH_SEND_MSG|MACH_RCV_MSG|MACH_MSG_OPTION_NONE,
-                        msgh_size,
-                        (mach_msg_size_t)sizeof(Reply),
-                        InP->Head.msgh_local_port,
-                        MACH_MSG_TIMEOUT_NONE,
-                        MACH_PORT_NULL);
-
-  if (msg_result != MACH_MSG_SUCCESS) {
-    fprintf(stderr, "** mach_msg() error forwarding exception!!\n");
-    return msg_result;
-  }
-
-  return KERN_SUCCESS;
+  return ForwardException(task, failed_thread, exception, code, code_count);
 }
+#endif
 
 ExceptionHandler::ExceptionHandler(const string &dump_path,
                                    FilterCallback filter,
                                    MinidumpCallback callback,
                                    void* callback_context,
                                    bool install_handler,
                                    const char* port_name)
     : dump_path_(),
@@ -403,17 +346,17 @@ bool ExceptionHandler::WriteMinidumpForC
                     callback_context, nullptr, result);
   }
   return result;
 }
 
 bool ExceptionHandler::WriteMinidumpWithException(
     int exception_type,
     int exception_code,
-    int64_t exception_subcode,
+    int exception_subcode,
     breakpad_ucontext_t* task_context,
     mach_port_t thread_name,
     bool exit_after_write,
     bool report_current_thread) {
   bool result = false;
 
 #if TARGET_OS_IPHONE
   // _exit() should never be called on iOS.
@@ -482,17 +425,17 @@ bool ExceptionHandler::WriteMinidumpWith
     }
   }
 
   return result;
 }
 
 kern_return_t ForwardException(mach_port_t task, mach_port_t failed_thread,
                                exception_type_t exception,
-                               mach_exception_data_t code,
+                               exception_data_t code,
                                mach_msg_type_number_t code_count) {
   // At this time, we should have called Uninstall() on the exception handler
   // so that the current exception ports are the ones that we should be
   // forwarding to.
   ExceptionParameters current;
 
   current.count = EXC_TYPES_COUNT;
   mach_port_t current_task = mach_task_self();
@@ -517,20 +460,22 @@ kern_return_t ForwardException(mach_port
     fprintf(stderr, "** No previous ports for forwarding!! \n");
     exit(KERN_FAILURE);
   }
 
   mach_port_t target_port = current.ports[found];
   exception_behavior_t target_behavior = current.behaviors[found];
 
   kern_return_t result;
-  switch (target_behavior & ~MACH_EXCEPTION_CODES) {
+  // TODO: Handle the case where |target_behavior| has MACH_EXCEPTION_CODES
+  // set. https://bugs.chromium.org/p/google-breakpad/issues/detail?id=551
+  switch (target_behavior) {
     case EXCEPTION_DEFAULT:
-      result = mach_exception_raise(target_port, failed_thread, task, exception,
-                                    code, code_count);
+      result = exception_raise(target_port, failed_thread, task, exception,
+                               code, code_count);
       break;
     default:
       fprintf(stderr, "** Unknown exception behavior: %d\n", target_behavior);
       result = KERN_FAILURE;
       break;
   }
 
   return result;
@@ -610,26 +555,29 @@ void* ExceptionHandler::WaitForMessage(v
 
         if (self->use_minidump_write_mutex_)
           pthread_mutex_unlock(&self->minidump_write_mutex_);
       } else {
         // When forking a child process with the exception handler installed,
         // if the child crashes, it will send the exception back to the parent
         // process.  The check for task == self_task() ensures that only
         // exceptions that occur in the parent process are caught and
-        // processed.
+        // processed.  If the exception was not caused by this task, we
+        // still need to call into the exception server and have it return
+        // KERN_FAILURE (see catch_exception_raise) in order for the kernel
+        // to move onto the host exception handler for the child task
         if (receive.task.name == mach_task_self()) {
           self->SuspendThreads();
 
 #if USE_PROTECTED_ALLOCATIONS
           if (gBreakpadAllocator)
             gBreakpadAllocator->Unprotect();
 #endif
 
-          mach_exception_data_type_t subcode = 0;
+          int subcode = 0;
           if (receive.exception == EXC_BAD_ACCESS && receive.code_count > 1)
             subcode = receive.code[1];
 
           // Generate the minidump with the exception data.
           self->WriteMinidumpWithException(receive.exception, receive.code[0],
                                            subcode, NULL, receive.thread.name,
                                            true, false);
 
@@ -642,20 +590,27 @@ void* ExceptionHandler::WaitForMessage(v
 #endif
 
           self->UninstallHandler(true);
 
 #if USE_PROTECTED_ALLOCATIONS
           if (gBreakpadAllocator)
             gBreakpadAllocator->Protect();
 #endif
+          // It's not safe to call exc_server with threads suspended.
+          // exc_server can trigger dlsym(3) calls which deadlock if
+          // another thread is paused while in dlopen(3).
+          self->ResumeThreads();
         }
 
+        // Pass along the exception to the server, which will setup the
+        // message and call catch_exception_raise() and put the return
+        // code into the reply.
         ExceptionReplyMessage reply;
-        if (!mach_exc_server(&receive.header, &reply.header))
+        if (!breakpad_exc_server(&receive.header, &reply.header))
           exit(1);
 
         // Send a reply and exit
         mach_msg(&(reply.header), MACH_SEND_MSG,
                  reply.header.msgh_size, 0, MACH_PORT_NULL,
                  MACH_MSG_TIMEOUT_NONE, MACH_PORT_NULL);
       }
     }
@@ -747,23 +702,20 @@ bool ExceptionHandler::InstallHandler() 
   kern_return_t result = task_get_exception_ports(current_task,
                                                   s_exception_mask,
                                                   previous_->masks,
                                                   &previous_->count,
                                                   previous_->ports,
                                                   previous_->behaviors,
                                                   previous_->flavors);
 
-  // Setup the exception ports on this task.  Such documentation as exists for
-  // task_set_exception_port() and friends can be found in the source code for
-  // xnu.  Apple's implementation is available at https://opensource.apple.com/.
+  // Setup the exception ports on this task
   if (result == KERN_SUCCESS)
     result = task_set_exception_ports(current_task, s_exception_mask,
-                                      handler_port_,
-                                      EXCEPTION_DEFAULT | MACH_EXCEPTION_CODES,
+                                      handler_port_, EXCEPTION_DEFAULT,
                                       THREAD_STATE_NONE);
 
   installed_exception_handler_ = (result == KERN_SUCCESS);
 
   return installed_exception_handler_;
 }
 
 bool ExceptionHandler::UninstallHandler(bool in_exception) {
--- a/toolkit/crashreporter/breakpad-client/mac/handler/exception_handler.h
+++ b/toolkit/crashreporter/breakpad-client/mac/handler/exception_handler.h
@@ -99,17 +99,17 @@ class ExceptionHandler {
                                    bool succeeded);
 
   // A callback function which will be called directly if an exception occurs.
   // This bypasses the minidump file writing and simply gives the client
   // the exception information.
   typedef bool (*DirectCallback)( void *context,
                                   int exception_type,
                                   int exception_code,
-                                  int64_t exception_subcode,
+                                  int exception_subcode,
                                   mach_port_t thread_name);
 
   // Creates a new ExceptionHandler instance to handle writing minidumps.
   // Minidump files will be written to dump_path, and the optional callback
   // is called after writing the dump file, as described above.
   // If install_handler is true, then a minidump will be written whenever
   // an unhandled exception occurs.  If it is false, minidumps will only
   // be written when WriteMinidump is called.
@@ -198,17 +198,17 @@ class ExceptionHandler {
   // success, false otherwise.
   bool SendMessageToHandlerThread(HandlerThreadMessage message_id);
 
   // All minidump writing goes through this one routine.
   // |task_context| can be NULL. If not, it will be used to retrieve the
   // context of the current thread, instead of using |thread_get_state|.
   bool WriteMinidumpWithException(int exception_type,
                                   int exception_code,
-                                  int64_t exception_subcode,
+                                  int exception_subcode,
                                   breakpad_ucontext_t *task_context,
                                   mach_port_t thread_name,
                                   bool exit_after_write,
                                   bool report_current_thread);
 
   // When installed, this static function will be call from a newly created
   // pthread with |this| as the argument
   static void *WaitForMessage(void *exception_handler_class);
--- a/toolkit/crashreporter/breakpad-client/mac/handler/minidump_generator.h
+++ b/toolkit/crashreporter/breakpad-client/mac/handler/minidump_generator.h
@@ -92,17 +92,17 @@ class MinidumpGenerator {
   static string UniqueNameInDirectory(const string &dir, string *unique_name);
 
   // Write out the minidump into |path|
   // All of the components of |path| must exist and be writable
   // Return true if successful, false otherwise
   bool Write(const char *path);
 
   // Specify some exception information, if applicable
-  void SetExceptionInformation(int type, int code, int64_t subcode,
+  void SetExceptionInformation(int type, int code, int subcode,
                                mach_port_t thread_name) {
     exception_type_ = type;
     exception_code_ = code;
     exception_subcode_ = subcode;
     exception_thread_ = thread_name;
   }
 
   // Specify the task context. If |task_context| is not NULL, it will be used
@@ -195,17 +195,17 @@ class MinidumpGenerator {
  protected:
   // Use this writer to put the data to disk
   MinidumpFileWriter writer_;
 
  private:
   // Exception information
   int exception_type_;
   int exception_code_;
-  int64_t exception_subcode_;
+  int exception_subcode_;
   mach_port_t exception_thread_;
   mach_port_t crashing_task_;
   mach_port_t handler_thread_;
 
   // CPU type of the task being dumped.
   cpu_type_t cpu_type_;
 
   // System information