Fix to cache NOT_FOUND results from symbol supplier on a per-minidump basis

http://breakpad.appspot.com/64001

R=ted.mielczarek, brdevmn
A=nealsid



git-svn-id: http://google-breakpad.googlecode.com/svn/trunk@543 4c0a9323-5329-0410-9bdc-e9ce6186880e

3 files changed, 68 insertions, 2 deletions

diff --git a/src/google_breakpad/processor/stackwalker.h b/src/google_breakpad/processor/stackwalker.h
index a475b230..72da76b0 100644
--- a/src/google_breakpad/processor/stackwalker.h
+++ b/src/google_breakpad/processor/stackwalker.h
@@ -41,12 +41,13 @@
 #ifndef GOOGLE_BREAKPAD_PROCESSOR_STACKWALKER_H__
 #define GOOGLE_BREAKPAD_PROCESSOR_STACKWALKER_H__
 
-#include <vector>
+#include <set>
 #include "google_breakpad/common/breakpad_types.h"
 
 namespace google_breakpad {
 
 class CallStack;
+class CodeModule;
 class CodeModules;
 class MemoryRegion;
 class MinidumpContext;
@@ -55,7 +56,7 @@ struct StackFrame;
 class SymbolSupplier;
 class SystemInfo;
 
-using std::vector;
+using std::set;
 
 
 class Stackwalker {
@@ -139,6 +140,11 @@ class Stackwalker {
 
   // The optional SymbolSupplier for resolving source line info.
   SymbolSupplier *supplier_;
+
+  // A list of modules that we haven't found symbols for.  We track
+  // this in order to avoid repeatedly looking them up again within
+  // one minidump.
+  set<std::string> no_symbol_modules_;
 };
 
 
diff --git a/src/processor/minidump_processor_unittest.cc b/src/processor/minidump_processor_unittest.cc
index 7df12172..dd4f633e 100644
--- a/src/processor/minidump_processor_unittest.cc
+++ b/src/processor/minidump_processor_unittest.cc
@@ -34,6 +34,7 @@
 #include <string>
 #include <iostream>
 #include <fstream>
+#include <map>
 #include "breakpad_googletest_includes.h"
 #include "google_breakpad/processor/basic_source_line_resolver.h"
 #include "google_breakpad/processor/call_stack.h"
@@ -47,6 +48,8 @@
 #include "processor/logging.h"
 #include "processor/scoped_ptr.h"
 
+using std::map;
+
 namespace google_breakpad {
 class MockMinidump : public Minidump {
  public:
@@ -74,6 +77,10 @@ using google_breakpad::scoped_ptr;
 using google_breakpad::SymbolSupplier;
 using google_breakpad::SystemInfo;
 using std::string;
+using ::testing::_;
+using ::testing::Mock;
+using ::testing::Ne;
+using ::testing::Property;
 using ::testing::Return;
 
 static const char *kSystemInfoOS = "Windows NT";
@@ -155,6 +162,19 @@ SymbolSupplier::SymbolResult TestSymbolSupplier::GetSymbolFile(
   return s;
 }
 
+// A mock symbol supplier that always returns NOT_FOUND; one current
+// use for testing the processor's caching of symbol lookups.
+class MockSymbolSupplier : public SymbolSupplier {
+ public:
+  MockSymbolSupplier() { }
+  MOCK_METHOD3(GetSymbolFile, SymbolResult(const CodeModule*,
+                                           const SystemInfo*,
+                                           string*));
+  MOCK_METHOD4(GetSymbolFile, SymbolResult(const CodeModule*,
+                                           const SystemInfo*,
+                                           string*,
+                                           string*));
+};
 
 class MinidumpProcessorTest : public ::testing::Test {
 
@@ -186,6 +206,43 @@ TEST_F(MinidumpProcessorTest, TestCorruptMinidumps) {
             google_breakpad::PROCESS_ERROR_NO_THREAD_LIST);
 }
 
+// This test case verifies that the symbol supplier is only consulted
+// once per minidump per module.
+TEST_F(MinidumpProcessorTest, TestSymbolSupplierLookupCounts) {
+  MockSymbolSupplier supplier;
+  BasicSourceLineResolver resolver;
+  MinidumpProcessor processor(&supplier, &resolver);
+
+  string minidump_file = string(getenv("srcdir") ? getenv("srcdir") : ".") +
+                         "/src/processor/testdata/minidump2.dmp";
+  ProcessState state;
+  EXPECT_CALL(supplier, GetSymbolFile(
+      Property(&google_breakpad::CodeModule::code_file,
+               "c:\\test_app.exe"),
+      _, _, _)).WillOnce(Return(SymbolSupplier::NOT_FOUND));
+  EXPECT_CALL(supplier, GetSymbolFile(
+      Property(&google_breakpad::CodeModule::code_file,
+               Ne("c:\\test_app.exe")),
+      _, _, _)).WillRepeatedly(Return(SymbolSupplier::NOT_FOUND));
+  ASSERT_EQ(processor.Process(minidump_file, &state),
+            google_breakpad::PROCESS_OK);
+
+  ASSERT_TRUE(Mock::VerifyAndClearExpectations(&supplier));
+
+  // We need to verify that across minidumps, the processor will refetch
+  // symbol files, even with the same symbol supplier.
+  EXPECT_CALL(supplier, GetSymbolFile(
+      Property(&google_breakpad::CodeModule::code_file,
+               "c:\\test_app.exe"),
+      _, _, _)).WillOnce(Return(SymbolSupplier::NOT_FOUND));
+  EXPECT_CALL(supplier, GetSymbolFile(
+      Property(&google_breakpad::CodeModule::code_file,
+               Ne("c:\\test_app.exe")),
+      _, _, _)).WillRepeatedly(Return(SymbolSupplier::NOT_FOUND));
+  ASSERT_EQ(processor.Process(minidump_file, &state),
+            google_breakpad::PROCESS_OK);
+}
+
 TEST_F(MinidumpProcessorTest, TestBasicProcessing) {
   TestSymbolSupplier supplier;
   BasicSourceLineResolver resolver;
diff --git a/src/processor/stackwalker.cc b/src/processor/stackwalker.cc
index 3a3725fa..3b9a313a 100644
--- a/src/processor/stackwalker.cc
+++ b/src/processor/stackwalker.cc
@@ -93,6 +93,8 @@ bool Stackwalker::Walk(CallStack *stack) {
         frame->module = module;
         if (resolver_ &&
             !resolver_->HasModule(frame->module->code_file()) &&
+            no_symbol_modules_.find(
+                module->code_file()) == no_symbol_modules_.end() &&
             supplier_) {
           string symbol_data, symbol_file;
           SymbolSupplier::SymbolResult symbol_result =
@@ -105,6 +107,7 @@ bool Stackwalker::Walk(CallStack *stack) {
                                                   symbol_data);
               break;
             case SymbolSupplier::NOT_FOUND:
+              no_symbol_modules_.insert(module->code_file());
               break;  // nothing to do
             case SymbolSupplier::INTERRUPT:
               return false;