From c5782715455a9bd7d9d03d5d9a429e4228b7f090 Mon Sep 17 00:00:00 2001 From: "mark@chromium.org" Date: Wed, 25 Jul 2012 15:34:00 +0000 Subject: Speculatively back out r984. See http://codereview.chromium.org/10805065/ and http://build.chromium.org/p/chromium/builders/NACL%20Tests%20%28x64%29/builds/34563 chrome src/native_client/tests/inbrowser_crash_test/crash_dump_tester.py says that the observed failures are a symptom of crash_service.exe itself crashing. git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@999 4c0a9323-5329-0410-9bdc-e9ce6186880e --- src/client/windows/common/ipc_protocol.h | 5 --- src/client/windows/crash_generation/client_info.cc | 45 ---------------------- src/client/windows/crash_generation/client_info.h | 8 ---- .../crash_generation/crash_generation_server.cc | 2 - .../windows/crash_generation/minidump_generator.cc | 22 +++-------- .../windows/crash_generation/minidump_generator.h | 3 -- src/client/windows/unittests/minidump_test.cc | 29 +++----------- src/google_breakpad/common/minidump_format.h | 6 +-- 8 files changed, 12 insertions(+), 108 deletions(-) diff --git a/src/client/windows/common/ipc_protocol.h b/src/client/windows/common/ipc_protocol.h index 42ea8455..b03c032b 100644 --- a/src/client/windows/common/ipc_protocol.h +++ b/src/client/windows/common/ipc_protocol.h @@ -82,11 +82,6 @@ struct CustomInfoEntry { wchar_t value[kValueMaxLength]; }; -struct CustomDataStream { - size_t size; - u_int8_t stream[1]; -}; - // Constants for the protocol between client and the server. // Tags sent with each message indicating the purpose of diff --git a/src/client/windows/crash_generation/client_info.cc b/src/client/windows/crash_generation/client_info.cc index 294096f3..60bbac82 100644 --- a/src/client/windows/crash_generation/client_info.cc +++ b/src/client/windows/crash_generation/client_info.cc @@ -31,8 +31,6 @@ #include "client/windows/common/ipc_protocol.h" static const wchar_t kCustomInfoProcessUptimeName[] = L"ptime"; -static const wchar_t kCustomDataStreamCustomFieldName[] = L"custom-data-stream"; -static const size_t kMaxCustomDataStreamSize = 100 * 1024 * 1024; static const size_t kMaxCustomInfoEntries = 4096; namespace google_breakpad { @@ -50,7 +48,6 @@ ClientInfo::ClientInfo(CrashGenerationServer* crash_server, ex_info_(ex_info), assert_info_(assert_info), custom_client_info_(custom_client_info), - custom_data_stream_(NULL), thread_id_(thread_id), process_handle_(NULL), dump_requested_handle_(NULL), @@ -89,11 +86,6 @@ bool ClientInfo::Initialize() { } ClientInfo::~ClientInfo() { - if (custom_data_stream_) { - delete custom_data_stream_; - custom_data_stream_ = NULL; - } - if (dump_request_wait_handle_) { // Wait for callbacks that might already be running to finish. UnregisterWaitEx(dump_request_wait_handle_, INVALID_HANDLE_VALUE); @@ -207,43 +199,6 @@ bool ClientInfo::PopulateCustomInfo() { return (bytes_count != read_count); } -bool ClientInfo::PopulateCustomDataStream() { - for (SIZE_T i = 0; i < custom_client_info_.count; ++i) { - if (_wcsicmp(kCustomDataStreamCustomFieldName, - custom_client_info_.entries[i].name) != 0) { - continue; - } - wchar_t address_str[CustomInfoEntry::kValueMaxLength]; - memcpy(address_str, custom_client_info_.entries[i].value, - CustomInfoEntry::kValueMaxLength); - wchar_t* size_str = wcschr(address_str, ':'); - if (!size_str) - return false; - - size_str[0] = 0; - ++size_str; - void* address = reinterpret_cast(_wcstoi64(address_str, NULL, 16)); - long size = wcstol(size_str, NULL, 16); - if (size <= 0 || size > kMaxCustomDataStreamSize) - return false; - - custom_data_stream_ = reinterpret_cast( - new u_int8_t[sizeof(CustomDataStream) + size - 1]); - - SIZE_T bytes_count = 0; - if (!ReadProcessMemory(process_handle_, address, - custom_data_stream_->stream, size, &bytes_count)) { - delete custom_data_stream_; - custom_data_stream_ = NULL; - return false; - } - - return true; - } - - return false; -} - CustomClientInfo ClientInfo::GetCustomInfo() const { CustomClientInfo custom_info; custom_info.entries = custom_info_entries_.get(); diff --git a/src/client/windows/crash_generation/client_info.h b/src/client/windows/crash_generation/client_info.h index 3d88ffd4..999e6678 100644 --- a/src/client/windows/crash_generation/client_info.h +++ b/src/client/windows/crash_generation/client_info.h @@ -61,7 +61,6 @@ class ClientInfo { MINIDUMP_TYPE dump_type() const { return dump_type_; } EXCEPTION_POINTERS** ex_info() const { return ex_info_; } MDRawAssertionInfo* assert_info() const { return assert_info_; } - CustomDataStream* custom_data_stream() const { return custom_data_stream_; } DWORD* thread_id() const { return thread_id_; } HANDLE process_handle() const { return process_handle_; } HANDLE dump_requested_handle() const { return dump_requested_handle_; } @@ -91,10 +90,6 @@ class ClientInfo { bool GetClientExceptionInfo(EXCEPTION_POINTERS** ex_info) const; bool GetClientThreadId(DWORD* thread_id) const; - // Reads the custom data stream (if supplied) from the client process - // address space. - bool PopulateCustomDataStream(); - // Reads the custom information from the client process address space. bool PopulateCustomInfo(); @@ -135,9 +130,6 @@ class ClientInfo { // Custom information about the client. CustomClientInfo custom_client_info_; - // Custom data stream supplied by the client. - CustomDataStream* custom_data_stream_; - // Contains the custom client info entries read from the client process // memory. This will be populated only if the method GetClientCustomInfo // is called. diff --git a/src/client/windows/crash_generation/crash_generation_server.cc b/src/client/windows/crash_generation/crash_generation_server.cc index de3d3fce..477973c1 100644 --- a/src/client/windows/crash_generation/crash_generation_server.cc +++ b/src/client/windows/crash_generation/crash_generation_server.cc @@ -796,7 +796,6 @@ void CALLBACK CrashGenerationServer::OnDumpRequest(void* context, BOOLEAN) { assert(context); ClientInfo* client_info = reinterpret_cast(context); client_info->PopulateCustomInfo(); - client_info->PopulateCustomDataStream(); CrashGenerationServer* crash_server = client_info->crash_server(); assert(crash_server); @@ -893,7 +892,6 @@ bool CrashGenerationServer::GenerateDump(const ClientInfo& client, GetCurrentThreadId(), client_ex_info, client.assert_info(), - client.custom_data_stream(), client.dump_type(), true, dump_path); diff --git a/src/client/windows/crash_generation/minidump_generator.cc b/src/client/windows/crash_generation/minidump_generator.cc index f407ef8c..fe4937aa 100644 --- a/src/client/windows/crash_generation/minidump_generator.cc +++ b/src/client/windows/crash_generation/minidump_generator.cc @@ -271,15 +271,14 @@ bool MinidumpGenerator::WriteMinidump(HANDLE process_handle, DWORD requesting_thread_id, EXCEPTION_POINTERS* exception_pointers, MDRawAssertionInfo* assert_info, - CustomDataStream* custom_data_stream, MINIDUMP_TYPE dump_type, bool is_client_pointers, wstring* dump_path) { // Just call the full WriteMinidump with NULL as the full_dump_path. return this->WriteMinidump(process_handle, process_id, thread_id, requesting_thread_id, exception_pointers, - assert_info, custom_data_stream, dump_type, - is_client_pointers, dump_path, NULL); + assert_info, dump_type, is_client_pointers, + dump_path, NULL); } bool MinidumpGenerator::WriteMinidump(HANDLE process_handle, @@ -288,7 +287,6 @@ bool MinidumpGenerator::WriteMinidump(HANDLE process_handle, DWORD requesting_thread_id, EXCEPTION_POINTERS* exception_pointers, MDRawAssertionInfo* assert_info, - CustomDataStream* custom_data_stream, MINIDUMP_TYPE dump_type, bool is_client_pointers, wstring* dump_path, @@ -370,9 +368,9 @@ bool MinidumpGenerator::WriteMinidump(HANDLE process_handle, breakpad_info.requesting_thread_id = requesting_thread_id; } - // Leave room in user_stream_array for possible assertion info, handle - // operations, and custom data streams. - MINIDUMP_USER_STREAM user_stream_array[4]; + // Leave room in user_stream_array for possible assertion info and handle + // operations streams. + MINIDUMP_USER_STREAM user_stream_array[3]; user_stream_array[0].Type = MD_BREAKPAD_INFO_STREAM; user_stream_array[0].BufferSize = sizeof(breakpad_info); user_stream_array[0].Buffer = &breakpad_info; @@ -416,16 +414,6 @@ bool MinidumpGenerator::WriteMinidump(HANDLE process_handle, ++user_streams.UserStreamCount; } - if (custom_data_stream) { - user_stream_array[user_streams.UserStreamCount].Type = - MD_CUSTOM_DATA_STREAM; - user_stream_array[user_streams.UserStreamCount].BufferSize = - static_cast(custom_data_stream->size); - user_stream_array[user_streams.UserStreamCount].Buffer = - custom_data_stream->stream; - ++user_streams.UserStreamCount; - } - // If the process is terminated by STATUS_INVALID_HANDLE exception store // the trace of operatios for the offending handle value. Do nothing special // if the client already requested the handle trace to be stored in the dump. diff --git a/src/client/windows/crash_generation/minidump_generator.h b/src/client/windows/crash_generation/minidump_generator.h index b2e973e6..5f9e4b54 100644 --- a/src/client/windows/crash_generation/minidump_generator.h +++ b/src/client/windows/crash_generation/minidump_generator.h @@ -33,7 +33,6 @@ #include #include #include -#include "client/windows/common/ipc_protocol.h" #include "google_breakpad/common/minidump_format.h" namespace google_breakpad { @@ -58,7 +57,6 @@ class MinidumpGenerator { DWORD requesting_thread_id, EXCEPTION_POINTERS* exception_pointers, MDRawAssertionInfo* assert_info, - CustomDataStream* custom_data_stream, MINIDUMP_TYPE dump_type, bool is_client_pointers, std::wstring* dump_path); @@ -72,7 +70,6 @@ class MinidumpGenerator { DWORD requesting_thread_id, EXCEPTION_POINTERS* exception_pointers, MDRawAssertionInfo* assert_info, - CustomDataStream* custom_data_stream, MINIDUMP_TYPE dump_type, bool is_client_pointers, std::wstring* dump_path, diff --git a/src/client/windows/unittests/minidump_test.cc b/src/client/windows/unittests/minidump_test.cc index f47ee598..ab7ae3b7 100644 --- a/src/client/windows/unittests/minidump_test.cc +++ b/src/client/windows/unittests/minidump_test.cc @@ -31,7 +31,6 @@ #include #include -#include "../common/ipc_protocol.h" #include "../crash_generation/minidump_generator.h" #include "dump_analysis.h" // NOLINT @@ -87,8 +86,7 @@ class MinidumpTest: public testing::Test { } } - bool WriteDump(ULONG flags, MDRawAssertionInfo* assert, - google_breakpad::CustomDataStream* custom_data) { + bool WriteDump(ULONG flags) { using google_breakpad::MinidumpGenerator; // Fake exception is access violation on write to this. @@ -114,8 +112,7 @@ class MinidumpTest: public testing::Test { ::GetCurrentThreadId(), ::GetCurrentThreadId(), &ex_ptrs, - assert, - custom_data, + NULL, static_cast(flags), TRUE, &dump_file_, @@ -180,7 +177,7 @@ TEST_F(MinidumpTest, Version) { } TEST_F(MinidumpTest, Normal) { - EXPECT_TRUE(WriteDump(MiniDumpNormal, NULL, NULL)); + EXPECT_TRUE(WriteDump(MiniDumpNormal)); DumpAnalysis mini(dump_file_); // We expect threads, modules and some memory. @@ -209,13 +206,10 @@ TEST_F(MinidumpTest, Normal) { // We expect no off-stack memory in this dump. EXPECT_FALSE(mini.HasMemory(this)); - - // We do not expect a custom data stream. - EXPECT_FALSE(mini.HasStream(MD_CUSTOM_DATA_STREAM)); } TEST_F(MinidumpTest, SmallDump) { - ASSERT_TRUE(WriteDump(kSmallDumpType, NULL, NULL)); + ASSERT_TRUE(WriteDump(kSmallDumpType)); DumpAnalysis mini(dump_file_); EXPECT_TRUE(mini.HasStream(ThreadListStream)); @@ -246,7 +240,7 @@ TEST_F(MinidumpTest, SmallDump) { } TEST_F(MinidumpTest, LargerDump) { - ASSERT_TRUE(WriteDump(kLargerDumpType, NULL, NULL)); + ASSERT_TRUE(WriteDump(kLargerDumpType)); DumpAnalysis mini(dump_file_); // The dump should have all of these streams. @@ -278,7 +272,7 @@ TEST_F(MinidumpTest, LargerDump) { } TEST_F(MinidumpTest, FullDump) { - ASSERT_TRUE(WriteDump(kFullDumpType, NULL, NULL)); + ASSERT_TRUE(WriteDump(kFullDumpType)); ASSERT_TRUE(dump_file_ != L""); ASSERT_TRUE(full_dump_file_ != L""); DumpAnalysis mini(dump_file_); @@ -335,15 +329,4 @@ TEST_F(MinidumpTest, FullDump) { EXPECT_FALSE(full.HasStream(TokenStream)); } -TEST_F(MinidumpTest, CustomData) { - google_breakpad::CustomDataStream custom_data; - custom_data.size = 1; - custom_data.stream[0] = 'A'; - EXPECT_TRUE(WriteDump(MiniDumpNormal, NULL, &custom_data)); - DumpAnalysis mini(dump_file_); - - // We expect a custom data stream. - EXPECT_TRUE(mini.HasStream(MD_CUSTOM_DATA_STREAM)); -} - } // namespace diff --git a/src/google_breakpad/common/minidump_format.h b/src/google_breakpad/common/minidump_format.h index 0fe7b566..28a81d41 100644 --- a/src/google_breakpad/common/minidump_format.h +++ b/src/google_breakpad/common/minidump_format.h @@ -341,8 +341,7 @@ typedef enum { MD_LINUX_ENVIRON = 0x47670007, /* /proc/$x/environ */ MD_LINUX_AUXV = 0x47670008, /* /proc/$x/auxv */ MD_LINUX_MAPS = 0x47670009, /* /proc/$x/maps */ - MD_LINUX_DSO_DEBUG = 0x4767000A, /* MDRawDebug */ - MD_CUSTOM_DATA_STREAM = 0x4767000B /* MDRawCustomDataStream */ + MD_LINUX_DSO_DEBUG = 0x4767000A /* MDRawDebug */ } MDStreamType; /* MINIDUMP_STREAM_TYPE */ @@ -735,9 +734,6 @@ typedef enum { * Breakpad extension types */ -typedef struct { - u_int8_t stream[1]; -} MDRawCustomDataStream; typedef struct { /* validity is a bitmask with values from MDBreakpadInfoValidity, indicating -- cgit v1.2.1